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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <fb8389f4-994a-b490-9adf-a6bfb370dea1@gmail.com>
Date:   Thu, 21 Mar 2019 07:42:48 -0700
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Cc:     "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        John Fastabend <john.fastabend@...il.com>,
        Ivan Vecera <ivecera@...hat.com>
Subject: Re: [PATCH net-next 1/2] net: sched: add empty status flag for NOLOCK
 qdisc



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@...hat.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;
>