[PATCH net-next 0/3] net: sched: update default qdisc visibility after Tx queue cnt changes

Matthew noticed that number of children reported by mq does not match
number of queues on reconfigured interfaces. For example if mq is
instantiated when there is 8 queues it will always show 8 children,
regardless of config being changed:

 # ethtool -L eth0 combined 8
 # tc qdisc replace dev eth0 root handle 100: mq
 # tc qdisc show dev eth0
 qdisc mq 100: root 
 qdisc pfifo_fast 0: parent 100:8 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:7 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:6 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:5 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:4 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:3 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:2 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:1 bands 3 priomap 1 2 ...
 # ethtool -L eth0 combined 1
 # tc qdisc show dev eth0
 qdisc mq 100: root 
 qdisc pfifo_fast 0: parent 100:8 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:7 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:6 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:5 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:4 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:3 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:2 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:1 bands 3 priomap 1 2 ...
 # ethtool -L eth0 combined 32
 # tc qdisc show dev eth0
 qdisc mq 100: root 
 qdisc pfifo_fast 0: parent 100:8 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:7 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:6 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:5 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:4 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:3 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:2 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:1 bands 3 priomap 1 2 ...

This patchset fixes this by hashing and unhasing the default
child qdiscs as number of queues gets adjusted.

Jakub Kicinski (3):
  net: sched: update default qdisc visibility after Tx queue cnt changes
  netdevsim: add ability to change channel count
  selftests: net: test ethtool -L vs mq

 drivers/net/netdevsim/ethtool.c               | 28 +++++++
 drivers/net/netdevsim/netdevsim.h             |  1 +
 include/net/sch_generic.h                     |  4 +
 net/core/dev.c                                |  2 +
 net/sched/sch_generic.c                       |  9 +++
 net/sched/sch_mq.c                            | 24 ++++++
 net/sched/sch_mqprio.c                        | 23 ++++++
 .../drivers/net/netdevsim/ethtool-common.sh   |  2 +-
 .../drivers/net/netdevsim/tc-mq-visibility.sh | 77 +++++++++++++++++++
 9 files changed, 169 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/drivers/net/netdevsim/tc-mq-visibility.sh

-- 
2.31.1

mq / mqprio make the default child qdiscs visible. They only do
so for the qdiscs which are within real_num_tx_queues when the
device is registered. Depending on order of calls in the driver,
or if user space changes config via ethtool -L the number of
qdiscs visible under tc qdisc show will differ from the number
of queues. This is confusing to users and potentially to system
configuration scripts which try to make sure qdiscs have the
right parameters.

Add a new Qdisc_ops callback and make relevant qdiscs TTRT.

Note that this uncovers the "shortcut" created by
commit 1f27cde313d7 ("net: sched: use pfifo_fast for non real queues")
The default child qdiscs beyond initial real_num_tx are always
pfifo_fast, no matter what the sysfs setting is. Fixing this
gets a little tricky because we'd need to keep a reference
on whatever the default qdisc was at the time of creation.
In practice this is likely an non-issue the qdiscs likely have
to be configured to non-default settings, so whatever user space
is doing such configuration can replace the pfifos... now that
it will see them.

Reported-by: Matthew Massey <redacted>
Reviewed-by: Dave Taht <redacted>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 include/net/sch_generic.h |  4 ++++
 net/core/dev.c            |  2 ++
 net/sched/sch_generic.c   |  9 +++++++++
 net/sched/sch_mq.c        | 24 ++++++++++++++++++++++++
 net/sched/sch_mqprio.c    | 23 +++++++++++++++++++++++
 5 files changed, 62 insertions(+)

diff --git a/include/net/sch_generic.h b/include/net/sch_generic.h
index c0069ac00e62..8c2d611639fc 100644
--- a/include/net/sch_generic.h
+++ b/include/net/sch_generic.h

@@ -308,6 +308,8 @@ struct Qdisc_ops {
 					  struct netlink_ext_ack *extack);
 	void			(*attach)(struct Qdisc *sch);
 	int			(*change_tx_queue_len)(struct Qdisc *, unsigned int);
+	void			(*change_real_num_tx)(struct Qdisc *sch,
+						      unsigned int new_real_tx);
 
 	int			(*dump)(struct Qdisc *, struct sk_buff *);
 	int			(*dump_stats)(struct Qdisc *, struct gnet_dump *);

