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

[Yunsheng Lin]

On 2021/5/28 9:31, Jason Wang wrote:
> 
> 在 2021/5/27 下午5:03, Yunsheng Lin 写道:
>> On 2021/5/27 16:05, Jason Wang wrote:
>>> 在 2021/5/27 下午3:21, Yunsheng Lin 写道:
>>>> 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?
>>>
>>> Just to make sure we are at the same page. What I really meant is "synchronized" not "serialized". So they can be called at the same time but need synchronization.
>>>
>>>
>>>> 406de7555424 ("ptr_ring: keep consumer_head valid at all times")
>>>
>>> This still needed in this case.
>>>
>>>
>>>>> /*
>>>>>    * 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?
>>>
>>> It means "the ring might be empty but __ptr_ring_empty() returns false".
>> But the ring might be non-empty but __ptr_ring_empty() returns true
>> for the data race described in the commit log:)
> 
> 
> Which commit log?

this commit log.
If the data race described in this commit log happens, the ring might be
non-empty, but __ptr_ring_empty() returns true.


> 
> 
>>
>>>
>>>> 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:)
>>>
>>> Is __ptr_ring_empty() synchronized with the consumer in your case? If yes, have you done some benchmark to see the difference?
>>>
>>> Have a look at page pool, this only helps when multiple refill request happens in parallel which can make some of the refill return early if the ring has been consumed.
>>>
>>> This is the slow-path and I'm not sure we see any difference. If one the request runs faster then the following request will go through the fast path.
>> Yes, I am agreed there may not be any difference.
>> But it is better to make it more reliable, right?
> 
> 
> No, any performance optimization must be benchmark to show obvious difference to be accepted.
> 
> ptr_ring has been used by various subsystems so we should not risk our self-eves to accept theoretical optimizations.

As a matter of fact, I am not treating it as a performance optimization for this patch.
I treated it as improvement for the checking of __ptr_ring_empty().
But you are right that we need to ensure there is not performance regression when improving
it.

Any existing and easy-to-setup testcase to benchmark the ptr_ring performance?

> 
> 
>>
>>> If it really helps, can we do it more simpler by:
>>>