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

[Patch net-next 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.

---

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   | 35 +++++++++++++++++++++++++++++++++++
 6 files changed, 73 insertions(+), 37 deletions(-)

-- 
2.13.0

[Patch net-next 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 ed0799a12bf2..75587a053c33 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -3325,6 +3325,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 94435cd09072..99d353e4cbb2 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7036,6 +7036,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 7bf8b85ade16..584a480c6274 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -325,29 +325,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)
@@ -355,7 +332,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 16d644a4f974..fb2f38193b86 100644
--- a/net/core/rtnetlink.c
+++ b/net/core/rtnetlink.c
@@ -2282,19 +2282,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

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

[John Fastabend]

On 01/19/2018 03:09 PM, Cong Wang wrote:
> 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>
> ---


[...]

>  static ssize_t tx_queue_len_store(struct device *dev,
>  				  struct device_attribute *attr,
>  				  const char *buf, size_t len)
> @@ -355,7 +332,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);
>  }

Is this protected by RTNL lock? If not what happens if this and do_setlink
both try to change tx queue length at the same time? Seems we could get
a race with multiple dev_deactivate/dev_activate sequences in-flight in
the following 2/3 patch.

Thanks,
John

>  NETDEVICE_SHOW_RW(tx_queue_len, fmt_dec);
>  

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

[Cong Wang]

On Sat, Jan 20, 2018 at 8:52 PM, John Fastabend
<john.fastabend@gmail.com> wrote:
> On 01/19/2018 03:09 PM, Cong Wang wrote:
>> 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>
>> ---
>
>
> [...]
>
>>  static ssize_t tx_queue_len_store(struct device *dev,
>>                                 struct device_attribute *attr,
>>                                 const char *buf, size_t len)
>> @@ -355,7 +332,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);
>>  }
>
> Is this protected by RTNL lock? If not what happens if this and do_setlink
> both try to change tx queue length at the same time? Seems we could get
> a race with multiple dev_deactivate/dev_activate sequences in-flight in
> the following 2/3 patch.

Surely it is protected by RTNL...

  96         if (!rtnl_trylock())
  97                 return restart_syscall();
  98
  99         if (dev_isalive(netdev)) {
 100                 ret = (*set)(netdev, new);
 101                 if (ret == 0)
 102                         ret = len;
 103         }
 104         rtnl_unlock();

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

[John Fastabend]

On 01/19/2018 03:09 PM, Cong Wang wrote:
> 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>
> ---


Acked-by: John Fastabend <john.fastabend@gmail.com>

[Patch net-next 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   | 22 ++++++++++++++++++++++
 3 files changed, 25 insertions(+)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index cd1be1f25c36..aae1baa1c30f 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);
+	void			(*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 *);
 
+void 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 99d353e4cbb2..24809d858a64 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -7058,6 +7058,7 @@ int dev_change_tx_queue_len(struct net_device *dev, unsigned long new_len)
 			dev->tx_queue_len = orig_len;
 			return res;
 		}
+		dev_qdisc_change_tx_queue_len(dev);
 	}
 
 	return 0;
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index ef8b4ecde2ac..30aaeb3c1bf1 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -1178,6 +1178,28 @@ void dev_deactivate(struct net_device *dev)
 }
 EXPORT_SYMBOL(dev_deactivate);
 
+static void qdisc_change_tx_queue_len(struct net_device *dev,
+				      struct netdev_queue *dev_queue,
+				      void *unused)
+{
+	struct Qdisc *qdisc = dev_queue->qdisc_sleeping;
+	const struct Qdisc_ops *ops = qdisc->ops;
+
+	if (ops->change_tx_queue_len)
+		ops->change_tx_queue_len(qdisc, dev->tx_queue_len);
+}
+
+void dev_qdisc_change_tx_queue_len(struct net_device *dev)
+{
+	bool up = dev->flags & IFF_UP;
+
+	if (up)
+		dev_deactivate(dev);
+	netdev_for_each_tx_queue(dev, qdisc_change_tx_queue_len, NULL);
+	if (up)
+		dev_activate(dev);
+}
+
 static void dev_init_scheduler_queue(struct net_device *dev,
 				     struct netdev_queue *dev_queue,
 				     void *_qdisc)
-- 
2.13.0

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

[John Fastabend]

