On 3/11/21 11:03 PM, michael.christie@xxxxxxxxxx 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@xxxxxxx +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), GF: Felix Imendörffer
