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

[Eric Dumazet <eric.dumazet@gmail.com>]

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 ?


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


>  static inline bool qdisc_run_begin(struct Qdisc *qdisc)
>  {
>  	if (qdisc->flags & TCQ_F_NOLOCK) {
>  		if (!spin_trylock(&qdisc->seqlock))
>  			return false;
> +		qdisc->not_empty = true;

Not clear why you have a qdisc_set_empty() helper only for one case.



>  	} else if (qdisc_is_running(qdisc)) {
>  		return false;
>  	}
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index a117d9260558..3b03c6b9be1e 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -671,6 +671,8 @@ static struct sk_buff *pfifo_fast_dequeue(struct Qdisc *qdisc)
>  		qdisc_qstats_cpu_backlog_dec(qdisc, skb);
>  		qdisc_bstats_cpu_update(qdisc, skb);
>  		qdisc_qstats_atomic_qlen_dec(qdisc);
> +	} else {
> +		qdisc_set_empty(qdisc);
>  	}
>  
>  	return skb;
>