On 22/01/2025 14:30, Ilya Maximets wrote: > On 1/22/25 13:02, Roi Dayan via dev wrote: >> >> >> On 22/01/2025 11:34, Eelco Chaudron wrote: >>> >>> >>> On 22 Jan 2025, at 8:37, Roi Dayan wrote: >>> >>>> On 21/01/2025 15:02, Ilya Maximets wrote: >>>>> On 1/20/25 16:13, Eelco Chaudron wrote: >>>>>> >>>>>> >>>>>> On 20 Jan 2025, at 14:50, Roi Dayan wrote: >>>>>> >>>>>>> On 20/01/2025 14:57, Eelco Chaudron wrote: >>>>>>>> >>>>>>>> >>>>>>>> On 15 Jan 2025, at 10:18, Roi Dayan via dev wrote: >>>>>>>> >>>>>>>>> Use this instead of directly using dpif_is_netdev() from dpif-netdev. >>>>>>>>> Return true in dpif-netdev. >>>>>>>> >>>>>>>> Not sure if we need an API for this, or maybe I should say, what is >>>>>>>> the definition >>>>>>>> of userspace? If this means anything but the kernel, we could probably >>>>>>>> use !dpif_is_netlink(). >>>>>>>> >>>>>>>> But I guess, the better question would be, what would we use this call >>>>>>>> for? If it’s similar >>>>>>>> to the existing use cases, we are probably better off by adding a >>>>>>>> features API to dpif. >>>>>>>> >>>>>>>> Thoughts? >>>>>>>> >>>>>>>> Cheers, >>>>>>>> >>>>>>>> Eelco >>>>>>> >>>>>>> >>>>>>> Hi, >>>>>>> >>>>>>> The meaning of dpif is a userspace means it needs to handle tunneling >>>>>>> and other stuff that >>>>>>> kernel dpif doesn't need as the kernel does it for it. >>>>>>> So while !dpif_is_netlink() is even more correct and better than >>>>>>> current dpif_is_netdev() >>>>>>> there was also a point here that dpif.c is not familiar with a specific >>>>>>> dpif and not accessing >>>>>>> a specific dpif function. >>>>>> >>>>>> So, this means a lot of potential stuff, so to me a general >>>>>> dpif_is_userspace() does not make sense. >>>>>> >>>>>>> I think introducing a features API to dpif could be an overkill at this >>>>>>> point. >>>>>>> So if future dpifs might need something specific other than handled by >>>>>>> kernel or not I think >>>>>>> this can be done at a later time. >>>>>> >>>>>> It might look like an overkill right now, but the current use cases >>>>>> might warrant one already. >>>>>> Or are you suggesting to use/introduce !dpif_is_netlink() instead for >>>>>> now? >>>>>> >>>>>> >>>>>> Ilya and other maintainers have any preferences or thoughts on this. >>>>> >>>>> In general, using either dpif_is_netlink or dpif_is_netdev outside of >>>>> dpif-netlink >>>>> and dpif-netdev respectively is icky. And I agree that even today the >>>>> number of >>>>> uses of these functions outside their modules justifies doing the feature >>>>> discovery >>>>> properly. Majority of the cases here in dpif.c are caused by the fact >>>>> that >>>>> dpif-netdev doesn't validate actions, so it's not possible to probe them. >>>>> It may be >>>>> better to introduce some form of action validation in case of probing >>>>> instead, so >>>>> the features can be discovered normally. >>>>> >>>>> Best regards, Ilya Maximets. >>>> >>>> I think this is out-of-scope for this patch. >>>> In this small commit I just wanted to do the separation from dpif.c knowing >>>> about existing dpifs and accessing them directly and not introducing new >>>> layers and mechanisem. > > Feature probing is not a new mechanism. We do that for many other actions > and it is a primary mechanism for a datapath feature discovery. > >>>> Can we continue with this patch currently? a rename to the callback is >>>> also feasible though >>>> I think its saying exactly what it is. >>> >>> This patch introduces a new API, but its purpose is unclear to me. While >>> it’s a small change, >>> I don't see a concrete use case for it. The term 'userspace' seems vague >>> when used outside the >>> context of the actual dpif. Could you provide a clear example of how this >>> API will be used? >>> This might help us explore better alternative approaches if needed. For the >>> current use cases >>> see Ilya’s response. >>> >>> Cheers, >>> >>> Eelco >>> >> >> A concrete example is tunnel handling. In "netdev" datapath, OVS userspace >> application handles >> tunnel push and pop, as opposed to "netlink" datapath in which the kernel >> handles it. >> A more suitable name for it in the first place was "is_userspace". >> We want to add another userspace datapath (dpif-doca), that will handle >> tunnels, therefore this >> change. > > This is exactly the use case for a feature probing. How we do that for other > matches and actions is constructing a dummy flow rule, calling flow_put with a > probe flag set and letting datapath validate it. This works for various > versions > of linux kernel datapath that can be running and that works for windows > datapath. > This works fine for userspace datapath as well, except only for matches. For > actions it doesn't work, because the userspace datapath doesn't validate > actions. > If we plan to have multiple implementations for userspace-style datpaths, > then we > should probe them properly as we do for multiple implementations of > dpif-netlink.
Are you talking about the TC probing in netdev-offload-tc ? those probing is not being done from the dpif.c. TC offloading checks the probes there and either offload or not. Maybe all those wrappers checking dpif_is_netdev in dpif.c should not exists then and maybe all actions should just be passed into the dpif and the dpif itself should decide what to do with the actions. dpif-netlink should decide if to skip the actions or not. All this seems out of scope of keeping the same behavior as it but just opening it a bit without touching the specific logic that exists now. Moving to the proposal using !dpif_is_netlink also solves the issue for userspace dpif implementations. if not, I don't understand what kind of probing is expected here. can you share an example? > > This is_userspace call is actually a request "if this implementation supports > the following actions: tunnel_push_pop, explicit drop, lb_output and > psample?". > Answer "true" corresponds to "yes, yes, yes, no", and "false" means "no, > maybe, > no, maybe", and then dpif.c proceeds to probe for "maybe" actions. > > So, this call is only a shortcut with non-obvious overloaded meaning that will > change over time as we add new actions. Instead, we can remove it and just do > probing as dpif layer already does for most of other matches and actions. > It should also be easier for you to maintain an alternative userspace datapath > implementation this way, as you'll not need to worry about what this call > actually means, as dpif layer would just probe what it needs directly instead > of assuming things based on a single boolean value. > > Best regards, Ilya Maximets. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
