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

On 2021/5/27 14:53, Jason Wang wrote:

在 2021/5/27 下午2:07, Yunsheng Lin 写道:

quoted

On 2021/5/27 12:57, Jason Wang wrote:

quoted

在 2021/5/26 下午8:29, Yunsheng Lin 写道:

quoted

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.

quoted

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