On 22/01/2025 15:02, Roi Dayan wrote: > > > 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? >
Hi, Just updating here that point is not to change the API or logic of the callers using the dpif_is_netdev. dpif_supports_tnl_push_pop dpif_may_support_explicit_drop_action dpif_supports_lb_output_action dpif_may_support_psample My intention that if I test with a new userspace dpif then not getting blocked by dpif that those features are not supported just because its not specific dpif-netdev. Please provide an example how you suggest you proceed here. Thanks, Roi >> >> 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
