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. 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
