[PATCH net-next] Revert "netdevsim: Add multi-queue support"

This reverts commit d4861fc6be581561d6964700110a4dede54da6a6.

netdevsim is for enabling upstream tests, two weeks in
and there's no sign of upstream test using the "mutli-queue"
option.

We can add this option back when such test materializes.
Right now it's dead code.

Signed-off-by: Jakub Kicinski <kuba@kernel.org>
---
 drivers/net/netdevsim/bus.c       | 17 +++++++----------
 drivers/net/netdevsim/netdev.c    |  6 ++----
 drivers/net/netdevsim/netdevsim.h |  1 -
 3 files changed, 9 insertions(+), 15 deletions(-)

diff --git a/drivers/net/netdevsim/bus.c b/drivers/net/netdevsim/bus.c
index ff01e5bdc72e..ccec29970d5b 100644
--- a/drivers/net/netdevsim/bus.c
+++ b/drivers/net/netdevsim/bus.c

@@ -262,31 +262,29 @@ static struct device_type nsim_bus_dev_type = {
 };
 
 static struct nsim_bus_dev *
-nsim_bus_dev_new(unsigned int id, unsigned int port_count, unsigned int num_queues);
+nsim_bus_dev_new(unsigned int id, unsigned int port_count);
 
 static ssize_t
 new_device_store(struct bus_type *bus, const char *buf, size_t count)
 {
-	unsigned int id, port_count, num_queues;
 	struct nsim_bus_dev *nsim_bus_dev;
+	unsigned int port_count;
+	unsigned int id;
 	int err;
 
-	err = sscanf(buf, "%u %u %u", &id, &port_count, &num_queues);
+	err = sscanf(buf, "%u %u", &id, &port_count);
 	switch (err) {
 	case 1:
 		port_count = 1;
 		fallthrough;
 	case 2:
-		num_queues = 1;
-		fallthrough;
-	case 3:
 		if (id > INT_MAX) {
 			pr_err("Value of \"id\" is too big.\n");
 			return -EINVAL;
 		}
 		break;
 	default:
-		pr_err("Format for adding new device is \"id port_count num_queues\" (uint uint unit).\n");
+		pr_err("Format for adding new device is \"id port_count\" (uint uint).\n");
 		return -EINVAL;
 	}
 

@@ -297,7 +295,7 @@ new_device_store(struct bus_type *bus, const char *buf, size_t count)
 		goto err;
 	}
 
