Re: [patch net-next rfc 00/15] netdevsim: impement proper device model

[Jakub Kicinski <jakub.kicinski@netronome.com>]

On Thu, 18 Apr 2019 09:22:56 +0200, Jiri Pirko wrote:
> Tue, Apr 16, 2019 at 08:04:59PM CEST, jakub.kicinski@netronome.com wrote:
> >On Tue, 16 Apr 2019 10:59:37 +0200, Jiri Pirko wrote:  
> >> >> 4) netdevsim instances are created by "ip link add" which is great for
> >> >>    soft devices with no hw backend. The rtnl core allocates netdev and
> >> >>    calls into driver holding rtnl mutex. For hw-backed devices, this
> >> >>    flow is wrong as it breaks order in which things are done.
> >> >> 
> >> >> This patchset adjust netdevsim to fix all above.
> >> >> 
> >> >> In order to support proper devlink and devlink port instances and to be
> >> >> able to emulate real devices, there is need to implement bus probe and
> >> >> instantiate everything from there. User can specify device id and port
> >> >> count to be instantianted. For example:
> >> >> 
> >> >> echo "10 4" > /sys/bus/netdevsim/new_device    
> >> >
> >> >I really don't like the design where ID has to be allocated by user
> >> >space.  It's a step back.
> >> >
> >> >I also dislike declaring ports from the start.  In real drivers ports
> >> >are never "atomically" registered, they are crated and destroyed one     
> >> 
> >> Care to define "atomically" here? It is done in a very similar way
> >> to how it is done in mlxsw for example. Same flows.
> >> 
> >>   
> >> >by one, and a lot of races/UAFs/bugs lie in those small periods of
> >> >time where one netdev got unregistered, but other are still around...    
> >> 
> >> Same here. Not sure where do you see the differences.  
> >
> >The difference is that today I can do this:
> >
> >create a netdevsim1 with shared dev 1
> >create some state associated with shared dev 1
> >create a netdevsim2 with shared dev 1
> >check if all the shared dev 1 state created for netdevsim1 is visible
> >	via netdevsim2  
> 
> Hmm, you are testing netdevsim implementation then, not the kernel
> interfaces. What is the point of testing netdevsim?

BPF offload tries to leave as much code as possible in the core, and
make the drivers simple.  I'm testing whether the core reacts correctly,
netdevsim just calls register/unregister.

> >destroy netdevsim1
> >check the shared dev 1 state again
> >
> >If I say "give me 2 ports" from the start, that makes the testing
> >(which is the whole point of this code) harder.
> >  
> >> Also, I plan to implement port splitting in follow-up patchset. All
> >> flows are there as well.  
> >
> >Sure, let's just be clear that we won't be merging an ABI that has just
> >a netdevsim implementation, right?  I have some reservations about the  
> 
> So what do you suggest? Allow to somehow add and remove ports during
> test? You can already do that with VFs. Do you want to do that with
> netdevsim "physical" ports? If yes, how? I can imagine to extend devlink
> port api with something like:
> 
> $ sudo devlink dev
> netdevsim/netdevsim0
> $ sudo devlink port
> netdevsim/netdevsim0/0: type eth netdev eth0 flavour physical
> 
> $ sudo devlink dev port add netdevsim/netdevsim0 index 22
> $ sudo devlink port
> netdevsim/netdevsim0/0: type eth netdev eni0p1 flavour physical
> netdevsim/netdevsim0/22: type eth netdev eni0p23 flavour physical
> 
> $ sudo devlink port del netdevsim/netdevsim0/0
> $ sudo devlink port
> netdevsim/netdevsim0/22: type eth netdev eni0p23 flavour physical
> 
> But I see only usecase for this extension for netdevsim, not for real
> devices..

Hm.. I'm getting lost, sorry, I'm probably confusing myself here..

Netdevsim is supposed to test real, existing kernel interfaces and core
code.  What we do today with linking based on netdevs is quite simple
and works very well for the BPF offload tests.

If you want to test some devlink code, that's also real, perfect.

For BPF tests we want the ability to add and remove netdevs to a sdev
during tests, yes.  That ability cannot be lost.

> >"port splitting" or device slicing, which should be discussed over real
> >code, not netdevsim.