Re: [PATCH v5] cxlflash: Base support for IBM CXL Flash Adapter

[Brian King <brking@linux.vnet.ibm.com>]

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