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

From: Jason Wang
Date: Thu May 27 2021 - 22:29:39 EST



在 2021/5/28 上午10:26, 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?


You probably can start with a simple test in:

tools/virtio/ringtest/ptr_ring.c

Thanks




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