Re: [PATCH net-next v7 2/9] ptr_ring: add helper to detect newly freed space on consume

["Michael S. Tsirkin" <mst@redhat.com>]

On Fri, Jan 09, 2026 at 02:01:54PM +0800, Jason Wang wrote:
> On Thu, Jan 8, 2026 at 3:21 PM Simon Schippers
> <simon.schippers@tu-dortmund.de> wrote:
> >
> > On 1/8/26 04:23, Jason Wang wrote:
> > > On Thu, Jan 8, 2026 at 5:06 AM Simon Schippers
> > > <simon.schippers@tu-dortmund.de> wrote:
> > >>
> > >> This proposed function checks whether __ptr_ring_zero_tail() was invoked
> > >> within the last n calls to __ptr_ring_consume(), which indicates that new
> > >> free space was created. Since __ptr_ring_zero_tail() moves the tail to
> > >> the head - and no other function modifies either the head or the tail,
> > >> aside from the wrap-around case described below - detecting such a
> > >> movement is sufficient to detect the invocation of
> > >> __ptr_ring_zero_tail().
> > >>
> > >> The implementation detects this movement by checking whether the tail is
> > >> at most n positions behind the head. If this condition holds, the shift
> > >> of the tail to its current position must have occurred within the last n
> > >> calls to __ptr_ring_consume(), indicating that __ptr_ring_zero_tail() was
> > >> invoked and that new free space was created.
> > >>
> > >> This logic also correctly handles the wrap-around case in which
> > >> __ptr_ring_zero_tail() is invoked and the head and the tail are reset
> > >> to 0. Since this reset likewise moves the tail to the head, the same
> > >> detection logic applies.
> > >>
> > >> Co-developed-by: Tim Gebauer <tim.gebauer@tu-dortmund.de>
> > >> Signed-off-by: Tim Gebauer <tim.gebauer@tu-dortmund.de>
> > >> Signed-off-by: Simon Schippers <simon.schippers@tu-dortmund.de>
> > >> ---
> > >>  include/linux/ptr_ring.h | 13 +++++++++++++
> > >>  1 file changed, 13 insertions(+)
> > >>
> > >> diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
> > >> index a5a3fa4916d3..7cdae6d1d400 100644
> > >> --- a/include/linux/ptr_ring.h
> > >> +++ b/include/linux/ptr_ring.h
> > >> @@ -438,6 +438,19 @@ static inline int ptr_ring_consume_batched_bh(struct ptr_ring *r,
> > >>         return ret;
> > >>  }
> > >>
> > >> +/* Returns true if the consume of the last n elements has created space
> > >> + * in the ring buffer (i.e., a new element can be produced).
> > >> + *
> > >> + * Note: Because of batching, a successful call to __ptr_ring_consume() /
> > >> + * __ptr_ring_consume_batched() does not guarantee that the next call to
> > >> + * __ptr_ring_produce() will succeed.
> > >
> > > This sounds like a bug that needs to be fixed, as it requires the user
> > > to know the implementation details. For example, even if
> > > __ptr_ring_consume_created_space() returns true, __ptr_ring_produce()
> > > may still fail?
> >
> > No, it should not fail in that case.
> > If you only call consume and after that try to produce, *then* it is
> > likely to fail because __ptr_ring_zero_tail() is only invoked once per
> > batch.
> 
> Well, this makes the helper very hard for users.
> 
> So I think at least the documentation should specify the meaning of
> 'n' here.

Right. Documenting parameters is good.

> For example, is it the value returned by
> ptr_ring_consume_batched()(), and is it required to be called
> immediately after ptr_ring_consume_batched()? If it is, the API is
> kind of tricky to be used, we should consider to merge two helpers
> into a new single helper to ease the user.

I think you are right partially it's more a question of documentation and naming.
It's not that it's hard to use: follow up patches use it
without issues - it's that neither documentatin nor
naming explain how.

let's try to document, first of all: if it does not guarantee that
produce will succeed, then what *is* the guarantee this API gives?

> 
> What's more, there would be false positives. Considering there's not
> many entries in the ring, just after the first zeroing,
> __ptr_ring_consume_created_space() will return true, this will lead to
> unnecessary wakeups.

well optimizations are judged on their performance not on theoretical
analysis. in this instance, this should be rare enough.

> 
> And last, the function will always succeed if n is greater than the batch.
> 
> >
> > >
> > > Maybe revert fb9de9704775d?
> >
> > I disagree, as I consider this to be one of the key features of ptr_ring.
> 
> Nope, it's just an optimization and actually it changes the behaviour
> that might be noticed by the user.
> 
> Before the patch, ptr_ring_produce() is guaranteed to succeed after a
> ptr_ring_consume(). After that patch, it's not. We don't see complaint
> because the implication is not obvious (e.g more packet dropping).
> 
> >
> > That said, there are several other implementation details that users need
> > to be aware of.
> >
> > For example, __ptr_ring_empty() must only be called by the consumer. This
> > was for example the issue in dc82a33297fc ("veth: apply qdisc
> > backpressure on full ptr_ring to reduce TX drops") and the reason why
> > 5442a9da6978 ("veth: more robust handing of race to avoid txq getting
> > stuck") exists.
> 
> At least the behaviour is documented. This is not the case for the
> implications of fb9de9704775d.
> 
> Thanks
> 
> 
> >
> > >
> > >> + */
> > >> +static inline bool __ptr_ring_consume_created_space(struct ptr_ring *r,
> > >> +                                                   int n)
> > >> +{
> > >> +       return r->consumer_head - r->consumer_tail < n;
> > >> +}
> > >> +
> > >>  /* Cast to structure type and call a function without discarding from FIFO.
> > >>   * Function must return a value.
> > >>   * Callers must take consumer_lock.
> > >> --
> > >> 2.43.0
> > >>
> > >
> > > Thanks
> > >
> >