netdev - Re: [PATCH net-next v3 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: <09c40ed4-b55e-e790-3fe1-83663d0c23bc@redhat.com>
Date:   Fri, 22 Mar 2019 16:24:37 +0100
From:   Ivan Vecera <ivecera@...hat.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>
Subject: Re: [PATCH net-next v3 1/2] net: sched: add empty status flag for
 NOLOCK qdisc

On 22. 03. 19 16:01, 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.
> 
> v2 -> v3:
>   - qdisc_is_empty() has a const argument (Eric)
> 
> v1 -> v2:
>   - use really an 'empty' flag instead of 'not_empty', as
>     suggested by Eric
> 
> Signed-off-by: Paolo Abeni <pabeni@...hat.com>
> ---
>   include/net/sch_generic.h | 11 +++++++++++
>   net/sched/sch_generic.c   |  3 +++
>   2 files changed, 14 insertions(+)
> 
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index 31284c078d06..e227475e78ca 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 no enqueued skbs */
> +	bool			empty;
>   	struct rcu_head		rcu;
>   };
>   
> @@ -143,11 +146,19 @@ static inline bool qdisc_is_running(struct Qdisc *qdisc)
>   	return (raw_read_seqcount(&qdisc->running) & 1) ? true : false;
>   }
>   
> +static inline bool qdisc_is_empty(const struct Qdisc *qdisc)
> +{
> +	if (qdisc->flags & TCQ_F_NOLOCK)
> +		return qdisc->empty;
> +	return !qdisc->q.qlen;
> +}
> +
>   static inline bool qdisc_run_begin(struct Qdisc *qdisc)
>   {
>   	if (qdisc->flags & TCQ_F_NOLOCK) {
>   		if (!spin_trylock(&qdisc->seqlock))
>   			return false;
> +		qdisc->empty = false;
>   	} else if (qdisc_is_running(qdisc)) {
>   		return false;
>   	}
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index a117d9260558..81356ef38d1d 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->empty = true;
>   	}
>   
>   	return skb;
> @@ -880,6 +882,7 @@ struct Qdisc *qdisc_alloc(struct netdev_queue *dev_queue,
>   	sch->enqueue = ops->enqueue;
>   	sch->dequeue = ops->dequeue;
>   	sch->dev_queue = dev_queue;
> +	sch->empty = true;
>   	dev_hold(dev);
>   	refcount_set(&sch->refcnt, 1);
>   
> 
Reviewed-by: Ivan Vecera <ivecera@...hat.com>