On 01/19/2018 03:09 PM, 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   | 22 ++++++++++++++++++++++
>  3 files changed, 25 insertions(+)
> 
> diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
> index cd1be1f25c36..aae1baa1c30f 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);
> +	void			(*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 *);
>  
> +void 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 99d353e4cbb2..24809d858a64 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -7058,6 +7058,7 @@ int dev_change_tx_queue_len(struct net_device *dev, unsigned long new_len)
>  			dev->tx_queue_len = orig_len;
>  			return res;
>  		}
> +		dev_qdisc_change_tx_queue_len(dev);
>  	}
>  
>  	return 0;
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index ef8b4ecde2ac..30aaeb3c1bf1 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -1178,6 +1178,28 @@ void dev_deactivate(struct net_device *dev)
>  }
>  EXPORT_SYMBOL(dev_deactivate);
>  
> +static void qdisc_change_tx_queue_len(struct net_device *dev,
> +				      struct netdev_queue *dev_queue,
> +				      void *unused)
> +{
> +	struct Qdisc *qdisc = dev_queue->qdisc_sleeping;
> +	const struct Qdisc_ops *ops = qdisc->ops;
> +
> +	if (ops->change_tx_queue_len)
> +		ops->change_tx_queue_len(qdisc, dev->tx_queue_len);

hmm what happens if the resize fails in the next patch,

>  
> +static void pfifo_fast_change_tx_queue_len(struct Qdisc *sch, unsigned int new_len)
> +{
> +	struct pfifo_fast_priv *priv = qdisc_priv(sch);
> +	int prio;
> +
> +	for (prio = 0; prio < PFIFO_FAST_BANDS; prio++) {
> +		struct skb_array *q = band2list(priv, prio);
> +
> +		skb_array_resize(q, new_len, GFP_KERNEL);
> +	}
> +}
> +

Here skb_array_resize() can fail with ENOMEM, do we need to unwind the
change and push the error up the stack?

Thanks,
John

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

[Cong Wang]

On Sun, Jan 21, 2018 at 2:12 PM, John Fastabend
<john.fastabend@gmail.com> wrote:
> On 01/19/2018 03:09 PM, Cong Wang wrote:
>
> hmm what happens if the resize fails in the next patch,
>
>>
>> +static void pfifo_fast_change_tx_queue_len(struct Qdisc *sch, unsigned int new_len)
>> +{
>> +     struct pfifo_fast_priv *priv = qdisc_priv(sch);
>> +     int prio;
>> +
>> +     for (prio = 0; prio < PFIFO_FAST_BANDS; prio++) {
>> +             struct skb_array *q = band2list(priv, prio);
>> +
>> +             skb_array_resize(q, new_len, GFP_KERNEL);
>> +     }
>> +}
>> +
>
> Here skb_array_resize() can fail with ENOMEM, do we need to unwind the
> change and push the error up the stack?

Good point, but netdev_for_each_tx_queue() doesn't handle errors
in current code base, it is not trivial to make it returning int to reflect
errors, since we have to rollback a partial failure too.

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

[Cong Wang]

On Mon, Jan 22, 2018 at 2:44 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>
> Good point, but netdev_for_each_tx_queue() doesn't handle errors
> in current code base, it is not trivial to make it returning int to reflect
> errors, since we have to rollback a partial failure too.

I think for now we can just check for errors but not rollback,
at least attach_default_qdiscs() does not rollback either.
And I will add a comment saying we need to do it in future.

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

[John Fastabend]

On 01/23/2018 10:07 AM, Cong Wang wrote:
> On Mon, Jan 22, 2018 at 2:44 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>
>> Good point, but netdev_for_each_tx_queue() doesn't handle errors
>> in current code base, it is not trivial to make it returning int to reflect
>> errors, since we have to rollback a partial failure too.
> 
> I think for now we can just check for errors but not rollback,
> at least attach_default_qdiscs() does not rollback either.
> And I will add a comment saying we need to do it in future.
> 

Sorry I delayed responding was thinking about some solutions. Unrolling
is problematic because we may fail again on the unroll :( so I guess
for this to work we would need a two phase operation with a prep and
commit phase.

So agree for now checking for errors seems reasonable and is at least
better than without the patches. And as you say another patch set can
cleanup the bigger issue of rollback.

Thanks,
.John

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

[John Fastabend]

On 01/23/2018 10:07 AM, Cong Wang wrote:
> On Mon, Jan 22, 2018 at 2:44 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>
>> Good point, but netdev_for_each_tx_queue() doesn't handle errors
>> in current code base, it is not trivial to make it returning int to reflect
>> errors, since we have to rollback a partial failure too.
> 
> I think for now we can just check for errors but not rollback,
> at least attach_default_qdiscs() does not rollback either.
> And I will add a comment saying we need to do it in future.
> 

Sorry I delayed responding was thinking about some solutions. Unrolling
is problematic because we may fail again on the unroll :( so I guess
for this to work we would need a two phase operation with a prep and
commit phase.

So agree for now checking for errors seems reasonable and is at least
better than without the patches. And as you say another patch set can
cleanup the bigger issue of rollback.

Thanks,
.John

[Patch net-next 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 | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 30aaeb3c1bf1..7c48a6942238 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -763,6 +763,18 @@ static void pfifo_fast_destroy(struct Qdisc *sch)
 	}
 }
 
+static void pfifo_fast_change_tx_queue_len(struct Qdisc *sch, unsigned int new_len)
+{
+	struct pfifo_fast_priv *priv = qdisc_priv(sch);
+	int prio;
+
+	for (prio = 0; prio < PFIFO_FAST_BANDS; prio++) {
+		struct skb_array *q = band2list(priv, prio);
+
+		skb_array_resize(q, new_len, GFP_KERNEL);
+	}
+}
+
 struct Qdisc_ops pfifo_fast_ops __read_mostly = {
 	.id		=	"pfifo_fast",
 	.priv_size	=	sizeof(struct pfifo_fast_priv),
@@ -773,6 +785,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