-	nsim_bus_dev = nsim_bus_dev_new(id, port_count, num_queues);
+	nsim_bus_dev = nsim_bus_dev_new(id, port_count);
 	if (IS_ERR(nsim_bus_dev)) {
 		err = PTR_ERR(nsim_bus_dev);
 		goto err;

@@ -399,7 +397,7 @@ static struct bus_type nsim_bus = {
 #define NSIM_BUS_DEV_MAX_VFS 4
 
 static struct nsim_bus_dev *
-nsim_bus_dev_new(unsigned int id, unsigned int port_count, unsigned int num_queues)
+nsim_bus_dev_new(unsigned int id, unsigned int port_count)
 {
 	struct nsim_bus_dev *nsim_bus_dev;
 	int err;

@@ -415,7 +413,6 @@ nsim_bus_dev_new(unsigned int id, unsigned int port_count, unsigned int num_queu
 	nsim_bus_dev->dev.bus = &nsim_bus;
 	nsim_bus_dev->dev.type = &nsim_bus_dev_type;
 	nsim_bus_dev->port_count = port_count;
-	nsim_bus_dev->num_queues = num_queues;
 	nsim_bus_dev->initial_net = current->nsproxy->net_ns;
 	nsim_bus_dev->max_vfs = NSIM_BUS_DEV_MAX_VFS;
 	mutex_init(&nsim_bus_dev->nsim_bus_reload_lock);

diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c
index 50572e0f1f52..c3aeb15843e2 100644
--- a/drivers/net/netdevsim/netdev.c
+++ b/drivers/net/netdevsim/netdev.c

@@ -347,8 +347,7 @@ nsim_create(struct nsim_dev *nsim_dev, struct nsim_dev_port *nsim_dev_port)
 	struct netdevsim *ns;
 	int err;
 
-	dev = alloc_netdev_mq(sizeof(*ns), "eth%d", NET_NAME_UNKNOWN, nsim_setup,
-			      nsim_dev->nsim_bus_dev->num_queues);
+	dev = alloc_netdev(sizeof(*ns), "eth%d", NET_NAME_UNKNOWN, nsim_setup);
 	if (!dev)
 		return ERR_PTR(-ENOMEM);
 

@@ -393,8 +392,7 @@ void nsim_destroy(struct netdevsim *ns)
 static int nsim_validate(struct nlattr *tb[], struct nlattr *data[],
 			 struct netlink_ext_ack *extack)
 {
-	NL_SET_ERR_MSG_MOD(extack,
-			   "Please use: echo \"[ID] [PORT_COUNT] [NUM_QUEUES]\" > /sys/bus/netdevsim/new_device");
+	NL_SET_ERR_MSG_MOD(extack, "Please use: echo \"[ID] [PORT_COUNT]\" > /sys/bus/netdevsim/new_device");
 	return -EOPNOTSUPP;
 }
 

diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h
index 1c20bcbd9d91..ae462957dcee 100644
--- a/drivers/net/netdevsim/netdevsim.h
+++ b/drivers/net/netdevsim/netdevsim.h

@@ -352,7 +352,6 @@ struct nsim_bus_dev {
 	struct device dev;
 	struct list_head list;
 	unsigned int port_count;
-	unsigned int num_queues; /* Number of queues for each port on this bus */
 	struct net *initial_net; /* Purpose of this is to carry net pointer
 				  * during the probe time only.
 				  */

-- 
2.31.1

On Tue, Aug 3, 2021 at 5:39 AM Jakub Kicinski [off-list ref] wrote:

This reverts commit d4861fc6be581561d6964700110a4dede54da6a6.

netdevsim is for enabling upstream tests, two weeks in
and there's no sign of upstream test using the "mutli-queue"
option.

Since when netdevsim is *only* for upstream tests? Even if so,
where is this documented? And why not just point it out when
reviewing it instead of silently waiting for weeks?

We can add this option back when such test materializes.
Right now it's dead code.

It is clearly not dead. We internally used it for testing sch_mq,
this is clearly stated in the git log. How did you draw such a
conclusion without talking to authors?

But this does remind me of using netdevsim for tc-testing.

Thanks.

On Tue, 3 Aug 2021 10:11:13 -0700 Cong Wang wrote:

On Tue, Aug 3, 2021 at 5:39 AM Jakub Kicinski [off-list ref] wrote:

quoted

This reverts commit d4861fc6be581561d6964700110a4dede54da6a6.

netdevsim is for enabling upstream tests, two weeks in
and there's no sign of upstream test using the "mutli-queue"
option.  

Since when netdevsim is *only* for upstream tests?

Since it was created.

Even if so, where is this documented? And why not just point it 
out when reviewing it instead of silently waiting for weeks?

I was AFK for the last two weeks.

quoted

We can add this option back when such test materializes.
Right now it's dead code.  

It is clearly not dead. We internally used it for testing sch_mq,
this is clearly stated in the git log.

Please contribute those tests upstream or keep any test harness 
they require where such test are, out of tree.

How did you draw such a conclusion without talking to authors?

There is no upstream test using this code, and I did CC you, didn't I?

But this does remind me of using netdevsim for tc-testing.

Please bring the code back as part of the series adding upstream tests.

Thank you.

On Tue, Aug 3, 2021 at 2:18 PM Jakub Kicinski [off-list ref] wrote:

On Tue, 3 Aug 2021 10:11:13 -0700 Cong Wang wrote:

quoted

On Tue, Aug 3, 2021 at 5:39 AM Jakub Kicinski [off-list ref] wrote:

quoted

This reverts commit d4861fc6be581561d6964700110a4dede54da6a6.

netdevsim is for enabling upstream tests, two weeks in
and there's no sign of upstream test using the "mutli-queue"
option.

Since when netdevsim is *only* for upstream tests?

Since it was created.

Why it was created only for upstream? IOW, what's wrong with
using it only for non-upstream tests?

BTW, we also use dummy device for testing, it is not only for
upstream. It is extremely odd to single netdevsim out. I don't
see any special reason here.

quoted

Even if so, where is this documented? And why not just point it
out when reviewing it instead of silently waiting for weeks?

I was AFK for the last two weeks.

How about documenting it in netdev-FAQ (or literally any doc)?
This would save everyone's time.

quoted

quoted

We can add this option back when such test materializes.
Right now it's dead code.

It is clearly not dead. We internally used it for testing sch_mq,
this is clearly stated in the git log.

Please contribute those tests upstream or keep any test harness
they require where such test are, out of tree.

Peilin will add tc-testing for sch_mq which requires this netdevsim
feature.

quoted

How did you draw such a conclusion without talking to authors?

There is no upstream test using this code, and I did CC you, didn't I?

There are downstream tests, which are mentioned in changelog.

I am pretty sure upstream tests only cover part of the whole networking
code, if you really want to apply the rule, a lot of code are already dead.
Once again, I don't see any reason why you only treat netdevsim differently.
;)

quoted

But this does remind me of using netdevsim for tc-testing.

Please bring the code back as part of the series adding upstream tests.

Please remove all those not covered by upstream tests just to be fair??

Thank you!

On Tue, 3 Aug 2021 14:32:19 -0700 Cong Wang wrote:

On Tue, Aug 3, 2021 at 2:18 PM Jakub Kicinski [off-list ref] wrote:

quoted

On Tue, 3 Aug 2021 10:11:13 -0700 Cong Wang wrote:  

quoted

On Tue, Aug 3, 2021 at 5:39 AM Jakub Kicinski [off-list ref] wrote:  
Since when netdevsim is *only* for upstream tests?  

Since it was created.  

Why it was created only for upstream? IOW, what's wrong with
using it only for non-upstream tests?

BTW, we also use dummy device for testing, it is not only for
upstream. It is extremely odd to single netdevsim out. I don't
see any special reason here.

From my own experience companies which are serious about their
engineering have a lot of code dedicated to testing. I don't think
we can deal with all such code upstream.

At the same time I want to incentivize upstreaming all of the tests
which are widely applicable (i.e. not HW-specific).

Last but not least test harnesses are really weird from functional, code
lifetime and refactoring perspective. netdevsim is not expected to keep
uAPI as long as in-tree tests do no break/are updated as well.

quoted

quoted

Even if so, where is this documented? And why not just point it
out when reviewing it instead of silently waiting for weeks?  

I was AFK for the last two weeks.  

How about documenting it in netdev-FAQ (or literally any doc)?
This would save everyone's time.

Fair, I'll send a patch.

quoted

quoted

It is clearly not dead. We internally used it for testing sch_mq,
this is clearly stated in the git log.  

Please contribute those tests upstream or keep any test harness
they require where such test are, out of tree.  

Peilin will add tc-testing for sch_mq which requires this netdevsim
feature.

quoted

quoted

How did you draw such a conclusion without talking to authors?  

There is no upstream test using this code, and I did CC you, didn't I?  

There are downstream tests, which are mentioned in changelog.

I am pretty sure upstream tests only cover part of the whole networking
code, if you really want to apply the rule, a lot of code are already dead.
Once again, I don't see any reason why you only treat netdevsim differently.
;)

I hope the first part of this response scheds some light.

quoted

quoted

But this does remind me of using netdevsim for tc-testing.  

Please bring the code back as part of the series adding upstream tests.  

Please remove all those not covered by upstream tests just to be fair??

I'd love to remove all test harnesses upstream which are not used by
upstream tests, sure :)

On Tue, Aug 3, 2021 at 2:51 PM Jakub Kicinski [off-list ref] wrote:

On Tue, 3 Aug 2021 14:32:19 -0700 Cong Wang wrote:

quoted

On Tue, Aug 3, 2021 at 2:18 PM Jakub Kicinski [off-list ref] wrote:

quoted

On Tue, 3 Aug 2021 10:11:13 -0700 Cong Wang wrote:

quoted

On Tue, Aug 3, 2021 at 5:39 AM Jakub Kicinski [off-list ref] wrote:
Since when netdevsim is *only* for upstream tests?

Since it was created.

Why it was created only for upstream? IOW, what's wrong with
using it only for non-upstream tests?

BTW, we also use dummy device for testing, it is not only for
upstream. It is extremely odd to single netdevsim out. I don't
see any special reason here.

From my own experience companies which are serious about their
engineering have a lot of code dedicated to testing. I don't think
we can deal with all such code upstream.

At the same time I want to incentivize upstreaming all of the tests
which are widely applicable (i.e. not HW-specific).

So, nothing special for netdevsim? This seems applicable to all code,
not just netdevsim code.

Last but not least test harnesses are really weird from functional, code
lifetime and refactoring perspective. netdevsim is not expected to keep
uAPI as long as in-tree tests do no break/are updated as well.

Sure. Our test is not any special, sch_mq is in upstream, only sch_mq
tests are not yet. Peilin will send out sch_mq tests very soon.

quoted

quoted

quoted

Even if so, where is this documented? And why not just point it
out when reviewing it instead of silently waiting for weeks?

I was AFK for the last two weeks.

How about documenting it in netdev-FAQ (or literally any doc)?
This would save everyone's time.

Fair, I'll send a patch.

Great! Really appreciate it.

quoted

quoted

quoted

It is clearly not dead. We internally used it for testing sch_mq,
this is clearly stated in the git log.

Please contribute those tests upstream or keep any test harness
they require where such test are, out of tree.

Peilin will add tc-testing for sch_mq which requires this netdevsim
feature.

quoted

quoted

How did you draw such a conclusion without talking to authors?

There is no upstream test using this code, and I did CC you, didn't I?

There are downstream tests, which are mentioned in changelog.

I am pretty sure upstream tests only cover part of the whole networking
code, if you really want to apply the rule, a lot of code are already dead.
Once again, I don't see any reason why you only treat netdevsim differently.
;)

I hope the first part of this response scheds some light.

quoted

quoted

quoted

But this does remind me of using netdevsim for tc-testing.

Please bring the code back as part of the series adding upstream tests.

Please remove all those not covered by upstream tests just to be fair??

I'd love to remove all test harnesses upstream which are not used by
upstream tests, sure :)

