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

From: Jason Wang
Date: Thu May 27 2021 - 21:32:19 EST


在 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?




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.



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


diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
index 808f9d3ee546..c3a72dc77337 100644
--- a/include/linux/ptr_ring.h
+++ b/include/linux/ptr_ring.h
@@ -264,6 +264,10 @@ static inline void __ptr_ring_discard_one(struct ptr_ring *r)
int consumer_head = r->consumer_head;
int head = consumer_head++;

+ /* matching READ_ONCE in __ptr_ring_empty for lockless tests */
+ WRITE_ONCE(r->consumer_head,
+ consumer_head < r->size ? consumer_head : 0);
+
/* Once we have processed enough entries invalidate them in
* the ring all at once so producer can reuse their space in the ring.
* We also do this when we reach end of the ring - not mandatory
@@ -281,11 +285,8 @@ static inline void __ptr_ring_discard_one(struct ptr_ring *r)
r->consumer_tail = consumer_head;
}
if (unlikely(consumer_head >= r->size)) {
What I am thinking is that we can remove the above testing for
the likely case when the above checking is moved into the body
of "if (unlikely(consumer_head - r->consumer_tail >= r->batch ||
consumer_head >= r->size))".

Or is there any specific reason why we keep the testing for likely
case?


No reason but any optimization must be tested to show differences before being accepted.

Thanks




- consumer_head = 0;
r->consumer_tail = 0;
}
- /* matching READ_ONCE in __ptr_ring_empty for lockless tests */
- WRITE_ONCE(r->consumer_head, consumer_head);
}

static inline void *__ptr_ring_consume(struct ptr_ring *r)


Thanks




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

Thanks




.