Message-ID: <20190419052501.GA2247@nanopsycho>
Date:   Fri, 19 Apr 2019 07:25:01 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, mlxsw@...lanox.com
Subject: Re: [patch net-next rfc 00/15] netdevsim: impement proper device
 model

Thu, Apr 18, 2019 at 07:07:48PM CEST, jakub.kicinski@...ronome.com wrote:
>On Thu, 18 Apr 2019 09:22:56 +0200, Jiri Pirko wrote:
>> Tue, Apr 16, 2019 at 08:04:59PM CEST, jakub.kicinski@...ronome.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.

Yeah, but since the api is changing, the original approach of using
IFLA_LINK cannot be used anymore. Not to mention is is abuse of the API
from the very beginning. You want to add/del ports during lifetime, I'm
trying to find interface. Basically whe have two options:
1) devlink extension I suggested above
2) sysfs files to add/del ports. Similar to "new_device" and
   "del_device".

I like the idea of having 2) better, the reason is this is very speficic
to netdevsim and not really applicable on real devices.


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

Powered by blists - more mailing lists