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

[Jason Wang]

在 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".


> 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.

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)) {
-               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