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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
