| Message ID | 20190806191517.8713-1-dsahern@kernel.org |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net] netdevsim: Restore per-network namespace accounting for fib entries | expand |
On Tue, 6 Aug 2019 12:15:17 -0700, David Ahern wrote: > From: David Ahern <dsahern@gmail.com> > > Prior to the commit in the fixes tag, the resource controller in netdevsim > tracked fib entries and rules per network namespace. Restore that behavior. > > Fixes: 5fc494225c1e ("netdevsim: create devlink instance per netdevsim instance") > Signed-off-by: David Ahern <dsahern@gmail.com> Thanks. Let's see what Jiri says, but to me this patch seems to indeed restore the original per-namespace accounting when the more natural way forward may perhaps be to make nsim only count the fib entries where fib_info->net == devlink_net(devlink) > -void nsim_fib_destroy(struct nsim_fib_data *data) > +int nsim_fib_init(void) > { > - unregister_fib_notifier(&data->fib_nb); > - kfree(data); > + int err; > + > + err = register_pernet_subsys(&nsim_fib_net_ops); > + if (err < 0) { > + pr_err("Failed to register pernet subsystem\n"); > + goto err_out; > + } > + > + err = register_fib_notifier(&nsim_fib_nb, nsim_fib_dump_inconsistent); > + if (err < 0) { > + pr_err("Failed to register fib notifier\n"); unregister_pernet_subsys(&nsim_fib_net_ops); ? > + goto err_out; > + } > + > +err_out: > + return err; > }
Wed, Aug 07, 2019 at 12:32:14AM CEST, jakub.kicinski@netronome.com wrote: >On Tue, 6 Aug 2019 12:15:17 -0700, David Ahern wrote: >> From: David Ahern <dsahern@gmail.com> >> >> Prior to the commit in the fixes tag, the resource controller in netdevsim >> tracked fib entries and rules per network namespace. Restore that behavior. >> >> Fixes: 5fc494225c1e ("netdevsim: create devlink instance per netdevsim instance") >> Signed-off-by: David Ahern <dsahern@gmail.com> > >Thanks. > >Let's see what Jiri says, but to me this patch seems to indeed restore >the original per-namespace accounting when the more natural way forward >may perhaps be to make nsim only count the fib entries where I think that: 1) netdevsim is a glorified dummy device for testing kernel api, not for configuring per-namespace resource limitation. 2) If the conclusion os to use devlink instead of cgroups for resourse limitations, it should be done in a separate code, not in netdevsim. I would definitelly want to wait what is the result of discussion around 2) first. But one way or another netdevsim code should not do this, I would like to cutout the fib limitations from it instead, just to expose the setup resource limits through debugfs like the rest of the configuration of netdevsim. > > fib_info->net == devlink_net(devlink) > >> -void nsim_fib_destroy(struct nsim_fib_data *data) >> +int nsim_fib_init(void) >> { >> - unregister_fib_notifier(&data->fib_nb); >> - kfree(data); >> + int err; >> + >> + err = register_pernet_subsys(&nsim_fib_net_ops); >> + if (err < 0) { >> + pr_err("Failed to register pernet subsystem\n"); >> + goto err_out; >> + } >> + >> + err = register_fib_notifier(&nsim_fib_nb, nsim_fib_dump_inconsistent); >> + if (err < 0) { >> + pr_err("Failed to register fib notifier\n"); > > unregister_pernet_subsys(&nsim_fib_net_ops); >? > >> + goto err_out; >> + } >> + >> +err_out: >> + return err; >> }
On 8/7/19 12:27 AM, Jiri Pirko wrote: > Wed, Aug 07, 2019 at 12:32:14AM CEST, jakub.kicinski@netronome.com wrote: >> On Tue, 6 Aug 2019 12:15:17 -0700, David Ahern wrote: >>> From: David Ahern <dsahern@gmail.com> >>> >>> Prior to the commit in the fixes tag, the resource controller in netdevsim >>> tracked fib entries and rules per network namespace. Restore that behavior. >>> >>> Fixes: 5fc494225c1e ("netdevsim: create devlink instance per netdevsim instance") >>> Signed-off-by: David Ahern <dsahern@gmail.com> >> >> Thanks. >> >> Let's see what Jiri says, but to me this patch seems to indeed restore >> the original per-namespace accounting when the more natural way forward >> may perhaps be to make nsim only count the fib entries where > > I think that: > 1) netdevsim is a glorified dummy device for testing kernel api, not for > configuring per-namespace resource limitation. > 2) If the conclusion os to use devlink instead of cgroups for resourse > limitations, it should be done in a separate code, not in netdevsim. > > I would definitelly want to wait what is the result of discussion around 2) > first. But one way or another netdevsim code should not do this, I would > like to cutout the fib limitations from it instead, just to expose the > setup resource limits through debugfs like the rest of the configuration > of netdevsim. > > This is the most incredulous response. You break code you don't understand and argue that it should be left broken in older releases and ripped out in later ones rather than fixed back to its intent. Again, the devlink resource controller was added by me specifically to test the ability to fail in-kernel fib notifiers. When I add the nexthop in-kernel notifier, netdevsim again provides a simple means of testing it. It satisfies all of the conditions and intents of netdevsim - test kernel APIs without hardware.
Wed, Aug 07, 2019 at 02:39:56PM CEST, dsahern@gmail.com wrote: >On 8/7/19 12:27 AM, Jiri Pirko wrote: >> Wed, Aug 07, 2019 at 12:32:14AM CEST, jakub.kicinski@netronome.com wrote: >>> On Tue, 6 Aug 2019 12:15:17 -0700, David Ahern wrote: >>>> From: David Ahern <dsahern@gmail.com> >>>> >>>> Prior to the commit in the fixes tag, the resource controller in netdevsim >>>> tracked fib entries and rules per network namespace. Restore that behavior. >>>> >>>> Fixes: 5fc494225c1e ("netdevsim: create devlink instance per netdevsim instance") >>>> Signed-off-by: David Ahern <dsahern@gmail.com> >>> >>> Thanks. >>> >>> Let's see what Jiri says, but to me this patch seems to indeed restore >>> the original per-namespace accounting when the more natural way forward >>> may perhaps be to make nsim only count the fib entries where >> >> I think that: >> 1) netdevsim is a glorified dummy device for testing kernel api, not for >> configuring per-namespace resource limitation. >> 2) If the conclusion os to use devlink instead of cgroups for resourse >> limitations, it should be done in a separate code, not in netdevsim. >> >> I would definitelly want to wait what is the result of discussion around 2) >> first. But one way or another netdevsim code should not do this, I would >> like to cutout the fib limitations from it instead, just to expose the >> setup resource limits through debugfs like the rest of the configuration >> of netdevsim. >> >> > >This is the most incredulous response. You break code you don't >understand and argue that it should be left broken in older releases and >ripped out in later ones rather than fixed back to its intent. Yeah. I believe it was a mistake to add it in the first place. Abuses netdevsim for something it is not. I'm fine to use devlink the way you want to after we conclude 2), but outside netdevsim. Again, netdevsim is there for config api testing purposes. If things got broken, it is not that bit deal. I broke the way it is instantiated significantly for example (iplink->sysfs). > >Again, the devlink resource controller was added by me specifically to >test the ability to fail in-kernel fib notifiers. When I add the nexthop >in-kernel notifier, netdevsim again provides a simple means of testing it. > >It satisfies all of the conditions and intents of netdevsim - test >kernel APIs without hardware. Well, what you want to do is limit kernel sw resources per-namespace.
On 8/7/19 7:07 AM, Jiri Pirko wrote: > > Yeah. I believe it was a mistake to add it in the first place. Abuses > netdevsim for something it is not. I'm fine to use devlink the way you > want to after we conclude 2), but outside netdevsim. > > Again, netdevsim is there for config api testing purposes. If things > got broken, it is not that bit deal. I broke the way it is > instantiated significantly for example (iplink->sysfs). > netdevsim was created as a way of testing hardware focused kernel APIs and code paths without hardware. yes? The devlink api was added to netdevsim to test fib notifiers failing by handlers. The notifiers were added for mlxsw - a hardware driver - as a way to get notifications of fib changes. The easiest way to test the error paths was to code limits to fib entries very similar to what mlxsw implements with its 'devlink resource' implementation. ie., to implement 'devlink resource' for a software emulated device. netdevsim was the most logical place for it. See the commit logs starting at: commit b349e0b5ec5d7be57ac243fb08ae8b994c928165 Merge: 6e2135ce54b7 37923ed6b8ce Author: David S. Miller <davem@davemloft.net> Date: Thu Mar 29 14:10:31 2018 -0400 Merge branch 'net-Allow-FIB-notifiers-to-fail-add-and-replace' David Ahern says: ==================== net: Allow FIB notifiers to fail add and replace Everything about that implementation is using the s/w device to test code paths targeted at hardware offloads.
From: David Ahern <dsahern@kernel.org> Date: Tue, 6 Aug 2019 12:15:17 -0700 > From: David Ahern <dsahern@gmail.com> > > Prior to the commit in the fixes tag, the resource controller in netdevsim > tracked fib entries and rules per network namespace. Restore that behavior. > > Fixes: 5fc494225c1e ("netdevsim: create devlink instance per netdevsim instance") > Signed-off-by: David Ahern <dsahern@gmail.com> Applied, thanks for bringing this to our attention and fixing it David. Jiri, I disagree you on every single possible level. If you didn't like how netdevsim worked in this area the opportunity to do something about it was way back when it went in. No matter how completely busted or disagreeable an interface is, once we have committed it to a release (and in particular people are knowingly using and depending upon it) you cannot break it. It doesn't matter how much you disagree with something, you cannot break it when it's out there and actively in use. Do you have any idea how much stuff I'd like to break because I think the design turned out to be completely wrong? But I can't.
Mon, Aug 12, 2019 at 06:02:18AM CEST, davem@davemloft.net wrote: >From: David Ahern <dsahern@kernel.org> >Date: Tue, 6 Aug 2019 12:15:17 -0700 > >> From: David Ahern <dsahern@gmail.com> >> >> Prior to the commit in the fixes tag, the resource controller in netdevsim >> tracked fib entries and rules per network namespace. Restore that behavior. >> >> Fixes: 5fc494225c1e ("netdevsim: create devlink instance per netdevsim instance") >> Signed-off-by: David Ahern <dsahern@gmail.com> > >Applied, thanks for bringing this to our attention and fixing it David. > >Jiri, I disagree you on every single possible level. > >If you didn't like how netdevsim worked in this area the opportunity to do >something about it was way back when it went in. Yeah, I expressed my feelings back then. But that didn't help :( > >No matter how completely busted or disagreeable an interface is, once we have >committed it to a release (and in particular people are knowingly using and >depending upon it) you cannot break it. I understand it with real devices, but dummy testing device, who's purpose is just to test API. Why? > >It doesn't matter how much you disagree with something, you cannot break it >when it's out there and actively in use. > >Do you have any idea how much stuff I'd like to break because I think the >design turned out to be completely wrong? But I can't. Sure, me too :) But that is for real devices. That is a different story as I see it. Apparently, I'm wrong...
From: Jiri Pirko <jiri@resnulli.us> Date: Mon, 12 Aug 2019 10:36:35 +0200 > I understand it with real devices, but dummy testing device, who's > purpose is just to test API. Why? Because you'll break all of the wonderful testing infrastructure people like David have created.
Mon, Aug 12, 2019 at 05:28:02PM CEST, davem@davemloft.net wrote: >From: Jiri Pirko <jiri@resnulli.us> >Date: Mon, 12 Aug 2019 10:36:35 +0200 > >> I understand it with real devices, but dummy testing device, who's >> purpose is just to test API. Why? > >Because you'll break all of the wonderful testing infrastructure >people like David have created. Are you referring to selftests? There is no such test there :( But if it would be, could implement the limitations properly (like using cgroups), change the tests and remove this code from netdevsim?
On 8/13/19 1:14 AM, Jiri Pirko wrote: > Mon, Aug 12, 2019 at 05:28:02PM CEST, davem@davemloft.net wrote: >> From: Jiri Pirko <jiri@resnulli.us> >> Date: Mon, 12 Aug 2019 10:36:35 +0200 >> >>> I understand it with real devices, but dummy testing device, who's >>> purpose is just to test API. Why? >> >> Because you'll break all of the wonderful testing infrastructure >> people like David have created. > > Are you referring to selftests? There is no such test there :( I have one now and will be submitting it after net merges with net-next. > But if it would be, could implement the limitations > properly (like using cgroups), change the tests and remove this > code from netdevsim? The intent of this code and test is to have a s/w model similar to how mlxsw works - responding to notifiers and deciding to reject a change. You are currently adding (or trying to) more devlink based s/w tests, so you must see the value of netdevsim as a source of testing.
Tue, Aug 13, 2019 at 04:41:18PM CEST, dsahern@gmail.com wrote: >On 8/13/19 1:14 AM, Jiri Pirko wrote: >> Mon, Aug 12, 2019 at 05:28:02PM CEST, davem@davemloft.net wrote: >>> From: Jiri Pirko <jiri@resnulli.us> >>> Date: Mon, 12 Aug 2019 10:36:35 +0200 >>> >>>> I understand it with real devices, but dummy testing device, who's >>>> purpose is just to test API. Why? >>> >>> Because you'll break all of the wonderful testing infrastructure >>> people like David have created. >> >> Are you referring to selftests? There is no such test there :( > >I have one now and will be submitting it after net merges with net-next. > >> But if it would be, could implement the limitations >> properly (like using cgroups), change the tests and remove this >> code from netdevsim? > >The intent of this code and test is to have a s/w model similar to how >mlxsw works - responding to notifiers and deciding to reject a change. >You are currently adding (or trying to) more devlink based s/w tests, so >you must see the value of netdevsim as a source of testing. Sure I do. Not sure makes sence to repeat myself again, but why not: The way you use netdevsim with netnamespace limitation is nothing like it is done in hardware. Devlink resources should limit the resources of the device, not network namespace. You abused netdevsim and devlink for that. Not cool :( To be in sync with mlxsw, netdevsim should track fibs added to the ports and apply the resource limitations to that. That is the correct behaviour. Exacly like mlxsw does. Frankly I don't really understand why you keep pushing your broken design. Why the limitation applied only for fibs related to netdevsim ports is not enough for testing??? Would that work for you? Please? This is keeping me awake at night. Sigh :(
From: Jiri Pirko <jiri@resnulli.us> Date: Tue, 13 Aug 2019 09:14:45 +0200 > Mon, Aug 12, 2019 at 05:28:02PM CEST, davem@davemloft.net wrote: >>From: Jiri Pirko <jiri@resnulli.us> >>Date: Mon, 12 Aug 2019 10:36:35 +0200 >> >>> I understand it with real devices, but dummy testing device, who's >>> purpose is just to test API. Why? >> >>Because you'll break all of the wonderful testing infrastructure >>people like David have created. > > Are you referring to selftests? There is no such test there :( > But if it would be, could implement the limitations > properly (like using cgroups), change the tests and remove this > code from netdevsim? What about older kernels? Can't you see how illogical this is?
Tue, Aug 13, 2019 at 07:40:54PM CEST, davem@davemloft.net wrote: >From: Jiri Pirko <jiri@resnulli.us> >Date: Tue, 13 Aug 2019 09:14:45 +0200 > >> Mon, Aug 12, 2019 at 05:28:02PM CEST, davem@davemloft.net wrote: >>>From: Jiri Pirko <jiri@resnulli.us> >>>Date: Mon, 12 Aug 2019 10:36:35 +0200 >>> >>>> I understand it with real devices, but dummy testing device, who's >>>> purpose is just to test API. Why? >>> >>>Because you'll break all of the wonderful testing infrastructure >>>people like David have created. >> >> Are you referring to selftests? There is no such test there :( >> But if it would be, could implement the limitations >> properly (like using cgroups), change the tests and remove this >> code from netdevsim? > >What about older kernels? Can't you see how illogical this is? Not really, since this is a dummy testing device. Not like we would break anybody. Well, I changed instantiation of netdevsim from rtnetlink to sysfs, that is even bigger breakage, nobody cared (of course). Well sure we can comeup with netdevsim2, netdevsimN, to maintain backward compatibility of netdevsim. I find it ridiculous. But anyway, I don't really care that much. I resign as this seems futile :(
Tue, Aug 06, 2019 at 09:15:17PM CEST, dsahern@kernel.org wrote: >From: David Ahern <dsahern@gmail.com> > >Prior to the commit in the fixes tag, the resource controller in netdevsim >tracked fib entries and rules per network namespace. Restore that behavior. David, please help me understand. If the counters are per-device, not per-netns, they are both the same. If we have device (devlink instance) is in a netns and take only things happening in this netns into account, it should count exactly the same amount of fib entries, doesn't it? I re-thinked the devlink netns patchset and currently I'm going in slightly different direction. I'm having netns as an attribute of devlink reload. So all the port netdevices and everything gets re-instantiated into new netns. Works fine with mlxsw. There we also re-register the fib notifier. I think that this can work for your usecase in netdevsim too: 1) devlink instance is registering a fib notifier to track all fib entries in a namespace it belongs to. The counters are per-device - counting fib entries in a namespace the device is in. 2) another devlink instance can do the same tracking in the same namespace. No problem, it's a separate counter, but the numbers are the same. One can set different limits to different devlink instances, but you can have only one. That is the bahaviour you have now. 3) on devlink reload, netdevsim re-instantiates ports and re-registers fib notifier 4) on devlink reload with netns change, all should be fine as the re-registered fib nofitier replays the entries. The ports are re-instatiated in new netns. This way, we would get consistent behaviour between netdevsim and real devices (mlxsw), correct devlink-netns implementation (you also suggested to move ports to the namespace). Everyone should be happy. What do you think?
On 8/28/19 4:37 AM, Jiri Pirko wrote: > Tue, Aug 06, 2019 at 09:15:17PM CEST, dsahern@kernel.org wrote: >> From: David Ahern <dsahern@gmail.com> >> >> Prior to the commit in the fixes tag, the resource controller in netdevsim >> tracked fib entries and rules per network namespace. Restore that behavior. > > David, please help me understand. If the counters are per-device, not > per-netns, they are both the same. If we have device (devlink instance) > is in a netns and take only things happening in this netns into account, > it should count exactly the same amount of fib entries, doesn't it? if you are only changing where the counters are stored - net_generic vs devlink private - then yes, they should be equivalent. > > I re-thinked the devlink netns patchset and currently I'm going in > slightly different direction. I'm having netns as an attribute of > devlink reload. So all the port netdevices and everything gets > re-instantiated into new netns. Works fine with mlxsw. There we also > re-register the fib notifier. > > I think that this can work for your usecase in netdevsim too: > 1) devlink instance is registering a fib notifier to track all fib > entries in a namespace it belongs to. The counters are per-device - > counting fib entries in a namespace the device is in. > 2) another devlink instance can do the same tracking in the same > namespace. No problem, it's a separate counter, but the numbers are > the same. One can set different limits to different devlink > instances, but you can have only one. That is the bahaviour you have > now. > 3) on devlink reload, netdevsim re-instantiates ports and re-registers > fib notifier > 4) on devlink reload with netns change, all should be fine as the > re-registered fib nofitier replays the entries. The ports are > re-instatiated in new netns. > > This way, we would get consistent behaviour between netdevsim and real > devices (mlxsw), correct devlink-netns implementation (you also > suggested to move ports to the namespace). Everyone should be happy. > > What do you think? > Right now, registering the fib notifier walks all namespaces. That is not a scalable solution. Are you changing that to replay only a given netns? Are you changing the notifiers to be per-namespace? Also, you are still allowing devlink instances to be created within a namespace?
Wed, Aug 28, 2019 at 11:26:03PM CEST, dsahern@gmail.com wrote: >On 8/28/19 4:37 AM, Jiri Pirko wrote: >> Tue, Aug 06, 2019 at 09:15:17PM CEST, dsahern@kernel.org wrote: >>> From: David Ahern <dsahern@gmail.com> >>> >>> Prior to the commit in the fixes tag, the resource controller in netdevsim >>> tracked fib entries and rules per network namespace. Restore that behavior. >> >> David, please help me understand. If the counters are per-device, not >> per-netns, they are both the same. If we have device (devlink instance) >> is in a netns and take only things happening in this netns into account, >> it should count exactly the same amount of fib entries, doesn't it? > >if you are only changing where the counters are stored - net_generic vs >devlink private - then yes, they should be equivalent. Okay. > >> >> I re-thinked the devlink netns patchset and currently I'm going in >> slightly different direction. I'm having netns as an attribute of >> devlink reload. So all the port netdevices and everything gets >> re-instantiated into new netns. Works fine with mlxsw. There we also >> re-register the fib notifier. >> >> I think that this can work for your usecase in netdevsim too: >> 1) devlink instance is registering a fib notifier to track all fib >> entries in a namespace it belongs to. The counters are per-device - >> counting fib entries in a namespace the device is in. >> 2) another devlink instance can do the same tracking in the same >> namespace. No problem, it's a separate counter, but the numbers are >> the same. One can set different limits to different devlink >> instances, but you can have only one. That is the bahaviour you have >> now. >> 3) on devlink reload, netdevsim re-instantiates ports and re-registers >> fib notifier >> 4) on devlink reload with netns change, all should be fine as the >> re-registered fib nofitier replays the entries. The ports are >> re-instatiated in new netns. >> >> This way, we would get consistent behaviour between netdevsim and real >> devices (mlxsw), correct devlink-netns implementation (you also >> suggested to move ports to the namespace). Everyone should be happy. >> >> What do you think? >> > >Right now, registering the fib notifier walks all namespaces. That is >not a scalable solution. Are you changing that to replay only a given >netns? Are you changing the notifiers to be per-namespace? Eventually, that seems like good idea. Currently I want to do if (net==nsim_dev->mynet) done check at the beginning of the notifier. > >Also, you are still allowing devlink instances to be created within a >namespace? Yes, netdevsim is planned to be created directly in namespace.
On 8/29/19 12:28 AM, Jiri Pirko wrote: > Wed, Aug 28, 2019 at 11:26:03PM CEST, dsahern@gmail.com wrote: >> On 8/28/19 4:37 AM, Jiri Pirko wrote: >>> Tue, Aug 06, 2019 at 09:15:17PM CEST, dsahern@kernel.org wrote: >>>> From: David Ahern <dsahern@gmail.com> >>>> >>>> Prior to the commit in the fixes tag, the resource controller in netdevsim >>>> tracked fib entries and rules per network namespace. Restore that behavior. >>> >>> David, please help me understand. If the counters are per-device, not >>> per-netns, they are both the same. If we have device (devlink instance) >>> is in a netns and take only things happening in this netns into account, >>> it should count exactly the same amount of fib entries, doesn't it? >> >> if you are only changing where the counters are stored - net_generic vs >> devlink private - then yes, they should be equivalent. > > Okay. > >> >>> >>> I re-thinked the devlink netns patchset and currently I'm going in >>> slightly different direction. I'm having netns as an attribute of >>> devlink reload. So all the port netdevices and everything gets >>> re-instantiated into new netns. Works fine with mlxsw. There we also >>> re-register the fib notifier. >>> >>> I think that this can work for your usecase in netdevsim too: >>> 1) devlink instance is registering a fib notifier to track all fib >>> entries in a namespace it belongs to. The counters are per-device - >>> counting fib entries in a namespace the device is in. >>> 2) another devlink instance can do the same tracking in the same >>> namespace. No problem, it's a separate counter, but the numbers are >>> the same. One can set different limits to different devlink >>> instances, but you can have only one. That is the bahaviour you have >>> now. >>> 3) on devlink reload, netdevsim re-instantiates ports and re-registers >>> fib notifier >>> 4) on devlink reload with netns change, all should be fine as the >>> re-registered fib nofitier replays the entries. The ports are >>> re-instatiated in new netns. >>> >>> This way, we would get consistent behaviour between netdevsim and real >>> devices (mlxsw), correct devlink-netns implementation (you also >>> suggested to move ports to the namespace). Everyone should be happy. >>> >>> What do you think? >>> >> >> Right now, registering the fib notifier walks all namespaces. That is >> not a scalable solution. Are you changing that to replay only a given >> netns? Are you changing the notifiers to be per-namespace? > > Eventually, that seems like good idea. Currently I want to do > if (net==nsim_dev->mynet) > done > check at the beginning of the notifier. > The per-namespace replay should be done as part of this re-work. It should not be that big of a change. Add 'struct net' arg to register_fib_notifier. If set, call fib_net_dump only for that namespace. The seq check should be made per-namespace. You mentioned mlxsw works fine with moving ports to a new network namespace, so that will be a 'real' example with a known scalability problem that should be addressed now.
Thu, Aug 29, 2019 at 02:54:39PM CEST, dsahern@gmail.com wrote: >On 8/29/19 12:28 AM, Jiri Pirko wrote: >> Wed, Aug 28, 2019 at 11:26:03PM CEST, dsahern@gmail.com wrote: >>> On 8/28/19 4:37 AM, Jiri Pirko wrote: >>>> Tue, Aug 06, 2019 at 09:15:17PM CEST, dsahern@kernel.org wrote: >>>>> From: David Ahern <dsahern@gmail.com> >>>>> >>>>> Prior to the commit in the fixes tag, the resource controller in netdevsim >>>>> tracked fib entries and rules per network namespace. Restore that behavior. >>>> >>>> David, please help me understand. If the counters are per-device, not >>>> per-netns, they are both the same. If we have device (devlink instance) >>>> is in a netns and take only things happening in this netns into account, >>>> it should count exactly the same amount of fib entries, doesn't it? >>> >>> if you are only changing where the counters are stored - net_generic vs >>> devlink private - then yes, they should be equivalent. >> >> Okay. >> >>> >>>> >>>> I re-thinked the devlink netns patchset and currently I'm going in >>>> slightly different direction. I'm having netns as an attribute of >>>> devlink reload. So all the port netdevices and everything gets >>>> re-instantiated into new netns. Works fine with mlxsw. There we also >>>> re-register the fib notifier. >>>> >>>> I think that this can work for your usecase in netdevsim too: >>>> 1) devlink instance is registering a fib notifier to track all fib >>>> entries in a namespace it belongs to. The counters are per-device - >>>> counting fib entries in a namespace the device is in. >>>> 2) another devlink instance can do the same tracking in the same >>>> namespace. No problem, it's a separate counter, but the numbers are >>>> the same. One can set different limits to different devlink >>>> instances, but you can have only one. That is the bahaviour you have >>>> now. >>>> 3) on devlink reload, netdevsim re-instantiates ports and re-registers >>>> fib notifier >>>> 4) on devlink reload with netns change, all should be fine as the >>>> re-registered fib nofitier replays the entries. The ports are >>>> re-instatiated in new netns. >>>> >>>> This way, we would get consistent behaviour between netdevsim and real >>>> devices (mlxsw), correct devlink-netns implementation (you also >>>> suggested to move ports to the namespace). Everyone should be happy. >>>> >>>> What do you think? >>>> >>> >>> Right now, registering the fib notifier walks all namespaces. That is >>> not a scalable solution. Are you changing that to replay only a given >>> netns? Are you changing the notifiers to be per-namespace? >> >> Eventually, that seems like good idea. Currently I want to do >> if (net==nsim_dev->mynet) >> done >> check at the beginning of the notifier. >> > >The per-namespace replay should be done as part of this re-work. It >should not be that big of a change. Add 'struct net' arg to >register_fib_notifier. If set, call fib_net_dump only for that >namespace. The seq check should be made per-namespace. > >You mentioned mlxsw works fine with moving ports to a new network >namespace, so that will be a 'real' example with a known scalability >problem that should be addressed now. Fair enough. Will include this now. Thanks!
diff --git a/drivers/net/netdevsim/dev.c b/drivers/net/netdevsim/dev.c index c5c417a3c0ce..bcc40a236624 100644 --- a/drivers/net/netdevsim/dev.c +++ b/drivers/net/netdevsim/dev.c @@ -73,46 +73,47 @@ static void nsim_dev_port_debugfs_exit(struct nsim_dev_port *nsim_dev_port) debugfs_remove_recursive(nsim_dev_port->ddir); } +static struct net *nsim_devlink_net(struct devlink *devlink) +{ + return &init_net; +} + static u64 nsim_dev_ipv4_fib_resource_occ_get(void *priv) { - struct nsim_dev *nsim_dev = priv; + struct net *net = priv; - return nsim_fib_get_val(nsim_dev->fib_data, - NSIM_RESOURCE_IPV4_FIB, false); + return nsim_fib_get_val(net, NSIM_RESOURCE_IPV4_FIB, false); } static u64 nsim_dev_ipv4_fib_rules_res_occ_get(void *priv) { - struct nsim_dev *nsim_dev = priv; + struct net *net = priv; - return nsim_fib_get_val(nsim_dev->fib_data, - NSIM_RESOURCE_IPV4_FIB_RULES, false); + return nsim_fib_get_val(net, NSIM_RESOURCE_IPV4_FIB_RULES, false); } static u64 nsim_dev_ipv6_fib_resource_occ_get(void *priv) { - struct nsim_dev *nsim_dev = priv; + struct net *net = priv; - return nsim_fib_get_val(nsim_dev->fib_data, - NSIM_RESOURCE_IPV6_FIB, false); + return nsim_fib_get_val(net, NSIM_RESOURCE_IPV6_FIB, false); } static u64 nsim_dev_ipv6_fib_rules_res_occ_get(void *priv) { - struct nsim_dev *nsim_dev = priv; + struct net *net = priv; - return nsim_fib_get_val(nsim_dev->fib_data, - NSIM_RESOURCE_IPV6_FIB_RULES, false); + return nsim_fib_get_val(net, NSIM_RESOURCE_IPV6_FIB_RULES, false); } static int nsim_dev_resources_register(struct devlink *devlink) { - struct nsim_dev *nsim_dev = devlink_priv(devlink); struct devlink_resource_size_params params = { .size_max = (u64)-1, .size_granularity = 1, .unit = DEVLINK_RESOURCE_UNIT_ENTRY }; + struct net *net = nsim_devlink_net(devlink); int err; u64 n; @@ -126,8 +127,7 @@ static int nsim_dev_resources_register(struct devlink *devlink) goto out; } - n = nsim_fib_get_val(nsim_dev->fib_data, - NSIM_RESOURCE_IPV4_FIB, true); + 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); @@ -136,8 +136,7 @@ static int nsim_dev_resources_register(struct devlink *devlink) return err; } - n = nsim_fib_get_val(nsim_dev->fib_data, - NSIM_RESOURCE_IPV4_FIB_RULES, true); + 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); @@ -156,8 +155,7 @@ static int nsim_dev_resources_register(struct devlink *devlink) goto out; } - n = nsim_fib_get_val(nsim_dev->fib_data, - NSIM_RESOURCE_IPV6_FIB, true); + 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); @@ -166,8 +164,7 @@ static int nsim_dev_resources_register(struct devlink *devlink) return err; } - n = nsim_fib_get_val(nsim_dev->fib_data, - NSIM_RESOURCE_IPV6_FIB_RULES, true); + 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); @@ -179,19 +176,19 @@ static int nsim_dev_resources_register(struct devlink *devlink) devlink_resource_occ_get_register(devlink, NSIM_RESOURCE_IPV4_FIB, nsim_dev_ipv4_fib_resource_occ_get, - nsim_dev); + net); devlink_resource_occ_get_register(devlink, NSIM_RESOURCE_IPV4_FIB_RULES, nsim_dev_ipv4_fib_rules_res_occ_get, - nsim_dev); + net); devlink_resource_occ_get_register(devlink, NSIM_RESOURCE_IPV6_FIB, nsim_dev_ipv6_fib_resource_occ_get, - nsim_dev); + net); devlink_resource_occ_get_register(devlink, NSIM_RESOURCE_IPV6_FIB_RULES, nsim_dev_ipv6_fib_rules_res_occ_get, - nsim_dev); + net); out: return err; } @@ -199,11 +196,11 @@ static int nsim_dev_resources_register(struct devlink *devlink) static int nsim_dev_reload(struct devlink *devlink, struct netlink_ext_ack *extack) { - struct nsim_dev *nsim_dev = devlink_priv(devlink); enum nsim_resource_id res_ids[] = { NSIM_RESOURCE_IPV4_FIB, NSIM_RESOURCE_IPV4_FIB_RULES, NSIM_RESOURCE_IPV6_FIB, NSIM_RESOURCE_IPV6_FIB_RULES }; + struct net *net = nsim_devlink_net(devlink); int i; for (i = 0; i < ARRAY_SIZE(res_ids); ++i) { @@ -212,8 +209,7 @@ static int nsim_dev_reload(struct devlink *devlink, err = devlink_resource_size_get(devlink, res_ids[i], &val); if (!err) { - err = nsim_fib_set_max(nsim_dev->fib_data, - res_ids[i], val, extack); + err = nsim_fib_set_max(net, res_ids[i], val, extack); if (err) return err; } @@ -285,15 +281,9 @@ nsim_dev_create(struct nsim_bus_dev *nsim_bus_dev, unsigned int port_count) mutex_init(&nsim_dev->port_list_lock); nsim_dev->fw_update_status = true; - nsim_dev->fib_data = nsim_fib_create(); - if (IS_ERR(nsim_dev->fib_data)) { - err = PTR_ERR(nsim_dev->fib_data); - goto err_devlink_free; - } - err = nsim_dev_resources_register(devlink); if (err) - goto err_fib_destroy; + goto err_devlink_free; err = devlink_register(devlink, &nsim_bus_dev->dev); if (err) @@ -315,8 +305,6 @@ nsim_dev_create(struct nsim_bus_dev *nsim_bus_dev, unsigned int port_count) devlink_unregister(devlink); err_resources_unregister: devlink_resources_unregister(devlink, NULL); -err_fib_destroy: - nsim_fib_destroy(nsim_dev->fib_data); err_devlink_free: devlink_free(devlink); return ERR_PTR(err); @@ -330,7 +318,6 @@ static void nsim_dev_destroy(struct nsim_dev *nsim_dev) nsim_dev_debugfs_exit(nsim_dev); devlink_unregister(devlink); devlink_resources_unregister(devlink, NULL); - nsim_fib_destroy(nsim_dev->fib_data); mutex_destroy(&nsim_dev->port_list_lock); devlink_free(devlink); } diff --git a/drivers/net/netdevsim/fib.c b/drivers/net/netdevsim/fib.c index 8c57ba747772..f61d094746c0 100644 --- a/drivers/net/netdevsim/fib.c +++ b/drivers/net/netdevsim/fib.c @@ -18,6 +18,7 @@ #include <net/ip_fib.h> #include <net/ip6_fib.h> #include <net/fib_rules.h> +#include <net/netns/generic.h> #include "netdevsim.h" @@ -32,14 +33,15 @@ struct nsim_per_fib_data { }; struct nsim_fib_data { - struct notifier_block fib_nb; struct nsim_per_fib_data ipv4; struct nsim_per_fib_data ipv6; }; -u64 nsim_fib_get_val(struct nsim_fib_data *fib_data, - enum nsim_resource_id res_id, bool max) +static unsigned int nsim_fib_net_id; + +u64 nsim_fib_get_val(struct net *net, enum nsim_resource_id res_id, bool max) { + struct nsim_fib_data *fib_data = net_generic(net, nsim_fib_net_id); struct nsim_fib_entry *entry; switch (res_id) { @@ -62,10 +64,10 @@ u64 nsim_fib_get_val(struct nsim_fib_data *fib_data, return max ? entry->max : entry->num; } -int nsim_fib_set_max(struct nsim_fib_data *fib_data, - enum nsim_resource_id res_id, u64 val, +int nsim_fib_set_max(struct net *net, enum nsim_resource_id res_id, u64 val, struct netlink_ext_ack *extack) { + struct nsim_fib_data *fib_data = net_generic(net, nsim_fib_net_id); struct nsim_fib_entry *entry; int err = 0; @@ -118,9 +120,9 @@ static int nsim_fib_rule_account(struct nsim_fib_entry *entry, bool add, return err; } -static int nsim_fib_rule_event(struct nsim_fib_data *data, - struct fib_notifier_info *info, bool add) +static int nsim_fib_rule_event(struct fib_notifier_info *info, bool add) { + struct nsim_fib_data *data = net_generic(info->net, nsim_fib_net_id); struct netlink_ext_ack *extack = info->extack; int err = 0; @@ -155,9 +157,9 @@ static int nsim_fib_account(struct nsim_fib_entry *entry, bool add, return err; } -static int nsim_fib_event(struct nsim_fib_data *data, - struct fib_notifier_info *info, bool add) +static int nsim_fib_event(struct fib_notifier_info *info, bool add) { + struct nsim_fib_data *data = net_generic(info->net, nsim_fib_net_id); struct netlink_ext_ack *extack = info->extack; int err = 0; @@ -176,22 +178,18 @@ static int nsim_fib_event(struct nsim_fib_data *data, static int nsim_fib_event_nb(struct notifier_block *nb, unsigned long event, void *ptr) { - struct nsim_fib_data *data = container_of(nb, struct nsim_fib_data, - fib_nb); struct fib_notifier_info *info = ptr; int err = 0; switch (event) { case FIB_EVENT_RULE_ADD: /* fall through */ case FIB_EVENT_RULE_DEL: - err = nsim_fib_rule_event(data, info, - event == FIB_EVENT_RULE_ADD); + err = nsim_fib_rule_event(info, event == FIB_EVENT_RULE_ADD); break; case FIB_EVENT_ENTRY_ADD: /* fall through */ case FIB_EVENT_ENTRY_DEL: - err = nsim_fib_event(data, info, - event == FIB_EVENT_ENTRY_ADD); + err = nsim_fib_event(info, event == FIB_EVENT_ENTRY_ADD); break; } @@ -201,23 +199,30 @@ static int nsim_fib_event_nb(struct notifier_block *nb, unsigned long event, /* inconsistent dump, trying again */ static void nsim_fib_dump_inconsistent(struct notifier_block *nb) { - struct nsim_fib_data *data = container_of(nb, struct nsim_fib_data, - fib_nb); + struct nsim_fib_data *data; + struct net *net; + + rcu_read_lock(); + for_each_net_rcu(net) { + data = net_generic(net, nsim_fib_net_id); + + data->ipv4.fib.num = 0ULL; + data->ipv4.rules.num = 0ULL; - data->ipv4.fib.num = 0ULL; - data->ipv4.rules.num = 0ULL; - data->ipv6.fib.num = 0ULL; - data->ipv6.rules.num = 0ULL; + data->ipv6.fib.num = 0ULL; + data->ipv6.rules.num = 0ULL; + } + rcu_read_unlock(); } -struct nsim_fib_data *nsim_fib_create(void) -{ - struct nsim_fib_data *data; - int err; +static struct notifier_block nsim_fib_nb = { + .notifier_call = nsim_fib_event_nb, +}; - data = kzalloc(sizeof(*data), GFP_KERNEL); - if (!data) - return ERR_PTR(-ENOMEM); +/* Initialize per network namespace state */ +static int __net_init nsim_fib_netns_init(struct net *net) +{ + struct nsim_fib_data *data = net_generic(net, nsim_fib_net_id); data->ipv4.fib.max = (u64)-1; data->ipv4.rules.max = (u64)-1; @@ -225,22 +230,37 @@ struct nsim_fib_data *nsim_fib_create(void) data->ipv6.fib.max = (u64)-1; data->ipv6.rules.max = (u64)-1; - data->fib_nb.notifier_call = nsim_fib_event_nb; - err = register_fib_notifier(&data->fib_nb, nsim_fib_dump_inconsistent); - if (err) { - pr_err("Failed to register fib notifier\n"); - goto err_out; - } + return 0; +} - return data; +static struct pernet_operations nsim_fib_net_ops = { + .init = nsim_fib_netns_init, + .id = &nsim_fib_net_id, + .size = sizeof(struct nsim_fib_data), +}; -err_out: - kfree(data); - return ERR_PTR(err); +void nsim_fib_exit(void) +{ + unregister_pernet_subsys(&nsim_fib_net_ops); + unregister_fib_notifier(&nsim_fib_nb); } -void nsim_fib_destroy(struct nsim_fib_data *data) +int nsim_fib_init(void) { - unregister_fib_notifier(&data->fib_nb); - kfree(data); + int err; + + err = register_pernet_subsys(&nsim_fib_net_ops); + if (err < 0) { + pr_err("Failed to register pernet subsystem\n"); + goto err_out; + } + + err = register_fib_notifier(&nsim_fib_nb, nsim_fib_dump_inconsistent); + if (err < 0) { + pr_err("Failed to register fib notifier\n"); + goto err_out; + } + +err_out: + return err; } diff --git a/drivers/net/netdevsim/netdev.c b/drivers/net/netdevsim/netdev.c index 0740940f41b1..55f57f76d01b 100644 --- a/drivers/net/netdevsim/netdev.c +++ b/drivers/net/netdevsim/netdev.c @@ -357,12 +357,18 @@ static int __init nsim_module_init(void) if (err) goto err_dev_exit; - err = rtnl_link_register(&nsim_link_ops); + err = nsim_fib_init(); if (err) goto err_bus_exit; + err = rtnl_link_register(&nsim_link_ops); + if (err) + goto err_fib_exit; + return 0; +err_fib_exit: + nsim_fib_exit(); err_bus_exit: nsim_bus_exit(); err_dev_exit: @@ -373,6 +379,7 @@ static int __init nsim_module_init(void) static void __exit nsim_module_exit(void) { rtnl_link_unregister(&nsim_link_ops); + nsim_fib_exit(); nsim_bus_exit(); nsim_dev_exit(); } diff --git a/drivers/net/netdevsim/netdevsim.h b/drivers/net/netdevsim/netdevsim.h index 79c05af2a7c0..9404637d34b7 100644 --- a/drivers/net/netdevsim/netdevsim.h +++ b/drivers/net/netdevsim/netdevsim.h @@ -169,12 +169,10 @@ int nsim_dev_port_add(struct nsim_bus_dev *nsim_bus_dev, int nsim_dev_port_del(struct nsim_bus_dev *nsim_bus_dev, unsigned int port_index); -struct nsim_fib_data *nsim_fib_create(void); -void nsim_fib_destroy(struct nsim_fib_data *fib_data); -u64 nsim_fib_get_val(struct nsim_fib_data *fib_data, - enum nsim_resource_id res_id, bool max); -int nsim_fib_set_max(struct nsim_fib_data *fib_data, - enum nsim_resource_id res_id, u64 val, +int nsim_fib_init(void); +void nsim_fib_exit(void); +u64 nsim_fib_get_val(struct net *net, enum nsim_resource_id res_id, bool max); +int nsim_fib_set_max(struct net *net, enum nsim_resource_id res_id, u64 val, struct netlink_ext_ack *extack); #if IS_ENABLED(CONFIG_XFRM_OFFLOAD)