[Patch net-next v2 0/3] net_sched: reflect tx_queue_len change for pfifo_fast

[Patch net-next v2 0/3] net_sched: reflect tx_queue_len change for pfifo_fast

[Cong Wang]

This pathcset restores the pfifo_fast qdisc behavior of dropping
packets based on latest dev->tx_queue_len. Patch 1 introduces
a helper, patch 2 introduces a new Qdisc ops which is called when
we modify tx_queue_len, patch 3 implements this ops for pfifo_fast.

Please see each patch for details.

---
v2: handle error case for ->change_tx_queue_len()

Cong Wang (3):
  net: introduce helper dev_change_tx_queue_len()
  net_sched: plug in qdisc ops change_tx_queue_len
  net_sched: implement ->change_tx_queue_len() for pfifo_fast

 include/linux/netdevice.h |  1 +
 include/net/sch_generic.h |  2 ++
 net/core/dev.c            | 29 +++++++++++++++++++++++++++
 net/core/net-sysfs.c      | 25 +-----------------------
 net/core/rtnetlink.c      | 18 +++++------------
 net/sched/sch_generic.c   | 50 +++++++++++++++++++++++++++++++++++++++++++++++
 6 files changed, 88 insertions(+), 37 deletions(-)

-- 
2.13.0

[Patch net-next v2 1/3] net: introduce helper dev_change_tx_queue_len()

[Cong Wang]

This patch promotes the local change_tx_queue_len() to a core
helper function, dev_change_tx_queue_len(), so that rtnetlink
and net-sysfs could share the code. This also prepares for the
following patch.

Note, the -EFAULT in the original code doesn't make sense,
we should propagate the errno from notifiers.

Cc: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/linux/netdevice.h |  1 +
 net/core/dev.c            | 28 ++++++++++++++++++++++++++++
 net/core/net-sysfs.c      | 25 +------------------------
 net/core/rtnetlink.c      | 18 +++++-------------
 4 files changed, 35 insertions(+), 37 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 581495f4e487..97fab46bdea2 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3327,6 +3327,7 @@ int dev_get_alias(const struct net_device *, char *, size_t);
 int dev_change_net_namespace(struct net_device *, struct net *, const char *);
 int __dev_set_mtu(struct net_device *, int);
 int dev_set_mtu(struct net_device *, int);
+int dev_change_tx_queue_len(struct net_device *, unsigned long);
 void dev_set_group(struct net_device *, int);
 int dev_set_mac_address(struct net_device *, struct sockaddr *);
 int dev_change_carrier(struct net_device *, bool new_carrier);
