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
| ||
|
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; >
Powered by blists - more mailing lists