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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
