[PATCH net-next 0/2] sched: refactor NOLOCK qdiscs

[PATCH net-next 0/2] sched: refactor NOLOCK qdiscs

[Paolo Abeni]

With the introduction of NOLOCK qdiscs, pfifo_fast performances in the
uncontended scenario degraded measurably, especially after the commit
eb82a9944792 ("net: sched, fix OOO packets with pfifo_fast").

This series restore the pfifo_fast performances in such scenario back the
previous level, mainly reducing the number of atomic operations required to
perform the qdisc_run() call. Even performances in the contended scenario
increase measurably.

Note: This series is on top of:

sched: manipulate __QDISC_STATE_RUNNING in qdisc_run_* helpers

Paolo Abeni (2):
  sched: replace __QDISC_STATE_RUNNING bit with a spin lock
  pfifo_fast: drop unneeded additional lock on dequeue

 include/linux/skb_array.h |  5 +++++
 include/net/sch_generic.h | 10 +++++-----
 net/sched/sch_generic.c   | 15 +++++++++++++--
 3 files changed, 23 insertions(+), 7 deletions(-)

-- 
2.14.3

[PATCH net-next 1/2] sched: replace __QDISC_STATE_RUNNING bit with a spin lock

[Paolo Abeni]

So that we can use lockdep on it.
The newly introduced sequence lock has the same scope of busylock,
so it shares the same lockdep annotation, but it's only used for
NOLOCK qdiscs.

