Re: [PATCH net-next] ptr_ring: make __ptr_ring_empty() checking more reliable

[Yunsheng Lin <linyunsheng@huawei.com>]

On 2021/5/27 14:53, Jason Wang wrote:
> 
> 在 2021/5/27 下午2:07, Yunsheng Lin 写道:
>> On 2021/5/27 12:57, Jason Wang wrote:
>>> 在 2021/5/26 下午8:29, Yunsheng Lin 写道:
>>>> Currently r->queue[] is cleared after r->consumer_head is moved
>>>> forward, which makes the __ptr_ring_empty() checking called in
>>>> page_pool_refill_alloc_cache() unreliable if the checking is done
>>>> after the r->queue clearing and before the consumer_head moving
>>>> forward.
>>>>
>>>> Move the r->queue[] clearing after consumer_head moving forward
>>>> to make __ptr_ring_empty() checking more reliable.
>>>
>>> If I understand this correctly, this can only happens if you run __ptr_ring_empty() in parallel with ptr_ring_discard_one().
>> Yes.
>>
>>> I think those two needs to be serialized. Or did I miss anything?
>> As the below comment in __ptr_ring_discard_one, if the above is true, I
>> do not think we need to keep consumer_head valid at all times, right?
>>
>>
>>     /* Note: we must keep consumer_head valid at all times for __ptr_ring_empty
>>      * to work correctly.
>>      */
> 
> 
> I'm not sure I understand. But my point is that you need to synchronize the __ptr_ring_discard_one() and __ptr_empty() as explained in the comment above __ptr_ring_empty():

I am saying if __ptr_ring_empty() and __ptr_ring_discard_one() is
always serialized, then it seems that the below commit is unnecessary?

406de7555424 ("ptr_ring: keep consumer_head valid at all times")

> 
> /*
>  * Test ring empty status without taking any locks.
>  *
>  * NB: This is only safe to call if ring is never resized.
>  *
>  * However, if some other CPU consumes ring entries at the same time, the value
>  * returned is not guaranteed to be correct.
>  *
>  * In this case - to avoid incorrectly detecting the ring
>  * as empty - the CPU consuming the ring entries is responsible
>  * for either consuming all ring entries until the ring is empty,
>  * or synchronizing with some other CPU and causing it to
>  * re-test __ptr_ring_empty and/or consume the ring enteries
>  * after the synchronization point.

I am not sure I understand "incorrectly detecting the ring as empty"
means, is it because of the data race described in the commit log?
Or other data race? I can not think of other data race if consuming
and __ptr_ring_empty() is serialized:)

I am agreed that __ptr_ring_empty() checking is not totally reliable
without taking r->consumer_lock, that is why I use "more reliable"
in the title:)



>  *
>  * Note: callers invoking this in a loop must use a compiler barrier,
>  * for example cpu_relax().
>  */
> 
> Thanks
> 
> 
>