Re: [PATCH] percpu_ida: Handle out-of-tags gracefully

[Bart Van Assche <bvanassche@acm.org>]

On 03/11/14 21:48, Alexander Gordeev wrote:
> On Tue, Mar 11, 2014 at 07:10:18PM +0100, Bart Van Assche wrote:
>>> I assume the BUG() above hits? If so, I am failing to understand how
>>> the code gets here. Mind elaborate?
>>
>> You are correct, the BUG() mentioned in the call stack in the
>> description of this patch does indeed correspond with the BUG()
>> statement in the above code. That BUG() was encountered while testing
>> the scsi-mq patch series with a workload with a large queue depth. I
>> think the fact that I hit that BUG() statement means that my workload
>> was queueing requests faster than these were processed by the SCSI LLD
>> and hence that percpu_ida_alloc() ran out of tags.
> 
> Function steal_tags() is entered with disabled interrupts and
> pool->lock taken. Then the 'for' cycle enters/loops while 'cpus_have_tags'
> is not zero. Which means we can not end up with no set bits at all -
> and that is the reason why BUG() is (legitimately) placed there.

Sorry but the above reasoning is wrong. Even if interrupts are disabled
on one CPU, even if that CPU holds pool->lock, and even if
cpus_have_tags has at least one bit set at the time steal_tags() starts,
it is still possible that another CPU obtains "remote->lock" before
steal_tags() can obtain that lock and that that other CPU causes
remote->nr_free to drop to zero. I am aware the percpu_ida code is not
easy to read due to such complex interactions between CPU cores.
However, my understanding is that the goal of the percpu_ida allocator
was not that its code would be easy to read but that its performance
would be optimal.

Is this sufficient to make you have another look at my patch ?

Thanks,

Bart.