Messages in this thread | | | Subject | Re: [PATCH net-next] ptr_ring: make __ptr_ring_empty() checking more reliable | From | Yunsheng Lin <> | Date | Fri, 28 May 2021 10:26:12 +0800 |
| |
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: >>>
| |