Re: [PATCH net-next 6/6] netdevsim: Add simple FIB resource controller via devlink

[Jiri Pirko <jiri@resnulli.us>]

Wed, Mar 28, 2018 at 03:22:00AM CEST, dsa@cumulusnetworks.com wrote:
>Add devlink support to netdevsim and use it to implement a simple,
>profile based resource controller. Only one controller is needed
>per namespace, so the first netdevsim netdevice in a namespace
>registers with devlink. If that device is deleted, the resource
>settings are deleted.

I don't understand why you add 1:1 fixed relationship between
netnamespace and devlink instance. That is highly misleading and reader
might think that those 2 are somehow related. They are not. You can have
multiple devlink instances for many ports in a single namespace.

Could you please clarify?

Also, to see the relationship between individual netdevsim netdevices
and the parent devlink instance, we should use devlink_port
instances, like this: 

      devlink1              devlink2
       |    |                |    |
 dl_port1_1 dlport1_2   dlport2_1 dlport2_2
       |    |                |    |
     eth0  eth1             eth2 eth3

Note that "devlink instance" reprensents one ASIC.
The address of the devlink instance is the bus address of the ASIC.
Here, you use address of some/first netdevsim netdev instance.

The way it is implemented in netdevsim by this patch is wrong on
so many levels :(

Could you please fix this? I'm more than happy to help you with this,
please say so. Thanks!


[...]

>+	err = devlink_resource_register(devlink, "IPv4", (u64)-1,
>+					NSIM_RESOURCE_IPV4,
>+					DEVLINK_RESOURCE_ID_PARENT_TOP,
>+					&params, NULL);
>+	if (err) {
>+		pr_err("Failed to register IPv4 top resource\n");
>+		goto out;


this goto is pointless. Just return.