Re: [PATCH v5] cxlflash: Base support for IBM CXL Flash Adapter
On Jun 9, 2015, at 1:30 PM, Brian King wrote: > On 06/09/2015 11:01 AM, Matthew R. Ochs wrote: >> >> On Jun 9, 2015, at 9:37 AM, Manoj Kumar wrote: >>> On 6/9/2015 6:29 AM, Brian King wrote: > > This was the optimization to avoid the MMIO for both threads. The other > thread that raced should > do the atomic set of afu->room to a positive value. Let's take the simpler scenario of just one thread. Let's start with afu->room = 1 We call atomic64_dec_if_positive, which results in afu->room going to zero and 0 being returned, so we go into the if leg. If afu->room is zero every time we read it from the adapter and we exhaust our retries, we return SCSI_MLQUEUE_HOST_BUSY. However, the next time we enter cxlflash_send_cmd, since afu->cmd is now 0, it will no longer get decremented, but the return value will be -1, so we'll go down the else if leg. We'll never get into the if leg again to re-read afu->room from the AFU. The simplest fix might just be to set afu->room = 1 if you ever leave the if leg without having room. >>> >>> Good suggestion. Will atomic64_set(&afu->room, 1), if we exhaust retries in >>> both legs. >> >> While I agree this will work it seems a bit of a kludge. >> >> What if we instead take advantage of our existing work queue and create a >> new work item that >> simply MMIO reads and atomically sets afu->room? With this, instead of >> slamming in a 1 to >> satisfy our logic such that a subsequent command will MMIO read, we would >> schedule the new >> work item and let afu->room be updated with a real value from the card. >> >> The only downside I see with this approach is that it has the potential to >> relax the window of time >> that we're 'down' (no room) and sending back busy...although that might not >> be such a bad thing >> if we were to get into this condition. > > Seems reasonable. Would be simpler. I assume you then just schedule the work > from cxlflash_send_cmd and > just return SCSI_MLQUEUE_HOST_BUSY from cxlflash_send_cmd if you don't have > room? Correct. We'll keep things similar to how they are now (try our best to get room on the queuecommand thread) and then if we're unable to get room (udelay/retries exhausted) we'll schedule the work and return the busy rc. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] cxlflash: Base support for IBM CXL Flash Adapter
On 06/09/2015 11:01 AM, Matthew R. Ochs wrote: > Brian/Manoj, > > See my alternate proposal below. > > -matt > > On Jun 9, 2015, at 9:37 AM, Manoj Kumar wrote: >> On 6/9/2015 6:29 AM, Brian King wrote: This was the optimization to avoid the MMIO for both threads. The other thread that raced should do the atomic set of afu->room to a positive value. >>> >>> Let's take the simpler scenario of just one thread. >>> >>> Let's start with afu->room = 1 >>> We call atomic64_dec_if_positive, which results in afu->room going to zero >>> and 0 being returned, >>> so we go into the if leg. >>> >>> If afu->room is zero every time we read it from the adapter and we exhaust >>> our retries, >>> we return SCSI_MLQUEUE_HOST_BUSY. However, the next time we enter >>> cxlflash_send_cmd, >>> since afu->cmd is now 0, it will no longer get decremented, but the return >>> value will >>> be -1, so we'll go down the else if leg. We'll never get into the if leg >>> again to >>> re-read afu->room from the AFU. The simplest fix might just be to set >>> afu->room = 1 >>> if you ever leave the if leg without having room. >> >> Good suggestion. Will atomic64_set(&afu->room, 1), if we exhaust retries in >> both legs. > > While I agree this will work it seems a bit of a kludge. > > What if we instead take advantage of our existing work queue and create a new > work item that > simply MMIO reads and atomically sets afu->room? With this, instead of > slamming in a 1 to > satisfy our logic such that a subsequent command will MMIO read, we would > schedule the new > work item and let afu->room be updated with a real value from the card. > > The only downside I see with this approach is that it has the potential to > relax the window of time > that we're 'down' (no room) and sending back busy...although that might not > be such a bad thing > if we were to get into this condition. Seems reasonable. Would be simpler. I assume you then just schedule the work from cxlflash_send_cmd and just return SCSI_MLQUEUE_HOST_BUSY from cxlflash_send_cmd if you don't have room? -Brian -- Brian King Power Linux I/O IBM Linux Technology Center -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] cxlflash: Base support for IBM CXL Flash Adapter
Brian/Manoj, See my alternate proposal below. -matt On Jun 9, 2015, at 9:37 AM, Manoj Kumar wrote: > On 6/9/2015 6:29 AM, Brian King wrote: >>> >>> This was the optimization to avoid the MMIO for both threads. The other >>> thread that raced should >>> do the atomic set of afu->room to a positive value. >> >> Let's take the simpler scenario of just one thread. >> >> Let's start with afu->room = 1 >> We call atomic64_dec_if_positive, which results in afu->room going to zero >> and 0 being returned, >> so we go into the if leg. >> >> If afu->room is zero every time we read it from the adapter and we exhaust >> our retries, >> we return SCSI_MLQUEUE_HOST_BUSY. However, the next time we enter >> cxlflash_send_cmd, >> since afu->cmd is now 0, it will no longer get decremented, but the return >> value will >> be -1, so we'll go down the else if leg. We'll never get into the if leg >> again to >> re-read afu->room from the AFU. The simplest fix might just be to set >> afu->room = 1 >> if you ever leave the if leg without having room. > > Good suggestion. Will atomic64_set(&afu->room, 1), if we exhaust retries in > both legs. While I agree this will work it seems a bit of a kludge. What if we instead take advantage of our existing work queue and create a new work item that simply MMIO reads and atomically sets afu->room? With this, instead of slamming in a 1 to satisfy our logic such that a subsequent command will MMIO read, we would schedule the new work item and let afu->room be updated with a real value from the card. The only downside I see with this approach is that it has the potential to relax the window of time that we're 'down' (no room) and sending back busy...although that might not be such a bad thing if we were to get into this condition. Thoughts? -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] cxlflash: Base support for IBM CXL Flash Adapter
On 6/9/2015 6:29 AM, Brian King wrote: Pulled out going to sleep in the queuecommand path. udelay doesn't sleep, its a busy wait, so you can still use it in queuecommand, just don't spend too much time, and its probably better to udelay then to just re-read in a tight loop. Thanks for the clarification. Will update in the next patch (v6). This was the optimization to avoid the MMIO for both threads. The other thread that raced should do the atomic set of afu->room to a positive value. Let's take the simpler scenario of just one thread. Let's start with afu->room = 1 We call atomic64_dec_if_positive, which results in afu->room going to zero and 0 being returned, so we go into the if leg. If afu->room is zero every time we read it from the adapter and we exhaust our retries, we return SCSI_MLQUEUE_HOST_BUSY. However, the next time we enter cxlflash_send_cmd, since afu->cmd is now 0, it will no longer get decremented, but the return value will be -1, so we'll go down the else if leg. We'll never get into the if leg again to re-read afu->room from the AFU. The simplest fix might just be to set afu->room = 1 if you ever leave the if leg without having room. Good suggestion. Will atomic64_set(&afu->room, 1), if we exhaust retries in both legs. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] cxlflash: Base support for IBM CXL Flash Adapter
On 06/08/2015 06:24 PM, Manoj Kumar wrote:
> On 6/8/2015 5:56 PM, Brian King wrote:
>>> +retry:
>>> +newval = atomic64_dec_if_positive(&afu->room);
>>> +if (!newval) {
>>> +do {
>>> +room = readq_be(&afu->host_map->cmd_room);
>>> +atomic64_set(&afu->room, room);
>>> +if (room)
>>> +goto write_ioarrin;
>>> +} while (nretry++ < MC_ROOM_RETRY_CNT);
>>
>> It looks like you removed the udelay here. Was that intentional?
>
> Pulled out going to sleep in the queuecommand path.
udelay doesn't sleep, its a busy wait, so you can still use it in queuecommand,
just don't spend too much time, and its probably better to udelay then to
just re-read in a tight loop.
>
>>> +
>>> +pr_err("%s: no cmd_room to send 0x%X\n",
>>> + __func__, cmd->rcb.cdb[0]);
>>> +rc = SCSI_MLQUEUE_HOST_BUSY;
>>
>> If you actually get here, how do you get out of this state? Since now
>> afu->room is
>> zero and anyone that comes through here will go to the else if leg.
>
> This was the optimization to avoid the MMIO for both threads. The other
> thread that raced should do the atomic set of afu->room to a positive value.
Let's take the simpler scenario of just one thread.
Let's start with afu->room = 1
We call atomic64_dec_if_positive, which results in afu->room going to zero and
0 being returned,
so we go into the if leg.
If afu->room is zero every time we read it from the adapter and we exhaust our
retries,
we return SCSI_MLQUEUE_HOST_BUSY. However, the next time we enter
cxlflash_send_cmd,
since afu->cmd is now 0, it will no longer get decremented, but the return
value will
be -1, so we'll go down the else if leg. We'll never get into the if leg again
to
re-read afu->room from the AFU. The simplest fix might just be to set afu->room
= 1
if you ever leave the if leg without having room.
+ newval = atomic64_dec_if_positive(&afu->room);
+ if (!newval) {
+ do {
+ room = readq_be(&afu->host_map->cmd_room);
+ atomic64_set(&afu->room, room);
+ if (room)
+ goto write_ioarrin;
+ } while (nretry++ < MC_ROOM_RETRY_CNT);
+
+ pr_err("%s: no cmd_room to send 0x%X\n",
+ __func__, cmd->rcb.cdb[0]);
+ rc = SCSI_MLQUEUE_HOST_BUSY;
+ goto out;
+ } else if (unlikely(newval < 0)) {
+ /* This should be rare. i.e. Only if two threads race and
+* decrement before the MMIO read is done. In this case
+* just benefit from the other thread having updated
+* afu->room.
+*/
+ if (nretry++ < MC_ROOM_RETRY_CNT)
+ goto retry;
+ else {
+ rc = SCSI_MLQUEUE_HOST_BUSY;
+ goto out;
+ }
+ }
>
>>> +goto out;
>>> +} else if (unlikely(newval < 0)) {
>>> +/* This should be rare. i.e. Only if two threads race and
>>> + * decrement before the MMIO read is done. In this case
>>> + * just benefit from the other thread having updated
>>> + * afu->room.
>>> + */
>>> +if (nretry++ < MC_ROOM_RETRY_CNT)
>>
--
Brian King
Power Linux I/O
IBM Linux Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] cxlflash: Base support for IBM CXL Flash Adapter
Brian:
Thanks for the quick review. Comments below.
- Manoj
On 6/8/2015 5:56 PM, Brian King wrote:
+retry:
+ newval = atomic64_dec_if_positive(&afu->room);
+ if (!newval) {
+ do {
+ room = readq_be(&afu->host_map->cmd_room);
+ atomic64_set(&afu->room, room);
+ if (room)
+ goto write_ioarrin;
+ } while (nretry++ < MC_ROOM_RETRY_CNT);
It looks like you removed the udelay here. Was that intentional?
Pulled out going to sleep in the queuecommand path.
+
+ pr_err("%s: no cmd_room to send 0x%X\n",
+ __func__, cmd->rcb.cdb[0]);
+ rc = SCSI_MLQUEUE_HOST_BUSY;
If you actually get here, how do you get out of this state? Since now afu->room
is
zero and anyone that comes through here will go to the else if leg.
This was the optimization to avoid the MMIO for both threads. The other
thread that raced should do the atomic set of afu->room to a positive value.
+ goto out;
+ } else if (unlikely(newval < 0)) {
+ /* This should be rare. i.e. Only if two threads race and
+* decrement before the MMIO read is done. In this case
+* just benefit from the other thread having updated
+* afu->room.
+*/
+ if (nretry++ < MC_ROOM_RETRY_CNT)
I'm guessing you'd want the udelay here as well.
Same reason as the queuecommand issue above.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5] cxlflash: Base support for IBM CXL Flash Adapter
On 06/08/2015 05:30 PM, Matthew R. Ochs wrote:
> +
> +/**
> + * cxlflash_send_cmd() - sends an AFU command
> + * @afu: AFU associated with the host.
> + * @cmd: AFU command to send.
> + *
> + * Return:
> + * 0 on success
> + * -1 on failure
> + */
> +int cxlflash_send_cmd(struct afu *afu, struct afu_cmd *cmd)
> +{
> + int nretry = 0;
> + int rc = 0;
> + u64 room;
> + long newval;
> +
> + /*
> + * This routine is used by critical users such an AFU sync and to
> + * send a task management function (TMF). Thus we want to retry a
> + * bit before returning an error. To avoid the performance penalty
> + * of MMIO, we spread the update of 'room' over multiple commands.
> + */
> +retry:
> + newval = atomic64_dec_if_positive(&afu->room);
> + if (!newval) {
> + do {
> + room = readq_be(&afu->host_map->cmd_room);
> + atomic64_set(&afu->room, room);
> + if (room)
> + goto write_ioarrin;
> + } while (nretry++ < MC_ROOM_RETRY_CNT);
It looks like you removed the udelay here. Was that intentional?
> +
> + pr_err("%s: no cmd_room to send 0x%X\n",
> +__func__, cmd->rcb.cdb[0]);
> + rc = SCSI_MLQUEUE_HOST_BUSY;
If you actually get here, how do you get out of this state? Since now afu->room
is
zero and anyone that comes through here will go to the else if leg.
> + goto out;
> + } else if (unlikely(newval < 0)) {
> + /* This should be rare. i.e. Only if two threads race and
> + * decrement before the MMIO read is done. In this case
> + * just benefit from the other thread having updated
> + * afu->room.
> + */
> + if (nretry++ < MC_ROOM_RETRY_CNT)
I'm guessing you'd want the udelay here as well.
> + goto retry;
> + else {
> + rc = SCSI_MLQUEUE_HOST_BUSY;
> + goto out;
> + }
> + }
> +
> +write_ioarrin:
> + writeq_be((u64)&cmd->rcb, &afu->host_map->ioarrin);
> +out:
> + pr_debug("%s: cmd=%p len=%d ea=%p rc=%d\n", __func__, cmd,
> + cmd->rcb.data_len, (void *)cmd->rcb.data_ea, rc);
> + return rc;
> +}
> +
--
Brian King
Power Linux I/O
IBM Linux Technology Center
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