With this changeset we acquire such lock in the control path around
flushing operation (qdisc reset), to allow more NOLOCK qdisc perf
improvement in the next patch.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 include/net/sch_generic.h | 10 +++++-----
 net/sched/sch_generic.c   | 11 +++++++++++
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index 4d2b37226e75..98c10a28cd01 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -30,7 +30,6 @@ struct qdisc_rate_table {
 enum qdisc_state_t {
 	__QDISC_STATE_SCHED,
 	__QDISC_STATE_DEACTIVATED,
-	__QDISC_STATE_RUNNING,
 };
 
 struct qdisc_size_table {
@@ -102,6 +101,7 @@ struct Qdisc {
 	refcount_t		refcnt;
 
 	spinlock_t		busylock ____cacheline_aligned_in_smp;
+	spinlock_t		seqlock;
 };
 
 static inline void qdisc_refcount_inc(struct Qdisc *qdisc)
@@ -111,17 +111,17 @@ static inline void qdisc_refcount_inc(struct Qdisc *qdisc)
 	refcount_inc(&qdisc->refcnt);
 }
 
-static inline bool qdisc_is_running(const struct Qdisc *qdisc)
+static inline bool qdisc_is_running(struct Qdisc *qdisc)
 {
 	if (qdisc->flags & TCQ_F_NOLOCK)
-		return test_bit(__QDISC_STATE_RUNNING, &qdisc->state);
+		return spin_is_locked(&qdisc->seqlock);
 	return (raw_read_seqcount(&qdisc->running) & 1) ? true : false;
 }
 
 static inline bool qdisc_run_begin(struct Qdisc *qdisc)
 {
 	if (qdisc->flags & TCQ_F_NOLOCK) {
-		if (test_and_set_bit(__QDISC_STATE_RUNNING, &qdisc->state))
+		if (!spin_trylock(&qdisc->seqlock))
 			return false;
 	} else if (qdisc_is_running(qdisc)) {
 		return false;
@@ -138,7 +138,7 @@ static inline void qdisc_run_end(struct Qdisc *qdisc)
 {
 	write_seqcount_end(&qdisc->running);
 	if (qdisc->flags & TCQ_F_NOLOCK)
-		clear_bit(__QDISC_STATE_RUNNING, &qdisc->state);
+		spin_unlock(&qdisc->seqlock);
 }
 
 static inline bool qdisc_may_bulk(const struct Qdisc *qdisc)
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index ff3ce71aec93..a126f16bc30b 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -858,6 +858,11 @@ struct Qdisc *qdisc_alloc(struct netdev_queue *dev_queue,
 	lockdep_set_class(&sch->busylock,
 			  dev->qdisc_tx_busylock ?: &qdisc_tx_busylock);
 
+	/* seqlock has the same scope of busylock, for NOLOCK qdisc */
+	spin_lock_init(&sch->seqlock);
+	lockdep_set_class(&sch->busylock,
+			  dev->qdisc_tx_busylock ?: &qdisc_tx_busylock);
+
 	seqcount_init(&sch->running);
 	lockdep_set_class(&sch->running,
 			  dev->qdisc_running_key ?: &qdisc_running_key);
@@ -1097,6 +1102,10 @@ static void dev_deactivate_queue(struct net_device *dev,
 
 	qdisc = rtnl_dereference(dev_queue->qdisc);
 	if (qdisc) {
+		bool nolock = qdisc->flags & TCQ_F_NOLOCK;
+
+		if (nolock)
+			spin_lock_bh(&qdisc->seqlock);
 		spin_lock_bh(qdisc_lock(qdisc));
 
 		if (!(qdisc->flags & TCQ_F_BUILTIN))
@@ -1106,6 +1115,8 @@ static void dev_deactivate_queue(struct net_device *dev,
 		qdisc_reset(qdisc);
 
 		spin_unlock_bh(qdisc_lock(qdisc));
+		if (nolock)
+			spin_unlock_bh(&qdisc->seqlock);
 	}
 }
 
-- 
2.14.3

[PATCH net-next 2/2] pfifo_fast: drop unneeded additional lock on dequeue

[Paolo Abeni]

After the previous patch, for NOLOCK qdiscs, q->seqlock is
always held when the dequeue() is invoked, we can drop
any additional locking to protect such operation.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 include/linux/skb_array.h | 5 +++++
 net/sched/sch_generic.c   | 4 ++--
 2 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/include/linux/skb_array.h b/include/linux/skb_array.h
index a6b6e8bb3d7b..62d9b0a6329f 100644
--- a/include/linux/skb_array.h
+++ b/include/linux/skb_array.h
@@ -97,6 +97,11 @@ static inline bool skb_array_empty_any(struct skb_array *a)
 	return ptr_ring_empty_any(&a->ring);
 }
 
+static inline struct sk_buff *__skb_array_consume(struct skb_array *a)
+{
+	return __ptr_ring_consume(&a->ring);
+}
+
 static inline struct sk_buff *skb_array_consume(struct skb_array *a)
 {
 	return ptr_ring_consume(&a->ring);
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index a126f16bc30b..760ab1b09f8b 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -656,7 +656,7 @@ static struct sk_buff *pfifo_fast_dequeue(struct Qdisc *qdisc)
 		if (__skb_array_empty(q))
 			continue;
 
-		skb = skb_array_consume_bh(q);
+		skb = __skb_array_consume(q);
 	}
 	if (likely(skb)) {
 		qdisc_qstats_cpu_backlog_dec(qdisc, skb);
@@ -697,7 +697,7 @@ static void pfifo_fast_reset(struct Qdisc *qdisc)
 		if (!q->ring.queue)
 			continue;
 
-		while ((skb = skb_array_consume_bh(q)) != NULL)
+		while ((skb = __skb_array_consume(q)) != NULL)
 			kfree_skb(skb);
 	}
 
-- 
2.14.3

Re: [PATCH net-next 2/2] pfifo_fast: drop unneeded additional lock on dequeue

[Michael S. Tsirkin]

On Tue, May 15, 2018 at 04:24:37PM +0200, Paolo Abeni wrote:
> After the previous patch, for NOLOCK qdiscs, q->seqlock is
> always held when the dequeue() is invoked, we can drop
> any additional locking to protect such operation.
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  include/linux/skb_array.h | 5 +++++
>  net/sched/sch_generic.c   | 4 ++--
>  2 files changed, 7 insertions(+), 2 deletions(-)

Is the seqlock taken during qdisc_change_tx_queue_len?
We need to prevent that racing with dequeue.

> diff --git a/include/linux/skb_array.h b/include/linux/skb_array.h
> index a6b6e8bb3d7b..62d9b0a6329f 100644
> --- a/include/linux/skb_array.h
> +++ b/include/linux/skb_array.h
> @@ -97,6 +97,11 @@ static inline bool skb_array_empty_any(struct skb_array *a)
>  	return ptr_ring_empty_any(&a->ring);
>  }
>  
> +static inline struct sk_buff *__skb_array_consume(struct skb_array *a)
> +{
> +	return __ptr_ring_consume(&a->ring);
> +}
> +
>  static inline struct sk_buff *skb_array_consume(struct skb_array *a)
>  {
>  	return ptr_ring_consume(&a->ring);
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index a126f16bc30b..760ab1b09f8b 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -656,7 +656,7 @@ static struct sk_buff *pfifo_fast_dequeue(struct Qdisc *qdisc)
>  		if (__skb_array_empty(q))
>  			continue;
>  
> -		skb = skb_array_consume_bh(q);
> +		skb = __skb_array_consume(q);
>  	}
>  	if (likely(skb)) {
>  		qdisc_qstats_cpu_backlog_dec(qdisc, skb);
> @@ -697,7 +697,7 @@ static void pfifo_fast_reset(struct Qdisc *qdisc)
>  		if (!q->ring.queue)
>  			continue;
>  
> -		while ((skb = skb_array_consume_bh(q)) != NULL)
> +		while ((skb = __skb_array_consume(q)) != NULL)
>  			kfree_skb(skb);
>  	}
>  
> -- 
> 2.14.3

Re: [PATCH net-next 2/2] pfifo_fast: drop unneeded additional lock on dequeue

[Paolo Abeni]

On Tue, 2018-05-15 at 23:17 +0300, Michael S. Tsirkin wrote:
> On Tue, May 15, 2018 at 04:24:37PM +0200, Paolo Abeni wrote:
> > After the previous patch, for NOLOCK qdiscs, q->seqlock is
> > always held when the dequeue() is invoked, we can drop
> > any additional locking to protect such operation.
> > 
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> >  include/linux/skb_array.h | 5 +++++
> >  net/sched/sch_generic.c   | 4 ++--
> >  2 files changed, 7 insertions(+), 2 deletions(-)
> 
> Is the seqlock taken during qdisc_change_tx_queue_len?
> We need to prevent that racing with dequeue.

Thanks for the head-up! I missed that code-path.

I'll add the lock in qdisc_change_tx_queue_len() in v2.

Thanks you,

Paolo

Re: [PATCH net-next 2/2] pfifo_fast: drop unneeded additional lock on dequeue

[Paolo Abeni]

On Wed, 2018-05-16 at 09:56 +0200, Paolo Abeni wrote:
> On Tue, 2018-05-15 at 23:17 +0300, Michael S. Tsirkin wrote:
> > On Tue, May 15, 2018 at 04:24:37PM +0200, Paolo Abeni wrote:
> > > After the previous patch, for NOLOCK qdiscs, q->seqlock is
> > > always held when the dequeue() is invoked, we can drop
> > > any additional locking to protect such operation.
> > > 
> > > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > > ---
> > >  include/linux/skb_array.h | 5 +++++
> > >  net/sched/sch_generic.c   | 4 ++--
> > >  2 files changed, 7 insertions(+), 2 deletions(-)
> > 
> > Is the seqlock taken during qdisc_change_tx_queue_len?
> > We need to prevent that racing with dequeue.
> 
> Thanks for the head-up! I missed that code-path.
> 
> I'll add the lock in qdisc_change_tx_queue_len() in v2.

Actually the lock is not needed in qdisc_change_tx_queue_len(): the
device is deactivated before calling ops->change_tx_queue_len, so the
latter can't race with ops->dequeue().

I think the current patch is safe.

Cheers,

Paolo

Re: [PATCH net-next 2/2] pfifo_fast: drop unneeded additional lock on dequeue

[Michael S. Tsirkin]

On Wed, May 16, 2018 at 09:56:16AM +0200, Paolo Abeni wrote:
> On Tue, 2018-05-15 at 23:17 +0300, Michael S. Tsirkin wrote:
> > On Tue, May 15, 2018 at 04:24:37PM +0200, Paolo Abeni wrote:
> > > After the previous patch, for NOLOCK qdiscs, q->seqlock is
> > > always held when the dequeue() is invoked, we can drop
> > > any additional locking to protect such operation.
> > > 
> > > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > > ---
> > >  include/linux/skb_array.h | 5 +++++
> > >  net/sched/sch_generic.c   | 4 ++--
> > >  2 files changed, 7 insertions(+), 2 deletions(-)
> > 
> > Is the seqlock taken during qdisc_change_tx_queue_len?
> > We need to prevent that racing with dequeue.
> 
> Thanks for the head-up! I missed that code-path.
> 
> I'll add the lock in qdisc_change_tx_queue_len() in v2.
> 
> Thanks you,
> 
> Paolo

When you do, make sure locks nest consistently.

Re: [PATCH net-next 0/2] sched: refactor NOLOCK qdiscs

[David Miller]

From: Paolo Abeni <pabeni@redhat.com>
Date: Tue, 15 May 2018 16:24:35 +0200

> With the introduction of NOLOCK qdiscs, pfifo_fast performances in the
> uncontended scenario degraded measurably, especially after the commit
> eb82a9944792 ("net: sched, fix OOO packets with pfifo_fast").
> 
> This series restore the pfifo_fast performances in such scenario back the
> previous level, mainly reducing the number of atomic operations required to
> perform the qdisc_run() call. Even performances in the contended scenario
> increase measurably.
> 
> Note: This series is on top of:
> 
> sched: manipulate __QDISC_STATE_RUNNING in qdisc_run_* helpers

Series applied, thank you.