diff --git a/net/core/dev.c b/net/core/dev.c
index 77795f66c246..913655e82859 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7037,6 +7037,34 @@ int dev_set_mtu(struct net_device *dev, int new_mtu)
 EXPORT_SYMBOL(dev_set_mtu);
 
 /**
+ *	dev_change_tx_queue_len - Change TX queue length of a netdevice
+ *	@dev: device
+ *	@new_len: new tx queue length
+ */
+int dev_change_tx_queue_len(struct net_device *dev, unsigned long new_len)
+{
+	unsigned int orig_len = dev->tx_queue_len;
+	int res;
+
+	if (new_len != (unsigned int)new_len)
+		return -ERANGE;
+
+	if (new_len != orig_len) {
+		dev->tx_queue_len = new_len;
+		res = call_netdevice_notifiers(NETDEV_CHANGE_TX_QUEUE_LEN, dev);
+		res = notifier_to_errno(res);
+		if (res) {
+			netdev_err(dev,
+				   "refused to change device tx_queue_len\n");
+			dev->tx_queue_len = orig_len;
+			return res;
+		}
+	}
+
+	return 0;
+}
+
+/**
  *	dev_set_group - Change group this device belongs to
  *	@dev: device
  *	@new_group: group this device should belong to
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index c4a28f4667b6..60a5ad2c33ee 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -346,29 +346,6 @@ static ssize_t flags_store(struct device *dev, struct device_attribute *attr,
 }
 NETDEVICE_SHOW_RW(flags, fmt_hex);
 
-static int change_tx_queue_len(struct net_device *dev, unsigned long new_len)
-{
-	unsigned int orig_len = dev->tx_queue_len;
-	int res;
-
-	if (new_len != (unsigned int)new_len)
-		return -ERANGE;
-
-	if (new_len != orig_len) {
-		dev->tx_queue_len = new_len;
-		res = call_netdevice_notifiers(NETDEV_CHANGE_TX_QUEUE_LEN, dev);
-		res = notifier_to_errno(res);
-		if (res) {
-			netdev_err(dev,
-				   "refused to change device tx_queue_len\n");
-			dev->tx_queue_len = orig_len;
-			return -EFAULT;
-		}
-	}
-
-	return 0;
-}
-
 static ssize_t tx_queue_len_store(struct device *dev,
 				  struct device_attribute *attr,
 				  const char *buf, size_t len)
@@ -376,7 +353,7 @@ static ssize_t tx_queue_len_store(struct device *dev,
 	if (!capable(CAP_NET_ADMIN))
 		return -EPERM;
 
-	return netdev_store(dev, attr, buf, len, change_tx_queue_len);
+	return netdev_store(dev, attr, buf, len, dev_change_tx_queue_len);
 }
 NETDEVICE_SHOW_RW(tx_queue_len, fmt_dec);
 
diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
index 97874daa1336..6fa6b9c60694 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2291,19 +2291,11 @@ static int do_setlink(const struct sk_buff *skb,
 
 	if (tb[IFLA_TXQLEN]) {
 		unsigned int value = nla_get_u32(tb[IFLA_TXQLEN]);
-		unsigned int orig_len = dev->tx_queue_len;
-
-		if (dev->tx_queue_len ^ value) {
-			dev->tx_queue_len = value;
-			err = call_netdevice_notifiers(
-			      NETDEV_CHANGE_TX_QUEUE_LEN, dev);
-			err = notifier_to_errno(err);
-			if (err) {
-				dev->tx_queue_len = orig_len;
-				goto errout;
-			}
-			status |= DO_SETLINK_MODIFIED;
-		}
+
+		err = dev_change_tx_queue_len(dev, value);
+		if (err)
+			goto errout;
+		status |= DO_SETLINK_MODIFIED;
 	}
 
 	if (tb[IFLA_GSO_MAX_SIZE]) {
-- 
2.13.0

[Patch net-next v2 2/3] net_sched: plug in qdisc ops change_tx_queue_len

[Cong Wang]

Introduce a new qdisc ops ->change_tx_queue_len() so that
each qdisc could decide how to implement this if it wants.
Previously we simply read dev->tx_queue_len, after pfifo_fast
switches to skb array, we need this API to resize the skb array
when we change dev->tx_queue_len.

To avoid handling race conditions with TX BH, we need to
deactivate all TX queues before change the value and bring them
back after we are done, this also makes implementation easier.

Cc: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 include/net/sch_generic.h |  2 ++
 net/core/dev.c            |  1 +
 net/sched/sch_generic.c   | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 36 insertions(+)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index cd1be1f25c36..d13dd129d085 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h
@@ -200,6 +200,7 @@ struct Qdisc_ops {
 					  struct nlattr *arg,
 					  struct netlink_ext_ack *extack);
 	void			(*attach)(struct Qdisc *sch);
+	int			(*change_tx_queue_len)(struct Qdisc *, unsigned int);
 
 	int			(*dump)(struct Qdisc *, struct sk_buff *);
 	int			(*dump_stats)(struct Qdisc *, struct gnet_dump *);
@@ -488,6 +489,7 @@ void qdisc_class_hash_remove(struct Qdisc_class_hash *,
 void qdisc_class_hash_grow(struct Qdisc *, struct Qdisc_class_hash *);
 void qdisc_class_hash_destroy(struct Qdisc_class_hash *);
 
+int dev_qdisc_change_tx_queue_len(struct net_device *dev);
 void dev_init_scheduler(struct net_device *dev);
 void dev_shutdown(struct net_device *dev);
 void dev_activate(struct net_device *dev);
diff --git a/net/core/dev.c b/net/core/dev.c
index 913655e82859..a9d7d883416d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7059,6 +7059,7 @@ int dev_change_tx_queue_len(struct net_device *dev, unsigned long new_len)
 			dev->tx_queue_len = orig_len;
 			return res;
 		}
+		return dev_qdisc_change_tx_queue_len(dev);
 	}
 
 	return 0;
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 1816bde47256..08f9fa27e06e 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -1178,6 +1178,39 @@ void dev_deactivate(struct net_device *dev)
 }
 EXPORT_SYMBOL(dev_deactivate);
 
+static int qdisc_change_tx_queue_len(struct net_device *dev,
+				     struct netdev_queue *dev_queue)
+{
+	struct Qdisc *qdisc = dev_queue->qdisc_sleeping;
+	const struct Qdisc_ops *ops = qdisc->ops;
+
+	if (ops->change_tx_queue_len)
+		return ops->change_tx_queue_len(qdisc, dev->tx_queue_len);
+	return 0;
+}
+
+int dev_qdisc_change_tx_queue_len(struct net_device *dev)
+{
+	bool up = dev->flags & IFF_UP;
+	unsigned int i;
+	int ret = 0;
+
+	if (up)
+		dev_deactivate(dev);
+
+	for (i = 0; i < dev->num_tx_queues; i++) {
+		ret = qdisc_change_tx_queue_len(dev, &dev->_tx[i]);
+
+		/* TODO: revert changes on a partial failure */
+		if (ret)
+			break;
+	}
+
+	if (up)
+		dev_activate(dev);
+	return ret;
+}
+
 static void dev_init_scheduler_queue(struct net_device *dev,
 				     struct netdev_queue *dev_queue,
 				     void *_qdisc)
