On Thu, Jan 13, 2011 at 4:21 PM, David Dillow <[email protected]> wrote:
> On Thu, 2011-01-13 at 06:19 -0500, Bart Van Assche wrote:
>> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c
>> b/drivers/infiniband/ulp/srp/ib_srp.c
>> index 4b62105..3164c4d 100644
>> --- a/drivers/infiniband/ulp/srp/ib_srp.c
>> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
>> @@ -1132,16 +1132,12 @@ static int srp_queuecommand(struct Scsi_Host
>> *shost, struct scsi_cmnd *scmnd)
>>
>> spin_lock_irqsave(&target->lock, flags);
>> iu = __srp_get_tx_iu(target, SRP_IU_CMD);
>> - if (iu) {
>> - req = list_first_entry(&target->free_reqs, struct srp_request,
>> - list);
>> - list_del(&req->list);
>> - }
>> + if (unlikely(!iu))
>
> Please drop the unlikely -- it isn't at all unlikely on the hardware and
> loads I run.
OK, I have dropped "unlikely()" in the patch I have just posted - the
branch prediction circuitry in a CPU should be perfectly capable of
detecting in which direction that branch is taken.
But it surprises me that on the hardware and loads you run it
regularly happens that iu == NULL -- a recent patch had the exact
purpose to avoid that ("IB/srp: Reduce number of BUSY conditions").
With that patch it should only occur that iu == NULL if the target
violates the SRP spec by sending target-initiated information units so
fast that there is more than one unanswered request.
>> + goto err_unlock;
>> + req = list_first_entry(&target->free_reqs, struct srp_request, list);
>> + list_del(&req->list);
>> spin_unlock_irqrestore(&target->lock, flags);
>>
>> - if (!iu)
>> - goto err;
>> -
>> dev = target->srp_host->srp_dev->dev;
>> ib_dma_sync_single_for_cpu(dev, iu->dma, srp_max_iu_len,
>> DMA_TO_DEVICE);
>> @@ -1185,6 +1181,7 @@ err_iu:
>>
>> spin_lock_irqsave(&target->lock, flags);
>> list_add(&req->list, &target->free_reqs);
>> +err_unlock:
>
> Please add a line before the label.
OK, done.
> If Roland would like to rebase his tree and substitute your patch for my
> quickie, then I'm fine with that and you can have my Acked-by with the
> above changes. If not, then make it relative to the uninit var change
> and I'll be happy to put it in my pull request once the workqueue stuff
> is sorted.
The patch I have sent is relative to Linus' linux-2.6.git tree, so
removing the uninit var change is a separate step. Roland, I assume
that it's not necessary to post a separate patch for reverting that
change ?
Bart.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