@@ -684,6 +686,8 @@ 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_qdisc_change_real_num_tx(struct net_device *dev,
+				  unsigned int new_real_tx);
 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 74fd402d26dd..f930329f0dc2 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -2921,6 +2921,8 @@ int netif_set_real_num_tx_queues(struct net_device *dev, unsigned int txq)
 		if (dev->num_tc)
 			netif_setup_tc(dev, txq);
 
+		dev_qdisc_change_real_num_tx(dev, txq);
+
 		dev->real_num_tx_queues = txq;
 
 		if (disabling) {

diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index a8dd06c74e31..66d2fbe9ef50 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c

@@ -1330,6 +1330,15 @@ static int qdisc_change_tx_queue_len(struct net_device *dev,
 	return 0;
 }
 
+void dev_qdisc_change_real_num_tx(struct net_device *dev,
+				  unsigned int new_real_tx)
+{
+	struct Qdisc *qdisc = dev->qdisc;
+
+	if (qdisc->ops->change_real_num_tx)
+		qdisc->ops->change_real_num_tx(qdisc, new_real_tx);
+}
+
 int dev_qdisc_change_tx_queue_len(struct net_device *dev)
 {
 	bool up = dev->flags & IFF_UP;

diff --git a/net/sched/sch_mq.c b/net/sched/sch_mq.c
index e79f1afe0cfd..db18d8a860f9 100644
--- a/net/sched/sch_mq.c
+++ b/net/sched/sch_mq.c

@@ -125,6 +125,29 @@ static void mq_attach(struct Qdisc *sch)
 	priv->qdiscs = NULL;
 }
 
+static void mq_change_real_num_tx(struct Qdisc *sch, unsigned int new_real_tx)
+{
+#ifdef CONFIG_NET_SCHED
+	struct net_device *dev = qdisc_dev(sch);
+	struct Qdisc *qdisc;
+	unsigned int i;
+
+	for (i = new_real_tx; i < dev->real_num_tx_queues; i++) {
+		qdisc = netdev_get_tx_queue(dev, i)->qdisc_sleeping;
+		/* Only update the default qdiscs we created,
+		 * qdiscs with handles are always hashed.
+		 */
+		if (qdisc != &noop_qdisc && !qdisc->handle)
+			qdisc_hash_del(qdisc);
+	}
+	for (i = dev->real_num_tx_queues; i < new_real_tx; i++) {
+		qdisc = netdev_get_tx_queue(dev, i)->qdisc_sleeping;
+		if (qdisc != &noop_qdisc && !qdisc->handle)
+			qdisc_hash_add(qdisc, false);
+	}
+#endif
+}
+
 static int mq_dump(struct Qdisc *sch, struct sk_buff *skb)
 {
 	struct net_device *dev = qdisc_dev(sch);

@@ -288,6 +311,7 @@ struct Qdisc_ops mq_qdisc_ops __read_mostly = {
 	.init		= mq_init,
 	.destroy	= mq_destroy,
 	.attach		= mq_attach,
+	.change_real_num_tx = mq_change_real_num_tx,
 	.dump		= mq_dump,
 	.owner		= THIS_MODULE,
 };

diff --git a/net/sched/sch_mqprio.c b/net/sched/sch_mqprio.c
index 8766ab5b8788..7f23a92849d5 100644
--- a/net/sched/sch_mqprio.c
+++ b/net/sched/sch_mqprio.c

@@ -306,6 +306,28 @@ static void mqprio_attach(struct Qdisc *sch)
 	priv->qdiscs = NULL;
 }
 
+static void mqprio_change_real_num_tx(struct Qdisc *sch,
+				      unsigned int new_real_tx)
+{
+	struct net_device *dev = qdisc_dev(sch);
+	struct Qdisc *qdisc;
+	unsigned int i;
+
+	for (i = new_real_tx; i < dev->real_num_tx_queues; i++) {
+		qdisc = netdev_get_tx_queue(dev, i)->qdisc_sleeping;
+		/* Only update the default qdiscs we created,
+		 * qdiscs with handles are always hashed.
+		 */
+		if (qdisc != &noop_qdisc && !qdisc->handle)
+			qdisc_hash_del(qdisc);
+	}
+	for (i = dev->real_num_tx_queues; i < new_real_tx; i++) {
+		qdisc = netdev_get_tx_queue(dev, i)->qdisc_sleeping;
+		if (qdisc != &noop_qdisc && !qdisc->handle)
+			qdisc_hash_add(qdisc, false);
+	}
+}
+
 static struct netdev_queue *mqprio_queue_get(struct Qdisc *sch,
 					     unsigned long cl)
 {

@@ -623,6 +645,7 @@ static struct Qdisc_ops mqprio_qdisc_ops __read_mostly = {
 	.init		= mqprio_init,
 	.destroy	= mqprio_destroy,
 	.attach		= mqprio_attach,
+	.change_real_num_tx = mqprio_change_real_num_tx,
 	.dump		= mqprio_dump,
 	.owner		= THIS_MODULE,
 };

-- 
2.31.1

For testing visibility of mq/mqprio default children.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/netdevsim/ethtool.c   | 28 ++++++++++++++++++++++++++++
 drivers/net/netdevsim/netdevsim.h |  1 +
 2 files changed, 29 insertions(+)

diff --git a/drivers/net/netdevsim/ethtool.c b/drivers/net/netdevsim/ethtool.c
index b03a0513eb7e..0ab6a40be611 100644
--- a/drivers/net/netdevsim/ethtool.c
+++ b/drivers/net/netdevsim/ethtool.c

@@ -81,6 +81,30 @@ static int nsim_set_ringparam(struct net_device *dev,
 	return 0;
 }
 
+static void
+nsim_get_channels(struct net_device *dev, struct ethtool_channels *ch)
+{
+	struct netdevsim *ns = netdev_priv(dev);
+
+	ch->max_combined = ns->nsim_bus_dev->num_queues;
+	ch->combined_count = ns->ethtool.channels;
+}
+
+static int
+nsim_set_channels(struct net_device *dev, struct ethtool_channels *ch)
+{
+	struct netdevsim *ns = netdev_priv(dev);
+	int err;
+
+	err = netif_set_real_num_queues(dev, ch->combined_count,
+					ch->combined_count);
+	if (err)
+		return err;
+
+	ns->ethtool.channels = ch->combined_count;
+	return 0;
+}
+
 static int
 nsim_get_fecparam(struct net_device *dev, struct ethtool_fecparam *fecparam)
 {

@@ -118,6 +142,8 @@ static const struct ethtool_ops nsim_ethtool_ops = {
 	.get_coalesce			= nsim_get_coalesce,
 	.get_ringparam			= nsim_get_ringparam,
 	.set_ringparam			= nsim_set_ringparam,
+	.get_channels			= nsim_get_channels,
+	.set_channels			= nsim_set_channels,
 	.get_fecparam			= nsim_get_fecparam,
 	.set_fecparam			= nsim_set_fecparam,
 };

@@ -141,6 +167,8 @@ void nsim_ethtool_init(struct netdevsim *ns)
 	ns->ethtool.fec.fec = ETHTOOL_FEC_NONE;
 	ns->ethtool.fec.active_fec = ETHTOOL_FEC_NONE;
 
+	ns->ethtool.channels = ns->nsim_bus_dev->num_queues;
+
 	ethtool = debugfs_create_dir("ethtool", ns->nsim_dev_port->ddir);
 
 	debugfs_create_u32("get_err", 0600, ethtool, &ns->ethtool.get_err);

diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index 793c86dc5a9c..d42eec05490f 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h

@@ -62,6 +62,7 @@ struct nsim_ethtool_pauseparam {
 struct nsim_ethtool {
 	u32 get_err;
 	u32 set_err;
+	u32 channels;
 	struct nsim_ethtool_pauseparam pauseparam;
 	struct ethtool_coalesce coalesce;
 	struct ethtool_ringparam ring;

-- 
2.31.1

Add a selftest for checking mq children are visible after ethtool -L.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 .../drivers/net/netdevsim/ethtool-common.sh   |  2 +-
 .../drivers/net/netdevsim/tc-mq-visibility.sh | 77 +++++++++++++++++++
 2 files changed, 78 insertions(+), 1 deletion(-)
 create mode 100755 tools/testing/selftests/drivers/net/netdevsim/tc-mq-visibility.sh

diff --git a/tools/testing/selftests/drivers/net/netdevsim/ethtool-common.sh b/tools/testing/selftests/drivers/net/netdevsim/ethtool-common.sh
index 7ca1f030d209..922744059aaa 100644
--- a/tools/testing/selftests/drivers/net/netdevsim/ethtool-common.sh
+++ b/tools/testing/selftests/drivers/net/netdevsim/ethtool-common.sh

@@ -50,7 +50,7 @@ function make_netdev {
 	modprobe netdevsim
     fi
 
-    echo $NSIM_ID > /sys/bus/netdevsim/new_device
+    echo $NSIM_ID $@ > /sys/bus/netdevsim/new_device
     # get new device name
     ls /sys/bus/netdevsim/devices/netdevsim${NSIM_ID}/net/
 }

diff --git a/tools/testing/selftests/drivers/net/netdevsim/tc-mq-visibility.sh b/tools/testing/selftests/drivers/net/netdevsim/tc-mq-visibility.sh
new file mode 100755
index 000000000000..fd13c8cfb7a8
--- /dev/null
+++ b/tools/testing/selftests/drivers/net/netdevsim/tc-mq-visibility.sh

@@ -0,0 +1,77 @@
+#!/bin/bash
+# SPDX-License-Identifier: GPL-2.0-only
+
+source ethtool-common.sh
+
+set -o pipefail
+
+n_children() {
+    n=$(tc qdisc show dev $NDEV | grep '^qdisc' | wc -l)
+    echo $((n - 1))
+}
+
+tcq() {
+    tc qdisc $1 dev $NDEV ${@:2}
+}
+
+n_child_assert() {
+    n=$(n_children)
+    if [ $n -ne $1 ]; then
+	echo "ERROR ($root): ${@:2}, expected $1 have $n"
+	((num_errors++))
+    else
+	((num_passes++))
+    fi
+}
+
+
+for root in mq mqprio; do
+    NDEV=$(make_netdev 1 4)
+
+    opts=
+    [ $root == "mqprio" ] && opts='hw 0 num_tc 1 map 0 0 0 0  queues 1@0'
+
+    tcq add root handle 100: $root $opts
+    n_child_assert 4 'Init'
+
+    # All defaults
+
+    for n in 3 2 1 2 3 4 1 4; do
+	ethtool -L $NDEV combined $n
+	n_child_assert $n "Change queues to $n while down"
+    done
+
+    ip link set dev $NDEV up
+
+    for n in 3 2 1 2 3 4 1 4; do
+	ethtool -L $NDEV combined $n
+	n_child_assert $n "Change queues to $n while up"
+    done
+
+    # One real one
+    tcq replace parent 100:4 handle 204: pfifo_fast
+    n_child_assert 4 "One real queue"
+
+    ethtool -L $NDEV combined 1
+    n_child_assert 2 "One real queue, one default"
+
+    ethtool -L $NDEV combined 4
+    n_child_assert 4 "One real queue, rest default"
+
+    # Graft some
+    tcq replace parent 100:1 handle 204:
+    n_child_assert 3 "Grafted"
+
+    ethtool -L $NDEV combined 1
+    n_child_assert 1 "Grafted, one"
+
+    cleanup_nsim
+done
+
+if [ $num_errors -eq 0 ]; then
+    echo "PASSED all $((num_passes)) checks"
+    exit 0
+else
+    echo "FAILED $num_errors/$((num_errors+num_passes)) checks"
+    exit 1
+fi

-- 
2.31.1

Hello:

This series was applied to netdev/net-next.git (refs/heads/master):

On Mon, 13 Sep 2021 15:53:29 -0700 you wrote:

Matthew noticed that number of children reported by mq does not match
number of queues on reconfigured interfaces. For example if mq is
instantiated when there is 8 queues it will always show 8 children,
regardless of config being changed:

 # ethtool -L eth0 combined 8
 # tc qdisc replace dev eth0 root handle 100: mq
 # tc qdisc show dev eth0
 qdisc mq 100: root
 qdisc pfifo_fast 0: parent 100:8 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:7 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:6 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:5 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:4 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:3 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:2 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:1 bands 3 priomap 1 2 ...
 # ethtool -L eth0 combined 1
 # tc qdisc show dev eth0
 qdisc mq 100: root
 qdisc pfifo_fast 0: parent 100:8 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:7 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:6 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:5 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:4 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:3 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:2 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:1 bands 3 priomap 1 2 ...
 # ethtool -L eth0 combined 32
 # tc qdisc show dev eth0
 qdisc mq 100: root
 qdisc pfifo_fast 0: parent 100:8 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:7 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:6 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:5 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:4 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:3 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:2 bands 3 priomap 1 2 ...
 qdisc pfifo_fast 0: parent 100:1 bands 3 priomap 1 2 ...

[...]

Here is the summary with links:
  - [net-next,1/3] net: sched: update default qdisc visibility after Tx queue cnt changes
    https://git.kernel.org/netdev/net-next/c/1e080f17750d
  - [net-next,2/3] netdevsim: add ability to change channel count
    https://git.kernel.org/netdev/net-next/c/2e367522ce6b
  - [net-next,3/3] selftests: net: test ethtool -L vs mq
    https://git.kernel.org/netdev/net-next/c/2d6a58996ee2

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html

On Mon, Sep 13, 2021 at 3:53 PM Jakub Kicinski [off-list ref] wrote:

mq / mqprio make the default child qdiscs visible. They only do
so for the qdiscs which are within real_num_tx_queues when the
device is registered. Depending on order of calls in the driver,
or if user space changes config via ethtool -L the number of
qdiscs visible under tc qdisc show will differ from the number
of queues. This is confusing to users and potentially to system
configuration scripts which try to make sure qdiscs have the
right parameters.

Add a new Qdisc_ops callback and make relevant qdiscs TTRT.

Note that this uncovers the "shortcut" created by
commit 1f27cde313d7 ("net: sched: use pfifo_fast for non real queues")
The default child qdiscs beyond initial real_num_tx are always
pfifo_fast, no matter what the sysfs setting is. Fixing this
gets a little tricky because we'd need to keep a reference
on whatever the default qdisc was at the time of creation.
In practice this is likely an non-issue the qdiscs likely have
to be configured to non-default settings, so whatever user space
is doing such configuration can replace the pfifos... now that
it will see them.

Looks reasonable.

Don't we need to flip the device with dev_deactivate()+dev_activate()?
It looks like the only thing this function resets is qdisc itself, and only
partially.

This is nearly identical to mqprio_change_real_num_tx(), can we reuse
it?

Thanks.

On Wed, 15 Sep 2021 09:31:08 -0700 Cong Wang wrote:

On Mon, Sep 13, 2021 at 3:53 PM Jakub Kicinski [off-list ref] wrote:

quoted

diff --git a/net/core/dev.c b/net/core/dev.c
index 74fd402d26dd..f930329f0dc2 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -2921,6 +2921,8 @@ int netif_set_real_num_tx_queues(struct net_device *dev, unsigned int txq)
                if (dev->num_tc)
                        netif_setup_tc(dev, txq);

+               dev_qdisc_change_real_num_tx(dev, txq);
+  

Don't we need to flip the device with dev_deactivate()+dev_activate()?
It looks like the only thing this function resets is qdisc itself, and only
partially.

We're only making the qdiscs visible, there should be 
no datapath-visible change.

quoted

                dev->real_num_tx_queues = txq;

                if (disabling) {

quoted

diff --git a/net/sched/sch_mq.c b/net/sched/sch_mq.c
index e79f1afe0cfd..db18d8a860f9 100644
--- a/net/sched/sch_mq.c
+++ b/net/sched/sch_mq.c

@@ -125,6 +125,29 @@ static void mq_attach(struct Qdisc *sch)
        priv->qdiscs = NULL;
 }

+static void mq_change_real_num_tx(struct Qdisc *sch, unsigned int new_real_tx)  

This is nearly identical to mqprio_change_real_num_tx(), can we reuse
it?

Indeed, I was a little unsure where best to place the helper.
Since mq is always built if mqprio is my instinct would be to
export mq_change_real_num_tx and use it in mqprio. But I didn't 
see any existing exports (mq_attach(), mq_queue_get() are also
identical and are not shared) so I just copy&pasted the logic.

LMK if (a) that's fine; (b) I should share the new code; 
(c) I should post a patch to share all the code that's identical;...

On Wed, Sep 15, 2021 at 12:36 PM Jakub Kicinski [off-list ref] wrote:

On Wed, 15 Sep 2021 09:31:08 -0700 Cong Wang wrote:

quoted

On Mon, Sep 13, 2021 at 3:53 PM Jakub Kicinski [off-list ref] wrote:

quoted

diff --git a/net/core/dev.c b/net/core/dev.c
index 74fd402d26dd..f930329f0dc2 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c

@@ -2921,6 +2921,8 @@ int netif_set_real_num_tx_queues(struct net_device *dev, unsigned int txq)
                if (dev->num_tc)
                        netif_setup_tc(dev, txq);

+               dev_qdisc_change_real_num_tx(dev, txq);
+

Don't we need to flip the device with dev_deactivate()+dev_activate()?
It looks like the only thing this function resets is qdisc itself, and only
partially.

We're only making the qdiscs visible, there should be
no datapath-visible change.

Isn't every qdisc under mq visible to datapath?

Packets can be pending in qdisc's, and qdisc's can be scheduled
in TX softirq, so essentially we need to flip the device like other
places.

quoted

quoted

                dev->real_num_tx_queues = txq;

                if (disabling) {

quoted

quoted

diff --git a/net/sched/sch_mq.c b/net/sched/sch_mq.c
index e79f1afe0cfd..db18d8a860f9 100644
--- a/net/sched/sch_mq.c
+++ b/net/sched/sch_mq.c

@@ -125,6 +125,29 @@ static void mq_attach(struct Qdisc *sch)
        priv->qdiscs = NULL;
 }

+static void mq_change_real_num_tx(struct Qdisc *sch, unsigned int new_real_tx)

This is nearly identical to mqprio_change_real_num_tx(), can we reuse
it?

Indeed, I was a little unsure where best to place the helper.
Since mq is always built if mqprio is my instinct would be to
export mq_change_real_num_tx and use it in mqprio. But I didn't
see any existing exports (mq_attach(), mq_queue_get() are also
identical and are not shared) so I just copy&pasted the logic.

What about net/sched/sch_generic.c?

LMK if (a) that's fine; (b) I should share the new code;
(c) I should post a patch to share all the code that's identical;...

I think you can put the code in net/sched/sch_generic.c and export
it for mqprio (mq is built-in so can just call it).

Thanks.

On Thu, 16 Sep 2021 22:46:42 -0700 Cong Wang wrote:

On Wed, Sep 15, 2021 at 12:36 PM Jakub Kicinski [off-list ref] wrote:

quoted

On Wed, 15 Sep 2021 09:31:08 -0700 Cong Wang wrote:  

quoted

Don't we need to flip the device with dev_deactivate()+dev_activate()?
It looks like the only thing this function resets is qdisc itself, and only
partially.  

We're only making the qdiscs visible, there should be
no datapath-visible change.  

Isn't every qdisc under mq visible to datapath?

Packets can be pending in qdisc's, and qdisc's can be scheduled
in TX softirq, so essentially we need to flip the device like other

By visible I mean tc qdisc dump shows them. I'm adding/removing 
the qdiscs to netdev->qdisc_hash. That's only used by control 
paths to dump or find qdiscs by handle.

quoted

quoted

This is nearly identical to mqprio_change_real_num_tx(), can we reuse
it?  

Indeed, I was a little unsure where best to place the helper.
Since mq is always built if mqprio is my instinct would be to
export mq_change_real_num_tx and use it in mqprio. But I didn't
see any existing exports (mq_attach(), mq_queue_get() are also
identical and are not shared) so I just copy&pasted the logic.  

What about net/sched/sch_generic.c?

quoted

LMK if (a) that's fine; (b) I should share the new code;
(c) I should post a patch to share all the code that's identical;...  

I think you can put the code in net/sched/sch_generic.c and export
it for mqprio (mq is built-in so can just call it).

Will do, thanks!

On Fri, Sep 17, 2021 at 6:09 AM Jakub Kicinski [off-list ref] wrote:

On Thu, 16 Sep 2021 22:46:42 -0700 Cong Wang wrote:

quoted

On Wed, Sep 15, 2021 at 12:36 PM Jakub Kicinski [off-list ref] wrote:

quoted

On Wed, 15 Sep 2021 09:31:08 -0700 Cong Wang wrote:

quoted

Don't we need to flip the device with dev_deactivate()+dev_activate()?
It looks like the only thing this function resets is qdisc itself, and only
partially.

We're only making the qdiscs visible, there should be
no datapath-visible change.

Isn't every qdisc under mq visible to datapath?

Packets can be pending in qdisc's, and qdisc's can be scheduled
in TX softirq, so essentially we need to flip the device like other

By visible I mean tc qdisc dump shows them. I'm adding/removing
the qdiscs to netdev->qdisc_hash. That's only used by control
paths to dump or find qdiscs by handle.

Oh, I see, I missed this difference. If datapath can't see this change
immediately, we don't need to flip it.

Thanks.