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. >> 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. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
