Re: [PATCH net-next 1/2] net: sched: add empty status flag for NOLOCK qdisc

[Paolo Abeni <pabeni@redhat.com>]

Hi,

Thank you for the review.

On Thu, 2019-03-21 at 07:42 -0700, Eric Dumazet wrote:
> 
> On 03/21/2019 03:14 AM, Paolo Abeni wrote:
> > The queue is marked not empty after acquiring the seqlock,
> > and it's up to the NOLOCK qdisc clearing such flag on dequeue.
> > Since the empty status lays on the same cache-line of the
> > seqlock, it's always hot on cache during the updates.
> > 
> > This makes the empty flag update a little bit loosy. Given
> > the lack of synchronization between enqueue and dequeue, this
> > is unavoidable.
> > 
> 
> Seems nice !
> 
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> >  include/net/sch_generic.h | 17 +++++++++++++++++
> >  net/sched/sch_generic.c   |  2 ++
> >  2 files changed, 19 insertions(+)
> > 
> > diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> > index 31284c078d06..1dc0aeec5d39 100644
> > --- a/include/net/sch_generic.h
> > +++ b/include/net/sch_generic.h
> > @@ -113,6 +113,9 @@ struct Qdisc {
> >  
> >  	spinlock_t		busylock ____cacheline_aligned_in_smp;
> >  	spinlock_t		seqlock;
> > +
> > +	/* for NOLOCK qdisc, true if there are enqueued skbs */
> > +	bool			not_empty;
> >  	struct rcu_head		rcu;
> >  };
> >  
> > @@ -143,11 +146,25 @@ static inline bool qdisc_is_running(struct Qdisc *qdisc)
> >  	return (raw_read_seqcount(&qdisc->running) & 1) ? true : false;
> >  }
> >  
> > +static inline bool qdisc_is_empty(struct Qdisc *qdisc)
> > +{
> > +	if (qdisc->flags & TCQ_F_NOLOCK)
> > +		return !qdisc->not_empty;
> 
> double negations are not very readable IMO ...
> 
> Why not using qdisc->empty instead ?

So that the initial kzalloc() initialize this field corrently and no
explicit initialization is needed. Not a winning argument, I see. Will
use 'empty' in next version.

> > +	return !qdisc->q.qlen;
> > +}
> > +
> > +/* for NOLOCK qdisc, caller must held seqlock */
> > +static inline void qdisc_set_empty(struct Qdisc *qdisc)
> > +{
> > +	qdisc->not_empty = false;
> > +}
> > +
> I guess the helper is not really needed ?

I added this as an helper, to prepare the ground for other NOLOCK
qdiscs. Since none of them is on the horizon, I'll drop this in the
next version.

I'd like to leverage this patch in a later series to avoid the
qdisc_qstats_atomic_qlen_inc()/qdisc_qstats_atomic_qlen_dec() for
NOLOCK qdisc, as you suggested in
46b1c18f9deb326a7e18348e668e4c7ab7c7458b

In the control path we could resort to accumulate percpu values, and in
the data path we always check for empty/not empty status, AFAICS with
only one exception, in net/sched/sch_cbq.c:

static inline void
cbq_update_toplevel(struct cbq_sched_data *q, struct cbq_class *cl,
                    struct cbq_class *borrowed)
{
        if (cl && q->toplevel >= borrowed->level) {
                 if (cl->q->q.qlen > 1) {


This looks both problematic and strange to me. This test predates git
history, and the related comment is not very helpful to me. Could it be
a typo slipped in from the beginning ?!? (instead of 'cl->q->q.qlen >
0')

Thanks!

Paolo