-- 
2.13.0

[Patch net-next v2 3/3] net_sched: implement ->change_tx_queue_len() for pfifo_fast

[Cong Wang]

pfifo_fast used to drop based on qdisc_dev(qdisc)->tx_queue_len,
so we have to resize skb array when we change tx_queue_len.

Other qdiscs which read tx_queue_len are fine because they
all save it to sch->limit or somewhere else in qdisc during init.
They don't have to implement this, it is nicer if they do so
that users don't have to re-configure qdisc after changing
tx_queue_len.

Cc: John Fastabend <john.fastabend@gmail.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
 net/sched/sch_generic.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 08f9fa27e06e..d7b82c379617 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -763,6 +763,22 @@ static void pfifo_fast_destroy(struct Qdisc *sch)
 	}
 }
 
+static int pfifo_fast_change_tx_queue_len(struct Qdisc *sch, unsigned int new_len)
+{
+	struct pfifo_fast_priv *priv = qdisc_priv(sch);
+	int ret = 0, prio;
+
+	for (prio = 0; prio < PFIFO_FAST_BANDS; prio++) {
+		struct skb_array *q = band2list(priv, prio);
+
+		ret = skb_array_resize(q, new_len, GFP_KERNEL);
+		if (ret)
+			break;
+	}
+
+	return ret;
+}
+
 struct Qdisc_ops pfifo_fast_ops __read_mostly = {
 	.id		=	"pfifo_fast",
 	.priv_size	=	sizeof(struct pfifo_fast_priv),
@@ -773,6 +789,7 @@ struct Qdisc_ops pfifo_fast_ops __read_mostly = {
 	.destroy	=	pfifo_fast_destroy,
 	.reset		=	pfifo_fast_reset,
 	.dump		=	pfifo_fast_dump,
+	.change_tx_queue_len =  pfifo_fast_change_tx_queue_len,
 	.owner		=	THIS_MODULE,
 	.static_flags	=	TCQ_F_NOLOCK | TCQ_F_CPUSTATS,
 };
-- 
2.13.0

Re: [Patch net-next v2 0/3] net_sched: reflect tx_queue_len change for pfifo_fast

[David Miller]

From: Cong Wang <xiyou.wangcong@gmail.com>
Date: Tue, 23 Jan 2018 10:18:56 -0800

> This pathcset restores the pfifo_fast qdisc behavior of dropping
> packets based on latest dev->tx_queue_len. Patch 1 introduces
> a helper, patch 2 introduces a new Qdisc ops which is called when
> we modify tx_queue_len, patch 3 implements this ops for pfifo_fast.
> 
> Please see each patch for details.
> 
> ---
> v2: handle error case for ->change_tx_queue_len()

John, please review.

Thanks.

Re: [Patch net-next v2 2/3] net_sched: plug in qdisc ops change_tx_queue_len

[John Fastabend]

On 01/23/2018 10:18 AM, Cong Wang wrote:
> Introduce a new qdisc ops ->change_tx_queue_len() so that
> each qdisc could decide how to implement this if it wants.
> Previously we simply read dev->tx_queue_len, after pfifo_fast
> switches to skb array, we need this API to resize the skb array
> when we change dev->tx_queue_len.
> 
> To avoid handling race conditions with TX BH, we need to
> deactivate all TX queues before change the value and bring them
> back after we are done, this also makes implementation easier.
> 
> Cc: John Fastabend <john.fastabend@gmail.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
> ---
>  include/net/sch_generic.h |  2 ++
>  net/core/dev.c            |  1 +
>  net/sched/sch_generic.c   | 33 +++++++++++++++++++++++++++++++++
>  3 files changed, 36 insertions(+)
> 
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index cd1be1f25c36..d13dd129d085 100644
> --- a/include/net/sch_generic.h
> +++ b/include/net/sch_generic.h
> @@ -200,6 +200,7 @@ struct Qdisc_ops {
>  					  struct nlattr *arg,
>  					  struct netlink_ext_ack *extack);
>  	void			(*attach)(struct Qdisc *sch);
> +	int			(*change_tx_queue_len)(struct Qdisc *, unsigned int);
>  
>  	int			(*dump)(struct Qdisc *, struct sk_buff *);
>  	int			(*dump_stats)(struct Qdisc *, struct gnet_dump *);
> @@ -488,6 +489,7 @@ void qdisc_class_hash_remove(struct Qdisc_class_hash *,
>  void qdisc_class_hash_grow(struct Qdisc *, struct Qdisc_class_hash *);
>  void qdisc_class_hash_destroy(struct Qdisc_class_hash *);
>  
> +int dev_qdisc_change_tx_queue_len(struct net_device *dev);
>  void dev_init_scheduler(struct net_device *dev);
>  void dev_shutdown(struct net_device *dev);
>  void dev_activate(struct net_device *dev);
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 913655e82859..a9d7d883416d 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -7059,6 +7059,7 @@ int dev_change_tx_queue_len(struct net_device *dev, unsigned long new_len)
>  			dev->tx_queue_len = orig_len;
>  			return res;
>  		}
> +		return dev_qdisc_change_tx_queue_len(dev);
>  	}
>  
>  	return 0;
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 1816bde47256..08f9fa27e06e 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -1178,6 +1178,39 @@ void dev_deactivate(struct net_device *dev)
>  }
>  EXPORT_SYMBOL(dev_deactivate);
>  
> +static int qdisc_change_tx_queue_len(struct net_device *dev,
> +				     struct netdev_queue *dev_queue)
> +{
> +	struct Qdisc *qdisc = dev_queue->qdisc_sleeping;
> +	const struct Qdisc_ops *ops = qdisc->ops;
> +
> +	if (ops->change_tx_queue_len)
> +		return ops->change_tx_queue_len(qdisc, dev->tx_queue_len);
> +	return 0;
> +}
> +
> +int dev_qdisc_change_tx_queue_len(struct net_device *dev)
> +{
> +	bool up = dev->flags & IFF_UP;
> +	unsigned int i;
> +	int ret = 0;
> +
> +	if (up)
> +		dev_deactivate(dev);
> +
> +	for (i = 0; i < dev->num_tx_queues; i++) {
> +		ret = qdisc_change_tx_queue_len(dev, &dev->_tx[i]);
> +
> +		/* TODO: revert changes on a partial failure */
> +		if (ret)
> +			break;

After another look it seems we can solve this without too much pain
by using skb_array_resize_multiple() in patch 3/3. Then pass the
error pack here via qdisc_change_tx_queue_len and reset queue length
to orig_length.

Mind giving it a try? Or else I'll do it Friday probably.

Thanks,
John

Re: [Patch net-next v2 2/3] net_sched: plug in qdisc ops change_tx_queue_len

[Cong Wang]

On Wed, Jan 24, 2018 at 4:05 PM, John Fastabend
<john.fastabend@gmail.com> wrote:
> On 01/23/2018 10:18 AM, Cong Wang wrote:
>> +int dev_qdisc_change_tx_queue_len(struct net_device *dev)
>> +{
>> +     bool up = dev->flags & IFF_UP;
>> +     unsigned int i;
>> +     int ret = 0;
>> +
>> +     if (up)
>> +             dev_deactivate(dev);
>> +
>> +     for (i = 0; i < dev->num_tx_queues; i++) {
>> +             ret = qdisc_change_tx_queue_len(dev, &dev->_tx[i]);
>> +
>> +             /* TODO: revert changes on a partial failure */
>> +             if (ret)
>> +                     break;
>
> After another look it seems we can solve this without too much pain
> by using skb_array_resize_multiple() in patch 3/3. Then pass the
> error pack here via qdisc_change_tx_queue_len and reset queue length
> to orig_length.

Yeah, I was not aware of that API, looks reasonable. But, since you
are referring to dev_qdisc_change_tx_queue_len(), it doesn't help 2/3
because we still have to iterate each tx queue, so the above comment
still stands.

I will update patch 3/3.

Thanks.