Message ID | 5c30962d8213c8483aee810aa447028e56de963e.1553263445.git.pabeni@redhat.com |
---|---|
State | Accepted |
Delegated to: | David Miller |
Headers | show |
Series | net: dev: BYPASS for lockless qdisc | expand |
On 03/22/2019 08:01 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. > Reviewed-by: Eric Dumazet <edumazet@google.com>
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@redhat.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@redhat.com>
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);
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@redhat.com> --- include/net/sch_generic.h | 11 +++++++++++ net/sched/sch_generic.c | 3 +++ 2 files changed, 14 insertions(+)