Many net/*/ code can be gone. Maybe start with net/netrom/? ;)

Thanks.

From: Peilin Ye <redacted>

Recently we added multi-queue support to netdevsim in commit d4861fc6be58
("netdevsim: Add multi-queue support"); add a few control-plane selftests
for sch_mq using this new feature.

Use nsPlugin.py to avoid network interface name collisions.

Reviewed-by: Cong Wang <redacted>
Signed-off-by: Peilin Ye <redacted>
---
Hi all,

Here are some control-plane selftests for the mq Qdisc using netdevsim's
new multi-queue feature.  We are planning to add more data-plane selftests
in the future.

Thank you,
Peilin Ye

 .../tc-testing/tc-tests/qdiscs/mq.json        | 137 ++++++++++++++++++
 .../selftests/tc-testing/tdc_config.py        |   1 +
 2 files changed, 138 insertions(+)
 create mode 100644 tools/testing/selftests/tc-testing/tc-tests/qdiscs/mq.json

diff --git a/tools/testing/selftests/tc-testing/tc-tests/qdiscs/mq.json b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/mq.json
new file mode 100644
index 000000000000..88a20c781e49
--- /dev/null
+++ b/tools/testing/selftests/tc-testing/tc-tests/qdiscs/mq.json

@@ -0,0 +1,137 @@
+[
+	{
+	    "id": "ce7d",
+	    "name": "Add mq Qdisc to multi-queue device (4 queues)",
+	    "category": [
+            "qdisc",
+            "mq"
+	    ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+	    "setup": [
+            "echo \"1 1 4\" > /sys/bus/netdevsim/new_device"
+	    ],
+	    "cmdUnderTest": "$TC qdisc add dev $ETH root handle 1: mq",
+	    "expExitCode": "0",
+	    "verifyCmd": "$TC qdisc show dev $ETH",
+	    "matchPattern": "qdisc pfifo_fast 0: parent 1:[1-4] bands 3 priomap 1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1",
+	    "matchCount": "4",
+	    "teardown": [
+		    "echo \"1\" > /sys/bus/netdevsim/del_device"
+	    ]
+	},
+	{
+	    "id": "2f82",
+	    "name": "Add mq Qdisc to multi-queue device (256 queues)",
+	    "category": [
+            "qdisc",
+            "mq"
+	    ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+	    "setup": [
+            "echo \"1 1 256\" > /sys/bus/netdevsim/new_device"
+	    ],
+	    "cmdUnderTest": "$TC qdisc add dev $ETH root handle 1: mq",
+	    "expExitCode": "0",
+	    "verifyCmd": "$TC qdisc show dev $ETH",
+	    "matchPattern": "qdisc pfifo_fast 0: parent 1:[1-9,a-f][0-9,a-f]{0,2} bands 3 priomap 1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1",
+	    "matchCount": "256",
+	    "teardown": [
+		    "echo \"1\" > /sys/bus/netdevsim/del_device"
+	    ]
+	},
+	{
+	    "id": "c525",
+	    "name": "Add duplicate mq Qdisc",
+	    "category": [
+            "qdisc",
+            "mq"
+	    ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+	    "setup": [
+            "echo \"1 1 4\" > /sys/bus/netdevsim/new_device",
+            "$TC qdisc add dev $ETH root handle 1: mq"
+	    ],
+	    "cmdUnderTest": "$TC qdisc add dev $ETH root handle 1: mq",
+	    "expExitCode": "2",
+	    "verifyCmd": "$TC qdisc show dev $ETH",
+	    "matchPattern": "qdisc pfifo_fast 0: parent 1:[1-4] bands 3 priomap 1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1",
+	    "matchCount": "4",
+	    "teardown": [
+		    "echo \"1\" > /sys/bus/netdevsim/del_device"
+	    ]
+	},
+	{
+	    "id": "128a",
+	    "name": "Delete nonexistent mq Qdisc",
+	    "category": [
+            "qdisc",
+            "mq"
+	    ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+	    "setup": [
+            "echo \"1 1 4\" > /sys/bus/netdevsim/new_device"
+	    ],
+	    "cmdUnderTest": "$TC qdisc del dev $ETH root handle 1: mq",
+	    "expExitCode": "2",
+	    "verifyCmd": "$TC qdisc show dev $ETH",
+	    "matchPattern": "qdisc pfifo_fast 0: parent 1:[1-4] bands 3 priomap 1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1",
+	    "matchCount": "0",
+	    "teardown": [
+		    "echo \"1\" > /sys/bus/netdevsim/del_device"
+	    ]
+	},
+	{
+	    "id": "03a9",
+	    "name": "Delete mq Qdisc twice",
+	    "category": [
+            "qdisc",
+            "mq"
+	    ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+	    "setup": [
+            "echo \"1 1 4\" > /sys/bus/netdevsim/new_device",
+            "$TC qdisc add dev $ETH root handle 1: mq",
+            "$TC qdisc del dev $ETH root handle 1: mq"
+	    ],
+	    "cmdUnderTest": "$TC qdisc del dev $ETH root handle 1: mq",
+	    "expExitCode": "2",
+	    "verifyCmd": "$TC qdisc show dev $ETH",
+	    "matchPattern": "qdisc pfifo_fast 0: parent 1:[1-4] bands 3 priomap 1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1",
+	    "matchCount": "0",
+	    "teardown": [
+		    "echo \"1\" > /sys/bus/netdevsim/del_device"
+	    ]
+	},
+    {
+	    "id": "be0f",
+	    "name": "Add mq Qdisc to single-queue device",
+	    "category": [
+            "qdisc",
+            "mq"
+	    ],
+        "plugins": {
+            "requires": "nsPlugin"
+        },
+	    "setup": [
+            "echo \"1 1\" > /sys/bus/netdevsim/new_device"
+	    ],
+	    "cmdUnderTest": "$TC qdisc add dev $ETH root handle 1: mq",
+	    "expExitCode": "2",
+	    "verifyCmd": "$TC qdisc show dev $ETH",
+	    "matchPattern": "qdisc pfifo_fast 0: parent 1:[1-4] bands 3 priomap 1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1",
+	    "matchCount": "0",
+	    "teardown": [
+		    "echo \"1\" > /sys/bus/netdevsim/del_device"
+	    ]
+	}
+]

diff --git a/tools/testing/selftests/tc-testing/tdc_config.py b/tools/testing/selftests/tc-testing/tdc_config.py
index cd4a27ee1466..ea04f04c173e 100644
--- a/tools/testing/selftests/tc-testing/tdc_config.py
+++ b/tools/testing/selftests/tc-testing/tdc_config.py

@@ -17,6 +17,7 @@ NAMES = {
           'DEV1': 'v0p1',
           'DEV2': '',
           'DUMMY': 'dummy1',
+	  'ETH': 'eth0',
           'BATCH_FILE': './batch.txt',
           'BATCH_DIR': 'tmp',
           # Length of time in seconds to wait before terminating a command

-- 
2.20.1

On Tue, Aug 3, 2021 at 3:17 PM Peilin Ye [off-list ref] wrote:

+           "setup": [
+            "echo \"1 1 4\" > /sys/bus/netdevsim/new_device"
+           ],
+           "cmdUnderTest": "$TC qdisc add dev $ETH root handle 1: mq",
+           "expExitCode": "0",
+           "verifyCmd": "$TC qdisc show dev $ETH",
+           "matchPattern": "qdisc pfifo_fast 0: parent 1:[1-4] bands 3 priomap 1 2 2 2 1 2 0 0 1 1 1 1 1 1 1 1",
+           "matchCount": "4",
+           "teardown": [
+                   "echo \"1\" > /sys/bus/netdevsim/del_device"
+           ]
+       },

Like I mentioned to Peilin, I am _not_ sure whether it is better to create
netdevsim device in such a way. Maybe we need to create it before
these tests and pass it via cmdline?? Lucas?

Thanks.

On Tue, Aug 03, 2021 at 02:51:24PM -0700, Jakub Kicinski wrote:

On Tue, 3 Aug 2021 14:32:19 -0700 Cong Wang wrote:

quoted

On Tue, Aug 3, 2021 at 2:18 PM Jakub Kicinski [off-list ref] wrote:

<...>

quoted

Please remove all those not covered by upstream tests just to be fair??

I'd love to remove all test harnesses upstream which are not used by
upstream tests, sure :)

Jakub,

Something related and unrelated at the same time.

I need to get rid of devlink_reload_enable()/_disable() to fix some
panics in the devlink reload flow.

Such change is relatively easy for the HW drivers, but not so for the
netdevism due to attempt to synchronize sysfs with devlink.

  200         mutex_lock(&nsim_bus_dev->nsim_bus_reload_lock);
  201         devlink_reload_disable(devlink);
  202         ret = nsim_dev_port_add(nsim_bus_dev, NSIM_DEV_PORT_TYPE_PF, port_index);
  203         devlink_reload_enable(devlink);
  204         mutex_unlock(&nsim_bus_dev->nsim_bus_reload_lock);

Are these sysfs files declared as UAPI? Or can I update upstream test
suite and delete them safely?

Thanks

Hello:

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

On Tue,  3 Aug 2021 15:16:59 -0700 you wrote:

From: Peilin Ye <redacted>

Recently we added multi-queue support to netdevsim in commit d4861fc6be58
("netdevsim: Add multi-queue support"); add a few control-plane selftests
for sch_mq using this new feature.

Use nsPlugin.py to avoid network interface name collisions.

[...]

Here is the summary with links:
  - [net-next] tc-testing: Add control-plane selftests for sch_mq
    https://git.kernel.org/netdev/net-next/c/625af9f0298b

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

On Wed, 4 Aug 2021 10:14:36 +0300 Leon Romanovsky wrote:

On Tue, Aug 03, 2021 at 02:51:24PM -0700, Jakub Kicinski wrote:

quoted

On Tue, 3 Aug 2021 14:32:19 -0700 Cong Wang wrote:  

quoted

On Tue, Aug 3, 2021 at 2:18 PM Jakub Kicinski [off-list ref] wrote:  

<...>

quoted

quoted

Please remove all those not covered by upstream tests just to be fair??  

I'd love to remove all test harnesses upstream which are not used by
upstream tests, sure :)  

Jakub,

Something related and unrelated at the same time.

I need to get rid of devlink_reload_enable()/_disable() to fix some
panics in the devlink reload flow.

Such change is relatively easy for the HW drivers, but not so for the
netdevism due to attempt to synchronize sysfs with devlink.

  200         mutex_lock(&nsim_bus_dev->nsim_bus_reload_lock);
  201         devlink_reload_disable(devlink);
  202         ret = nsim_dev_port_add(nsim_bus_dev, NSIM_DEV_PORT_TYPE_PF, port_index);
  203         devlink_reload_enable(devlink);
  204         mutex_unlock(&nsim_bus_dev->nsim_bus_reload_lock);

Are these sysfs files declared as UAPI? Or can I update upstream test
suite and delete them safely?

You can change netdevsim in whatever way is appropriate.

What's your plan, tho? Jiri changed the spawning from rtnetlink 
to sysfs - may be good to consult with him before typing too much 
code.

On Wed, Aug 04, 2021 at 04:52:47AM -0700, Jakub Kicinski wrote:

On Wed, 4 Aug 2021 10:14:36 +0300 Leon Romanovsky wrote:

quoted

On Tue, Aug 03, 2021 at 02:51:24PM -0700, Jakub Kicinski wrote:

quoted

On Tue, 3 Aug 2021 14:32:19 -0700 Cong Wang wrote:  

quoted

On Tue, Aug 3, 2021 at 2:18 PM Jakub Kicinski [off-list ref] wrote:  

<...>

quoted

quoted

Please remove all those not covered by upstream tests just to be fair??  

I'd love to remove all test harnesses upstream which are not used by
upstream tests, sure :)  

Jakub,

Something related and unrelated at the same time.

I need to get rid of devlink_reload_enable()/_disable() to fix some
panics in the devlink reload flow.

Such change is relatively easy for the HW drivers, but not so for the
netdevism due to attempt to synchronize sysfs with devlink.

  200         mutex_lock(&nsim_bus_dev->nsim_bus_reload_lock);
  201         devlink_reload_disable(devlink);
  202         ret = nsim_dev_port_add(nsim_bus_dev, NSIM_DEV_PORT_TYPE_PF, port_index);
  203         devlink_reload_enable(devlink);
  204         mutex_unlock(&nsim_bus_dev->nsim_bus_reload_lock);

Are these sysfs files declared as UAPI? Or can I update upstream test
suite and delete them safely?

You can change netdevsim in whatever way is appropriate.

What's your plan, tho? Jiri changed the spawning from rtnetlink 
to sysfs - may be good to consult with him before typing too much 
code.

It is something preliminary, I have POC code which works but it is far
from the actual patches yet.

The problem is that "devlink reload" in its current form causes us
(mlx5) a lot of grief. We see deadlocks due to combinations of internal
flows with external ones, without going too much in details loops of
module removal together with health recovery and devlink reload doesn't
work properly :).

The same problem exists in all drivers that implement "devlink reload",
mlx5 just most complicated one and looks like most tested either.

My idea (for now) is pretty simple:
1. Move devlink ops callbacks from devlink_alloc phase to devlink_register().
2. Ensure that devlink_register() is the last command in the probe sequence.
3. Delete devlink_reload_enable/disable. It is not needed if proper ops used.
4. Add reference counting to struct devlink to make sure that we
properly account netlink users, so we will be able to drop big devlink_lock.
5. Convert linked list of devlink instances to be xarray. It gives us an
option to work relatively lockless.
....

Every step solves some bug, even first one solves current bug where
devlink reload statistics presented despite devlink_reload_disable().

Of course, we can try to patch devlink with specific fix for specific
bug, but better to make it error prone from the beginning.

So I'm trying to get a sense what can and what can't be done in the netdev.
And netdevsim combination of devlink and sysfs knobs adds challenges. :)

Thanks

On Wed, 4 Aug 2021 15:49:53 +0300 Leon Romanovsky wrote:

It is something preliminary, I have POC code which works but it is far
from the actual patches yet.

The problem is that "devlink reload" in its current form causes us
(mlx5) a lot of grief. We see deadlocks due to combinations of internal
flows with external ones, without going too much in details loops of
module removal together with health recovery and devlink reload doesn't
work properly :).

The same problem exists in all drivers that implement "devlink reload",
mlx5 just most complicated one and looks like most tested either.

My idea (for now) is pretty simple:
1. Move devlink ops callbacks from devlink_alloc phase to devlink_register().
2. Ensure that devlink_register() is the last command in the probe sequence.

IDK.. does that work with port registration vs netdev registration?
IIRC ports should be registered first.

In general devlink is between bus devices and netdevs so I think
devlink should be initialized first, right?

Is merging the two flows (probe vs reload) possible? What I mean is can
we make the drivers use reload_up() during probe? IOW if driver has
.reload_up() make devlink core call that on register with whatever
locks it holds to prevent double-reload?

Either way please make sure to dump all the knowledge you gain about
the locking to some doc. Seems like that's a major sore spot for
devlink.

3. Delete devlink_reload_enable/disable. It is not needed if proper ops used.
4. Add reference counting to struct devlink to make sure that we
properly account netlink users, so we will be able to drop big devlink_lock.
5. Convert linked list of devlink instances to be xarray. It gives us an
option to work relatively lockless.
....

Every step solves some bug, even first one solves current bug where
devlink reload statistics presented despite devlink_reload_disable().

Of course, we can try to patch devlink with specific fix for specific
bug, but better to make it error prone from the beginning.

So I'm trying to get a sense what can and what can't be done in the netdev.
And netdevsim combination of devlink and sysfs knobs adds challenges. :)

On Wed, Aug 04, 2021 at 08:56:17AM -0700, Jakub Kicinski wrote:

On Wed, 4 Aug 2021 15:49:53 +0300 Leon Romanovsky wrote:

quoted

It is something preliminary, I have POC code which works but it is far
from the actual patches yet.

The problem is that "devlink reload" in its current form causes us
(mlx5) a lot of grief. We see deadlocks due to combinations of internal
flows with external ones, without going too much in details loops of
module removal together with health recovery and devlink reload doesn't
work properly :).

The same problem exists in all drivers that implement "devlink reload",
mlx5 just most complicated one and looks like most tested either.

My idea (for now) is pretty simple:
1. Move devlink ops callbacks from devlink_alloc phase to devlink_register().
2. Ensure that devlink_register() is the last command in the probe sequence.

IDK.. does that work with port registration vs netdev registration?
IIRC ports should be registered first.

I just throw the sketch, the proper solution requires to change all
devlink_*_register() function to accept relevant to that object ops.

In general devlink is between bus devices and netdevs so I think
devlink should be initialized first, right?

Yes, because there are driver initialization parameters involved.

My personal opinion is that devlink is implemented in wrong layer
and everything would be much easier if it was part of driver/bus,
so it will be aware of driver core state machine and would hold
device_lock as part of driver core.

It would eliminate PCI recovery, devlink reload e.t.c races.

Is merging the two flows (probe vs reload) possible? What I mean is can
we make the drivers use reload_up() during probe? IOW if driver has
.reload_up() make devlink core call that on register with whatever
locks it holds to prevent double-reload?

It was one of my internal POC, the problem is that .probe() is called
with device_lock, so I needed to teach devlink to hold that lock too.
The end result didn't look nice.

Either way please make sure to dump all the knowledge you gain about
the locking to some doc. Seems like that's a major sore spot for
devlink.

Sure

quoted

3. Delete devlink_reload_enable/disable. It is not needed if proper ops used.
4. Add reference counting to struct devlink to make sure that we
properly account netlink users, so we will be able to drop big devlink_lock.
5. Convert linked list of devlink instances to be xarray. It gives us an
option to work relatively lockless.
....

Every step solves some bug, even first one solves current bug where
devlink reload statistics presented despite devlink_reload_disable().

Of course, we can try to patch devlink with specific fix for specific
bug, but better to make it error prone from the beginning.

So I'm trying to get a sense what can and what can't be done in the netdev.
And netdevsim combination of devlink and sysfs knobs adds challenges. :)