Re: [PATCH 14/31] hpsa: use reserved commands

[Hannes Reinecke <hare@suse.de>]

On 3/11/21 11:03 PM, michael.christie@oracle.com wrote:
> On 2/22/21 7:23 AM, Hannes Reinecke wrote:
>> -
>> -static struct CommandList *cmd_alloc(struct ctlr_info *h)
>> +static struct CommandList *cmd_alloc(struct ctlr_info *h, u8 direction)
>>  {
>> +	struct scsi_cmnd *scmd;
>>  	struct CommandList *c;
>> -	int refcount, i;
>> -	int offset = 0;
>> -
>> -	/*
>> -	 * There is some *extremely* small but non-zero chance that that
>> -	 * multiple threads could get in here, and one thread could
>> -	 * be scanning through the list of bits looking for a free
>> -	 * one, but the free ones are always behind him, and other
>> -	 * threads sneak in behind him and eat them before he can
>> -	 * get to them, so that while there is always a free one, a
>> -	 * very unlucky thread might be starved anyway, never able to
>> -	 * beat the other threads.  In reality, this happens so
>> -	 * infrequently as to be indistinguishable from never.
>> -	 *
>> -	 * Note that we start allocating commands before the SCSI host structure
>> -	 * is initialized.  Since the search starts at bit zero, this
>> -	 * all works, since we have at least one command structure available;
>> -	 * however, it means that the structures with the low indexes have to be
>> -	 * reserved for driver-initiated requests, while requests from the block
>> -	 * layer will use the higher indexes.
>> -	 */
>> -
>> -	for (;;) {
>> -		i = find_next_zero_bit(h->cmd_pool_bits,
>> -					HPSA_NRESERVED_CMDS,
>> -					offset);
>> -		if (unlikely(i >= HPSA_NRESERVED_CMDS)) {
>> -			offset = 0;
>> -			continue;
>> -		}
>> -		c = h->cmd_pool + i;
>> -		refcount = atomic_inc_return(&c->refcount);
>> -		if (unlikely(refcount > 1)) {
>> -			cmd_free(h, c); /* already in use */
>> -			offset = (i + 1) % HPSA_NRESERVED_CMDS;
>> -			continue;
>> -		}
>> -		set_bit(i & (BITS_PER_LONG - 1),
>> -			h->cmd_pool_bits + (i / BITS_PER_LONG));
>> -		break; /* it's ours now. */
>> +	int idx;
>> +
>> +	scmd = scsi_get_internal_cmd(h->raid_ctrl_sdev,
>> +				     (direction & XFER_WRITE) ?
>> +				     DMA_TO_DEVICE : DMA_FROM_DEVICE,
>> +				     REQ_NOWAIT);
>> +	if (!scmd) {
>> +		dev_warn(&h->pdev->dev, "failed to allocate reserved cmd\n");
>> +		return NULL;
> 
> I think in the orig code cmd_alloc would always return a non null pointer.
> It looks like we would always just keep looping.
> 
> Now, it looks like we could fail from the above code where we return NULL.
> I was not sure if it's maybe impossible to hit the "return NULL" becuase we
> only call this function when we know there will be a cmd availale. If we
> can fail then the cmd_alloc callers should check for NULL now I think.
> 
Yes, that's indeed the case with this patch.
But seeing that the original code would spin until a tag becomes
available we can drop the REQ_NOWAIT flags and should get roughly the
same behaviour as we have now.

Will be updating the patch.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		           Kernel Storage Architect
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer