On 11/14/18 1:43 AM, Christoph Hellwig wrote:
>> diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
>> index 6aa86dfcb32c..a6e3fbddfadf 100644
>> --- a/drivers/nvme/host/pci.c
>> +++ b/drivers/nvme/host/pci.c
>> @@ -1061,15 +1061,26 @@ static irqreturn_t nvme_irq_check(int irq, void *data)
>>
>> static int __nvme_poll(struct nvme_queue *nvmeq, unsigned int tag)
>> {
>> + unsigned long flags = 0; /* gcc 7.x fail */
>> u16 start, end;
>> - bool found;
>> + bool found, has_irq;
>>
>> if (!nvme_cqe_pending(nvmeq))
>> return 0;
>>
>> - spin_lock_irq(&nvmeq->cq_lock);
>> + /*
>> + * Polled queue doesn't have an IRQ, no need to disable ints
>> + */
>> + has_irq = !nvmeq->polled;
>> + if (has_irq)
>> + local_irq_save(flags);
>> +
>> + spin_lock(&nvmeq->cq_lock);
>> found = nvme_process_cq(nvmeq, &start, &end, tag);
>> - spin_unlock_irq(&nvmeq->cq_lock);
>> + spin_unlock(&nvmeq->cq_lock);
>> +
>> + if (has_irq)
>> + local_irq_restore(flags);
>
> Eww, this just looks ugly. Given that we are micro-optimizing here
> I'd rather just use a different ->poll implementation and thus blk_mq_ops
> if we have a separate poll code. Then we could leave the existing
> __nvme_poll as-is and have a new nvme_poll_noirq something like this:
Yeah good point, gets rid of those branches too. I'll make that change.
> And while we are at it: I think for the irq-driven queuest in a
> separate queue for poll setup we might not even need to take the
> CQ lock. Which might be an argument for only allowing polling
> if we have the separate queues just to keep everything simple.
We can definitely move in that direction, I'll take a look at what
is feasible.
--
Jens Axboe
