Re: [PATCH RFC net-next 7/7] netdevsim: Add simple FIB resource controller via devlink
Sun, Mar 25, 2018 at 04:24:11PM CEST, d...@cumulusnetworks.com wrote: >On 3/24/18 10:02 AM, Jiri Pirko wrote: Wait a second. What do you mean by "per-network namespace"? Devlink instance is always associated with one physical device. Like an ASIC. > has a net entry, the simplest design is to put it into the namespace > of > the controller. Without it, controlling resource sizes in namespace > 'foobar' has to be done from init_net, which is just wrong. >>> >>> you need to look at how netdevsim creates a device per netdevice. >> >> That means one devlink instance for each netdevsim device, doesn't it? >> > > yes. Still not sure how to handle namespaces in devlink. Originally, I thought it would be okay to leave all devlink instances in init_ns. Because what happens if you move netdev to another namespace? Should the devlink move as well? What if you have multiple ports, each in different namespace. Can user move devlink instance to another namespace? Etc. >>> >>> The devlink instance is associated with a 'struct device' and those do >>> not change namespaces AFAIK. >> >> Yeah. But you put devlink instance into namespace according to struct >> net_device. That is mismatch. >> > >New netdevsim netdevice creates a new 'struct device' which creates a >new devlink instance. The namespace the netdev is created in is then >passed to the devlink instance. Yes, the netdev could change namespaces, >but that is something we can easily prevent if it has a devlink instance. > >But really, we are way down a tangent with respect to the intent of this >patch set. I am fine with limiting the example resource controller to I know. That is just something that I spotted :)
Re: [PATCH RFC net-next 7/7] netdevsim: Add simple FIB resource controller via devlink
On Sun, 25 Mar 2018 08:27:42 -0600, David Ahern wrote: > On 3/25/18 12:35 AM, Jakub Kicinski wrote: > > On Sat, 24 Mar 2018 09:02:45 -0600, David Ahern wrote: > diff --git a/drivers/net/netdevsim/Makefile > b/drivers/net/netdevsim/Makefile > index 09388c06171d..449b2a1a1800 100644 > --- a/drivers/net/netdevsim/Makefile > +++ b/drivers/net/netdevsim/Makefile > @@ -9,3 +9,7 @@ ifeq ($(CONFIG_BPF_SYSCALL),y) > netdevsim-objs += \ > bpf.o > endif > + > +ifneq ($(CONFIG_NET_DEVLINK),) > >>> > >>> Hm. Don't you need MAY_USE_DEVLINK dependency perhaps? > >> > >> mlxsw uses CONFIG_NET_DEVLINK in its Makefile. > >> > >> MAY_USE_DEVLINK seems to only be used in Kconfig files. Not clear to me > >> why it is needed at all. > > > > NETDEVSIM=y && DEVLINK=m > > > > ok. I purposely did not add DEVLINK as a dependency to netdevsim to make > the resource controller truly optional. Can add it if you prefer. Oh, no, I don't mind. I just thought NETDEVSIM=y DEVLINK=m case will break the build, but I haven't tested. If it works perfect, let's not add unnecessary dependencies :) (FWIW the MAY_USE_DEVLINK dep is basically depend on DEVLINK || DEVLINK=n, so one can still build without devlink but if devlink is a module netdevsim will also have to be.)
Re: [PATCH RFC net-next 7/7] netdevsim: Add simple FIB resource controller via devlink
On 3/25/18 12:35 AM, Jakub Kicinski wrote: > On Sat, 24 Mar 2018 09:02:45 -0600, David Ahern wrote: diff --git a/drivers/net/netdevsim/Makefile b/drivers/net/netdevsim/Makefile index 09388c06171d..449b2a1a1800 100644 --- a/drivers/net/netdevsim/Makefile +++ b/drivers/net/netdevsim/Makefile @@ -9,3 +9,7 @@ ifeq ($(CONFIG_BPF_SYSCALL),y) netdevsim-objs += \ bpf.o endif + +ifneq ($(CONFIG_NET_DEVLINK),) >>> >>> Hm. Don't you need MAY_USE_DEVLINK dependency perhaps? >> >> mlxsw uses CONFIG_NET_DEVLINK in its Makefile. >> >> MAY_USE_DEVLINK seems to only be used in Kconfig files. Not clear to me >> why it is needed at all. > > NETDEVSIM=y && DEVLINK=m > ok. I purposely did not add DEVLINK as a dependency to netdevsim to make the resource controller truly optional. Can add it if you prefer.
Re: [PATCH RFC net-next 7/7] netdevsim: Add simple FIB resource controller via devlink
On 3/24/18 10:02 AM, Jiri Pirko wrote: >>> >>> Wait a second. What do you mean by "per-network namespace"? Devlink >>> instance is always associated with one physical device. Like an ASIC. >>> >>> has a net entry, the simplest design is to put it into the namespace of the controller. Without it, controlling resource sizes in namespace 'foobar' has to be done from init_net, which is just wrong. >> >> you need to look at how netdevsim creates a device per netdevice. > > That means one devlink instance for each netdevsim device, doesn't it? > yes. >>> >>> Still not sure how to handle namespaces in devlink. Originally, I >>> thought it would be okay to leave all devlink instances in init_ns. >>> Because what happens if you move netdev to another namespace? Should the >>> devlink move as well? What if you have multiple ports, each in different >>> namespace. Can user move devlink instance to another namespace? Etc. >>> >> >> The devlink instance is associated with a 'struct device' and those do >> not change namespaces AFAIK. > > Yeah. But you put devlink instance into namespace according to struct > net_device. That is mismatch. > New netdevsim netdevice creates a new 'struct device' which creates a new devlink instance. The namespace the netdev is created in is then passed to the devlink instance. Yes, the netdev could change namespaces, but that is something we can easily prevent if it has a devlink instance. But really, we are way down a tangent with respect to the intent of this patch set. I am fine with limiting the example resource controller to just the init_net; this patch set is really aimed at the bigger idea of allowing a notifier handler to fail route and rule adds. We can revisit devlink and namespaces another time, along with moving switch ports to namespaces - a key part of implementing a feature equivalent to what Cisco calls a VDC [1]. [1] https://www.cisco.com/c/en/us/products/collateral/switches/nexus-7000-10-slot-switch/White_Paper_Tech_Overview_Virtual_Device_Contexts.html
Re: [PATCH RFC net-next 7/7] netdevsim: Add simple FIB resource controller via devlink
On Sat, 24 Mar 2018 09:02:45 -0600, David Ahern wrote: > >> diff --git a/drivers/net/netdevsim/Makefile > >> b/drivers/net/netdevsim/Makefile > >> index 09388c06171d..449b2a1a1800 100644 > >> --- a/drivers/net/netdevsim/Makefile > >> +++ b/drivers/net/netdevsim/Makefile > >> @@ -9,3 +9,7 @@ ifeq ($(CONFIG_BPF_SYSCALL),y) > >> netdevsim-objs += \ > >>bpf.o > >> endif > >> + > >> +ifneq ($(CONFIG_NET_DEVLINK),) > > > > Hm. Don't you need MAY_USE_DEVLINK dependency perhaps? > > mlxsw uses CONFIG_NET_DEVLINK in its Makefile. > > MAY_USE_DEVLINK seems to only be used in Kconfig files. Not clear to me > why it is needed at all. NETDEVSIM=y && DEVLINK=m
Re: [PATCH RFC net-next 7/7] netdevsim: Add simple FIB resource controller via devlink
Sat, Mar 24, 2018 at 04:05:38PM CET, d...@cumulusnetworks.com wrote:
>On 3/24/18 1:26 AM, Jiri Pirko wrote:
>> Fri, Mar 23, 2018 at 04:13:14PM CET, d...@cumulusnetworks.com wrote:
>>> On 3/23/18 9:05 AM, Jiri Pirko wrote:
Fri, Mar 23, 2018 at 04:03:40PM CET, d...@cumulusnetworks.com wrote:
> On 3/23/18 9:01 AM, Jiri Pirko wrote:
>> Fri, Mar 23, 2018 at 03:31:02PM CET, d...@cumulusnetworks.com wrote:
>>> On 3/23/18 12:50 AM, Jiri Pirko wrote:
> +void nsim_devlink_setup(struct netdevsim *ns)
> +{
> + struct net *net = dev_net(ns->netdev);
> + bool *reg_devlink = net_generic(net, nsim_devlink_id);
> + struct devlink *devlink;
> + int err = -ENOMEM;
> +
> + /* only one device per namespace controls devlink */
> + if (!*reg_devlink) {
> + ns->devlink = NULL;
> + return;
> + }
> +
> + devlink = devlink_alloc(&nsim_devlink_ops, 0);
> + if (!devlink)
> + return;
> +
> + devlink_net_set(devlink, net);
> + err = devlink_register(devlink, &ns->dev);
This reg_devlink construct looks odd. Why don't you leave the devlink
instance in init_ns?
>>>
>>> It is a per-network namespace resource controller. Since struct devlink
>>
>> Wait a second. What do you mean by "per-network namespace"? Devlink
>> instance is always associated with one physical device. Like an ASIC.
>>
>>
>>> has a net entry, the simplest design is to put it into the namespace of
>>> the controller. Without it, controlling resource sizes in namespace
>>> 'foobar' has to be done from init_net, which is just wrong.
>
> you need to look at how netdevsim creates a device per netdevice.
That means one devlink instance for each netdevsim device, doesn't it?
>>>
>>> yes.
>>
>> Still not sure how to handle namespaces in devlink. Originally, I
>> thought it would be okay to leave all devlink instances in init_ns.
>> Because what happens if you move netdev to another namespace? Should the
>> devlink move as well? What if you have multiple ports, each in different
>> namespace. Can user move devlink instance to another namespace? Etc.
>>
>
>The devlink instance is associated with a 'struct device' and those do
>not change namespaces AFAIK.
Yeah. But you put devlink instance into namespace according to struct
net_device. That is mismatch.
Re: [PATCH RFC net-next 7/7] netdevsim: Add simple FIB resource controller via devlink
On 3/24/18 1:26 AM, Jiri Pirko wrote:
> Fri, Mar 23, 2018 at 04:13:14PM CET, d...@cumulusnetworks.com wrote:
>> On 3/23/18 9:05 AM, Jiri Pirko wrote:
>>> Fri, Mar 23, 2018 at 04:03:40PM CET, d...@cumulusnetworks.com wrote:
On 3/23/18 9:01 AM, Jiri Pirko wrote:
> Fri, Mar 23, 2018 at 03:31:02PM CET, d...@cumulusnetworks.com wrote:
>> On 3/23/18 12:50 AM, Jiri Pirko wrote:
+void nsim_devlink_setup(struct netdevsim *ns)
+{
+ struct net *net = dev_net(ns->netdev);
+ bool *reg_devlink = net_generic(net, nsim_devlink_id);
+ struct devlink *devlink;
+ int err = -ENOMEM;
+
+ /* only one device per namespace controls devlink */
+ if (!*reg_devlink) {
+ ns->devlink = NULL;
+ return;
+ }
+
+ devlink = devlink_alloc(&nsim_devlink_ops, 0);
+ if (!devlink)
+ return;
+
+ devlink_net_set(devlink, net);
+ err = devlink_register(devlink, &ns->dev);
>>>
>>> This reg_devlink construct looks odd. Why don't you leave the devlink
>>> instance in init_ns?
>>
>> It is a per-network namespace resource controller. Since struct devlink
>
> Wait a second. What do you mean by "per-network namespace"? Devlink
> instance is always associated with one physical device. Like an ASIC.
>
>
>> has a net entry, the simplest design is to put it into the namespace of
>> the controller. Without it, controlling resource sizes in namespace
>> 'foobar' has to be done from init_net, which is just wrong.
you need to look at how netdevsim creates a device per netdevice.
>>>
>>> That means one devlink instance for each netdevsim device, doesn't it?
>>>
>>
>> yes.
>
> Still not sure how to handle namespaces in devlink. Originally, I
> thought it would be okay to leave all devlink instances in init_ns.
> Because what happens if you move netdev to another namespace? Should the
> devlink move as well? What if you have multiple ports, each in different
> namespace. Can user move devlink instance to another namespace? Etc.
>
The devlink instance is associated with a 'struct device' and those do
not change namespaces AFAIK.
Re: [PATCH RFC net-next 7/7] netdevsim: Add simple FIB resource controller via devlink
On 3/23/18 9:47 PM, Jakub Kicinski wrote:
> On Thu, 22 Mar 2018 15:57:57 -0700, David Ahern wrote:
>> From: David Ahern
>>
>> 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.
>
> FWIW some nits from me blow.
>
>> diff --git a/drivers/net/netdevsim/Makefile b/drivers/net/netdevsim/Makefile
>> index 09388c06171d..449b2a1a1800 100644
>> --- a/drivers/net/netdevsim/Makefile
>> +++ b/drivers/net/netdevsim/Makefile
>> @@ -9,3 +9,7 @@ ifeq ($(CONFIG_BPF_SYSCALL),y)
>> netdevsim-objs += \
>> bpf.o
>> endif
>> +
>> +ifneq ($(CONFIG_NET_DEVLINK),)
>
> Hm. Don't you need MAY_USE_DEVLINK dependency perhaps?
mlxsw uses CONFIG_NET_DEVLINK in its Makefile.
MAY_USE_DEVLINK seems to only be used in Kconfig files. Not clear to me
why it is needed at all.
>
>> +netdevsim-objs += devlink.o fib.o
>> +endif
>> diff --git a/drivers/net/netdevsim/devlink.c
>> b/drivers/net/netdevsim/devlink.c
>> new file mode 100644
>> index ..d10558e1b022
>> --- /dev/null
>> +++ b/drivers/net/netdevsim/devlink.c
>
>> +static int devlink_resources_register(struct devlink *devlink)
>> +{
>> +struct devlink_resource_size_params params = {
>> +.size_max = (u64)-1,
>> +.size_granularity = 1,
>> +.unit = DEVLINK_RESOURCE_UNIT_ENTRY
>> +};
>> +struct net *net = devlink_net(devlink);
>> +int err;
>> +u64 n;
>> +
>> +/* Resources for IPv4 */
>> +err = devlink_resource_register(devlink, "IPv4", (u64)-1,
>> +NSIM_RESOURCE_IPV4,
>> +DEVLINK_RESOURCE_ID_PARENT_TOP,
>> +¶ms, NULL);
>> +if (err) {
>> +pr_err("Failed to register IPv4 top resource\n");
>> +goto out;
>
> nit: why goto out here and return err everywhere else? If I was to
> choose I'd rather see returns. goto X; X: return; is less
> obviously correct IMHO. Besides labels should be called by the
> action they perform/undo, so goto err_return? :S
Will fix. Just got lost in the many iterations leading up to the RFC.
>
>> +}
>> +
>> +n = nsim_fib_get_val(net, NSIM_RESOURCE_IPV4_FIB, true);
>> +err = devlink_resource_register(devlink, "fib", n,
>> +NSIM_RESOURCE_IPV4_FIB,
>> +NSIM_RESOURCE_IPV4,
>> +¶ms, &nsim_ipv4_fib_res_ops);
>> +if (err) {
>> +pr_err("Failed to register IPv4 FIB resource\n");
>> +return err;
>> +}
>> +
>> +n = nsim_fib_get_val(net, NSIM_RESOURCE_IPV4_FIB_RULES, true);
>> +err = devlink_resource_register(devlink, "fib-rules", n,
>> +NSIM_RESOURCE_IPV4_FIB_RULES,
>> +NSIM_RESOURCE_IPV4,
>> +¶ms, &nsim_ipv4_fib_rules_res_ops);
>> +if (err) {
>> +pr_err("Failed to register IPv4 FIB rules resource\n");
>> +return err;
>> +}
>> +
>> +/* Resources for IPv6 */
>> +err = devlink_resource_register(devlink, "IPv6", (u64)-1,
>> +NSIM_RESOURCE_IPV6,
>> +DEVLINK_RESOURCE_ID_PARENT_TOP,
>> +¶ms, NULL);
>> +if (err) {
>> +pr_err("Failed to register IPv6 top resource\n");
>> +goto out;
>> +}
>> +
>> +n = nsim_fib_get_val(net, NSIM_RESOURCE_IPV6_FIB, true);
>> +err = devlink_resource_register(devlink, "fib", n,
>> +NSIM_RESOURCE_IPV6_FIB,
>> +NSIM_RESOURCE_IPV6,
>> +¶ms, &nsim_ipv6_fib_res_ops);
>> +if (err) {
>> +pr_err("Failed to register IPv6 FIB resource\n");
>> +return err;
>> +}
>> +
>> +n = nsim_fib_get_val(net, NSIM_RESOURCE_IPV6_FIB_RULES, true);
>> +err = devlink_resource_register(devlink, "fib-rules", n,
>> +NSIM_RESOURCE_IPV6_FIB_RULES,
>> +NSIM_RESOURCE_IPV6,
>> +¶ms, &nsim_ipv6_fib_rules_res_ops);
>> +if (err) {
>> +pr_err("Failed to register IPv6 FIB rules resource\n");
>> +return err;
>> +}
>> +out:
>> +return err;
>> +}
>
>> +void nsim_devlink_teardown(struct netdevsim *ns)
>> +{
>> +if (ns->devlink) {
>> +struct net *net = dev_net(ns->netdev);
>> +bool *reg_devlink = net_generic(net, nsim_devlink_id);
>
> nit: reverse xmas tree
reg_devlink uses net, so the order can not be changed
>
>> +devlink_unregister(ns->devlink)
Re: [PATCH RFC net-next 7/7] netdevsim: Add simple FIB resource controller via devlink
Fri, Mar 23, 2018 at 04:13:14PM CET, d...@cumulusnetworks.com wrote:
>On 3/23/18 9:05 AM, Jiri Pirko wrote:
>> Fri, Mar 23, 2018 at 04:03:40PM CET, d...@cumulusnetworks.com wrote:
>>> On 3/23/18 9:01 AM, Jiri Pirko wrote:
Fri, Mar 23, 2018 at 03:31:02PM CET, d...@cumulusnetworks.com wrote:
> On 3/23/18 12:50 AM, Jiri Pirko wrote:
>>> +void nsim_devlink_setup(struct netdevsim *ns)
>>> +{
>>> + struct net *net = dev_net(ns->netdev);
>>> + bool *reg_devlink = net_generic(net, nsim_devlink_id);
>>> + struct devlink *devlink;
>>> + int err = -ENOMEM;
>>> +
>>> + /* only one device per namespace controls devlink */
>>> + if (!*reg_devlink) {
>>> + ns->devlink = NULL;
>>> + return;
>>> + }
>>> +
>>> + devlink = devlink_alloc(&nsim_devlink_ops, 0);
>>> + if (!devlink)
>>> + return;
>>> +
>>> + devlink_net_set(devlink, net);
>>> + err = devlink_register(devlink, &ns->dev);
>>
>> This reg_devlink construct looks odd. Why don't you leave the devlink
>> instance in init_ns?
>
> It is a per-network namespace resource controller. Since struct devlink
Wait a second. What do you mean by "per-network namespace"? Devlink
instance is always associated with one physical device. Like an ASIC.
> has a net entry, the simplest design is to put it into the namespace of
> the controller. Without it, controlling resource sizes in namespace
> 'foobar' has to be done from init_net, which is just wrong.
>>>
>>> you need to look at how netdevsim creates a device per netdevice.
>>
>> That means one devlink instance for each netdevsim device, doesn't it?
>>
>
>yes.
Still not sure how to handle namespaces in devlink. Originally, I
thought it would be okay to leave all devlink instances in init_ns.
Because what happens if you move netdev to another namespace? Should the
devlink move as well? What if you have multiple ports, each in different
namespace. Can user move devlink instance to another namespace? Etc.
Re: [PATCH RFC net-next 7/7] netdevsim: Add simple FIB resource controller via devlink
On Thu, 22 Mar 2018 15:57:57 -0700, David Ahern wrote:
> From: David Ahern
>
> 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.
FWIW some nits from me blow.
> diff --git a/drivers/net/netdevsim/Makefile b/drivers/net/netdevsim/Makefile
> index 09388c06171d..449b2a1a1800 100644
> --- a/drivers/net/netdevsim/Makefile
> +++ b/drivers/net/netdevsim/Makefile
> @@ -9,3 +9,7 @@ ifeq ($(CONFIG_BPF_SYSCALL),y)
> netdevsim-objs += \
> bpf.o
> endif
> +
> +ifneq ($(CONFIG_NET_DEVLINK),)
Hm. Don't you need MAY_USE_DEVLINK dependency perhaps?
> +netdevsim-objs += devlink.o fib.o
> +endif
> diff --git a/drivers/net/netdevsim/devlink.c b/drivers/net/netdevsim/devlink.c
> new file mode 100644
> index ..d10558e1b022
> --- /dev/null
> +++ b/drivers/net/netdevsim/devlink.c
> +static int devlink_resources_register(struct devlink *devlink)
> +{
> + struct devlink_resource_size_params params = {
> + .size_max = (u64)-1,
> + .size_granularity = 1,
> + .unit = DEVLINK_RESOURCE_UNIT_ENTRY
> + };
> + struct net *net = devlink_net(devlink);
> + int err;
> + u64 n;
> +
> + /* Resources for IPv4 */
> + err = devlink_resource_register(devlink, "IPv4", (u64)-1,
> + NSIM_RESOURCE_IPV4,
> + DEVLINK_RESOURCE_ID_PARENT_TOP,
> + ¶ms, NULL);
> + if (err) {
> + pr_err("Failed to register IPv4 top resource\n");
> + goto out;
nit: why goto out here and return err everywhere else? If I was to
choose I'd rather see returns. goto X; X: return; is less
obviously correct IMHO. Besides labels should be called by the
action they perform/undo, so goto err_return? :S
> + }
> +
> + n = nsim_fib_get_val(net, NSIM_RESOURCE_IPV4_FIB, true);
> + err = devlink_resource_register(devlink, "fib", n,
> + NSIM_RESOURCE_IPV4_FIB,
> + NSIM_RESOURCE_IPV4,
> + ¶ms, &nsim_ipv4_fib_res_ops);
> + if (err) {
> + pr_err("Failed to register IPv4 FIB resource\n");
> + return err;
> + }
> +
> + n = nsim_fib_get_val(net, NSIM_RESOURCE_IPV4_FIB_RULES, true);
> + err = devlink_resource_register(devlink, "fib-rules", n,
> + NSIM_RESOURCE_IPV4_FIB_RULES,
> + NSIM_RESOURCE_IPV4,
> + ¶ms, &nsim_ipv4_fib_rules_res_ops);
> + if (err) {
> + pr_err("Failed to register IPv4 FIB rules resource\n");
> + return err;
> + }
> +
> + /* Resources for IPv6 */
> + err = devlink_resource_register(devlink, "IPv6", (u64)-1,
> + NSIM_RESOURCE_IPV6,
> + DEVLINK_RESOURCE_ID_PARENT_TOP,
> + ¶ms, NULL);
> + if (err) {
> + pr_err("Failed to register IPv6 top resource\n");
> + goto out;
> + }
> +
> + n = nsim_fib_get_val(net, NSIM_RESOURCE_IPV6_FIB, true);
> + err = devlink_resource_register(devlink, "fib", n,
> + NSIM_RESOURCE_IPV6_FIB,
> + NSIM_RESOURCE_IPV6,
> + ¶ms, &nsim_ipv6_fib_res_ops);
> + if (err) {
> + pr_err("Failed to register IPv6 FIB resource\n");
> + return err;
> + }
> +
> + n = nsim_fib_get_val(net, NSIM_RESOURCE_IPV6_FIB_RULES, true);
> + err = devlink_resource_register(devlink, "fib-rules", n,
> + NSIM_RESOURCE_IPV6_FIB_RULES,
> + NSIM_RESOURCE_IPV6,
> + ¶ms, &nsim_ipv6_fib_rules_res_ops);
> + if (err) {
> + pr_err("Failed to register IPv6 FIB rules resource\n");
> + return err;
> + }
> +out:
> + return err;
> +}
> +void nsim_devlink_teardown(struct netdevsim *ns)
> +{
> + if (ns->devlink) {
> + struct net *net = dev_net(ns->netdev);
> + bool *reg_devlink = net_generic(net, nsim_devlink_id);
nit: reverse xmas tree
> + devlink_unregister(ns->devlink);
> + devlink_free(ns->devlink);
> + ns->devlink = NULL;
> +
> + nsim_devlink_net_reset(net);
> + *reg_devlink = true;
> + }
> +}
> diff --git a/drivers/net/netdevsim/fib.c b/drivers/net/netdevsim/fib.c
> new file mode 100644
> index ..b77dcafc7158
> --- /dev/null
> +++ b/drivers/net/netdevsim/fib.c
> +static int nsim_fib_e
Re: [PATCH RFC net-next 7/7] netdevsim: Add simple FIB resource controller via devlink
On 3/23/18 9:05 AM, Jiri Pirko wrote:
> Fri, Mar 23, 2018 at 04:03:40PM CET, d...@cumulusnetworks.com wrote:
>> On 3/23/18 9:01 AM, Jiri Pirko wrote:
>>> Fri, Mar 23, 2018 at 03:31:02PM CET, d...@cumulusnetworks.com wrote:
On 3/23/18 12:50 AM, Jiri Pirko wrote:
>> +void nsim_devlink_setup(struct netdevsim *ns)
>> +{
>> +struct net *net = dev_net(ns->netdev);
>> +bool *reg_devlink = net_generic(net, nsim_devlink_id);
>> +struct devlink *devlink;
>> +int err = -ENOMEM;
>> +
>> +/* only one device per namespace controls devlink */
>> +if (!*reg_devlink) {
>> +ns->devlink = NULL;
>> +return;
>> +}
>> +
>> +devlink = devlink_alloc(&nsim_devlink_ops, 0);
>> +if (!devlink)
>> +return;
>> +
>> +devlink_net_set(devlink, net);
>> +err = devlink_register(devlink, &ns->dev);
>
> This reg_devlink construct looks odd. Why don't you leave the devlink
> instance in init_ns?
It is a per-network namespace resource controller. Since struct devlink
>>>
>>> Wait a second. What do you mean by "per-network namespace"? Devlink
>>> instance is always associated with one physical device. Like an ASIC.
>>>
>>>
has a net entry, the simplest design is to put it into the namespace of
the controller. Without it, controlling resource sizes in namespace
'foobar' has to be done from init_net, which is just wrong.
>>
>> you need to look at how netdevsim creates a device per netdevice.
>
> That means one devlink instance for each netdevsim device, doesn't it?
>
yes.
Re: [PATCH RFC net-next 7/7] netdevsim: Add simple FIB resource controller via devlink
Fri, Mar 23, 2018 at 04:03:40PM CET, d...@cumulusnetworks.com wrote:
>On 3/23/18 9:01 AM, Jiri Pirko wrote:
>> Fri, Mar 23, 2018 at 03:31:02PM CET, d...@cumulusnetworks.com wrote:
>>> On 3/23/18 12:50 AM, Jiri Pirko wrote:
> +void nsim_devlink_setup(struct netdevsim *ns)
> +{
> + struct net *net = dev_net(ns->netdev);
> + bool *reg_devlink = net_generic(net, nsim_devlink_id);
> + struct devlink *devlink;
> + int err = -ENOMEM;
> +
> + /* only one device per namespace controls devlink */
> + if (!*reg_devlink) {
> + ns->devlink = NULL;
> + return;
> + }
> +
> + devlink = devlink_alloc(&nsim_devlink_ops, 0);
> + if (!devlink)
> + return;
> +
> + devlink_net_set(devlink, net);
> + err = devlink_register(devlink, &ns->dev);
This reg_devlink construct looks odd. Why don't you leave the devlink
instance in init_ns?
>>>
>>> It is a per-network namespace resource controller. Since struct devlink
>>
>> Wait a second. What do you mean by "per-network namespace"? Devlink
>> instance is always associated with one physical device. Like an ASIC.
>>
>>
>>> has a net entry, the simplest design is to put it into the namespace of
>>> the controller. Without it, controlling resource sizes in namespace
>>> 'foobar' has to be done from init_net, which is just wrong.
>
>you need to look at how netdevsim creates a device per netdevice.
That means one devlink instance for each netdevsim device, doesn't it?
Re: [PATCH RFC net-next 7/7] netdevsim: Add simple FIB resource controller via devlink
On 3/23/18 9:01 AM, Jiri Pirko wrote:
> Fri, Mar 23, 2018 at 03:31:02PM CET, d...@cumulusnetworks.com wrote:
>> On 3/23/18 12:50 AM, Jiri Pirko wrote:
+void nsim_devlink_setup(struct netdevsim *ns)
+{
+ struct net *net = dev_net(ns->netdev);
+ bool *reg_devlink = net_generic(net, nsim_devlink_id);
+ struct devlink *devlink;
+ int err = -ENOMEM;
+
+ /* only one device per namespace controls devlink */
+ if (!*reg_devlink) {
+ ns->devlink = NULL;
+ return;
+ }
+
+ devlink = devlink_alloc(&nsim_devlink_ops, 0);
+ if (!devlink)
+ return;
+
+ devlink_net_set(devlink, net);
+ err = devlink_register(devlink, &ns->dev);
>>>
>>> This reg_devlink construct looks odd. Why don't you leave the devlink
>>> instance in init_ns?
>>
>> It is a per-network namespace resource controller. Since struct devlink
>
> Wait a second. What do you mean by "per-network namespace"? Devlink
> instance is always associated with one physical device. Like an ASIC.
>
>
>> has a net entry, the simplest design is to put it into the namespace of
>> the controller. Without it, controlling resource sizes in namespace
>> 'foobar' has to be done from init_net, which is just wrong.
you need to look at how netdevsim creates a device per netdevice.
Re: [PATCH RFC net-next 7/7] netdevsim: Add simple FIB resource controller via devlink
Fri, Mar 23, 2018 at 03:31:02PM CET, d...@cumulusnetworks.com wrote:
>On 3/23/18 12:50 AM, Jiri Pirko wrote:
>>> +void nsim_devlink_setup(struct netdevsim *ns)
>>> +{
>>> + struct net *net = dev_net(ns->netdev);
>>> + bool *reg_devlink = net_generic(net, nsim_devlink_id);
>>> + struct devlink *devlink;
>>> + int err = -ENOMEM;
>>> +
>>> + /* only one device per namespace controls devlink */
>>> + if (!*reg_devlink) {
>>> + ns->devlink = NULL;
>>> + return;
>>> + }
>>> +
>>> + devlink = devlink_alloc(&nsim_devlink_ops, 0);
>>> + if (!devlink)
>>> + return;
>>> +
>>> + devlink_net_set(devlink, net);
>>> + err = devlink_register(devlink, &ns->dev);
>>
>> This reg_devlink construct looks odd. Why don't you leave the devlink
>> instance in init_ns?
>
>It is a per-network namespace resource controller. Since struct devlink
Wait a second. What do you mean by "per-network namespace"? Devlink
instance is always associated with one physical device. Like an ASIC.
>has a net entry, the simplest design is to put it into the namespace of
>the controller. Without it, controlling resource sizes in namespace
>'foobar' has to be done from init_net, which is just wrong.
Re: [PATCH RFC net-next 7/7] netdevsim: Add simple FIB resource controller via devlink
On 3/23/18 12:50 AM, Jiri Pirko wrote:
>> +void nsim_devlink_setup(struct netdevsim *ns)
>> +{
>> +struct net *net = dev_net(ns->netdev);
>> +bool *reg_devlink = net_generic(net, nsim_devlink_id);
>> +struct devlink *devlink;
>> +int err = -ENOMEM;
>> +
>> +/* only one device per namespace controls devlink */
>> +if (!*reg_devlink) {
>> +ns->devlink = NULL;
>> +return;
>> +}
>> +
>> +devlink = devlink_alloc(&nsim_devlink_ops, 0);
>> +if (!devlink)
>> +return;
>> +
>> +devlink_net_set(devlink, net);
>> +err = devlink_register(devlink, &ns->dev);
>
> This reg_devlink construct looks odd. Why don't you leave the devlink
> instance in init_ns?
It is a per-network namespace resource controller. Since struct devlink
has a net entry, the simplest design is to put it into the namespace of
the controller. Without it, controlling resource sizes in namespace
'foobar' has to be done from init_net, which is just wrong.
Re: [PATCH RFC net-next 7/7] netdevsim: Add simple FIB resource controller via devlink
Thu, Mar 22, 2018 at 11:57:57PM CET, d...@cumulusnetworks.com wrote:
>From: David Ahern
[...]
>+void nsim_devlink_teardown(struct netdevsim *ns)
>+{
>+ if (ns->devlink) {
>+ struct net *net = dev_net(ns->netdev);
>+ bool *reg_devlink = net_generic(net, nsim_devlink_id);
>+
>+ devlink_unregister(ns->devlink);
>+ devlink_free(ns->devlink);
>+ ns->devlink = NULL;
>+
>+ nsim_devlink_net_reset(net);
>+ *reg_devlink = true;
>+ }
>+}
>+
>+void nsim_devlink_setup(struct netdevsim *ns)
>+{
>+ struct net *net = dev_net(ns->netdev);
>+ bool *reg_devlink = net_generic(net, nsim_devlink_id);
>+ struct devlink *devlink;
>+ int err = -ENOMEM;
>+
>+ /* only one device per namespace controls devlink */
>+ if (!*reg_devlink) {
>+ ns->devlink = NULL;
>+ return;
>+ }
>+
>+ devlink = devlink_alloc(&nsim_devlink_ops, 0);
>+ if (!devlink)
>+ return;
>+
>+ devlink_net_set(devlink, net);
>+ err = devlink_register(devlink, &ns->dev);
This reg_devlink construct looks odd. Why don't you leave the devlink
instance in init_ns?
>+ if (err)
>+ goto err_devlink_free;
>+
>+ err = devlink_resources_register(devlink);
>+ if (err)
>+ goto err_dl_unregister;
>+
>+ ns->devlink = devlink;
>+
>+ *reg_devlink = false;
>+
>+ return;
>+
>+err_dl_unregister:
>+ devlink_unregister(devlink);
>+err_devlink_free:
>+ devlink_free(devlink);
>+}
>+
>+/* Initialize per network namespace state */
>+static int __net_init nsim_devlink_netns_init(struct net *net)
>+{
>+ bool *reg_devlink = net_generic(net, nsim_devlink_id);
>+
>+ *reg_devlink = true;
>+
>+ return 0;
>+}
>+
>+static struct pernet_operations nsim_devlink_net_ops __net_initdata = {
>+ .init = nsim_devlink_netns_init,
>+ .id = &nsim_devlink_id,
>+ .size = sizeof(bool),
>+ .async = true,
>+};
