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. Cheers, Eelco >>> Signed-off-by: Roi Dayan <[email protected]> >>> Acked-by: Eli Britstein <[email protected]> >>> --- >>> lib/dpif-netdev.c | 9 ++++++++- >>> lib/dpif-netdev.h | 2 -- >>> lib/dpif-netlink.c | 1 + >>> lib/dpif-provider.h | 3 +++ >>> lib/dpif.c | 14 ++++++++++---- >>> 5 files changed, 22 insertions(+), 7 deletions(-) >>> >>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c >>> index 2a529f272d16..8dd0f8c2b2b7 100644 >>> --- a/lib/dpif-netdev.c >>> +++ b/lib/dpif-netdev.c >>> @@ -686,7 +686,7 @@ pmd_thread_ctx_time_update(struct dp_netdev_pmd_thread >>> *pmd) >>> } >>> >>> /* Returns true if 'dpif' is a netdev or dummy dpif, false otherwise. */ >>> -bool >>> +static bool >>> dpif_is_netdev(const struct dpif *dpif) >>> { >>> return dpif->dpif_class->open == dpif_netdev_open; >>> @@ -9995,6 +9995,12 @@ dpif_netdev_bond_stats_get(struct dpif *dpif, >>> uint32_t bond_id, >>> return 0; >>> } >>> >>> +static bool >>> +dpif_netdev_is_userspace(void) >>> +{ >>> + return true; >>> +} >>> + >>> const struct dpif_class dpif_netdev_class = { >>> "netdev", >>> true, /* cleanup_required */ >>> @@ -10084,6 +10090,7 @@ const struct dpif_class dpif_netdev_class = { >>> NULL, /* cache_get_name */ >>> NULL, /* cache_get_size */ >>> NULL, /* cache_set_size */ >>> + dpif_netdev_is_userspace, >>> }; >>> >>> static void >>> diff --git a/lib/dpif-netdev.h b/lib/dpif-netdev.h >>> index 6db6ed2e2128..3323eb0204af 100644 >>> --- a/lib/dpif-netdev.h >>> +++ b/lib/dpif-netdev.h >>> @@ -33,8 +33,6 @@ extern "C" { >>> * headers to be aligned on a 4-byte boundary. */ >>> enum { DP_NETDEV_HEADROOM = 2 + VLAN_HEADER_LEN }; >>> >>> -bool dpif_is_netdev(const struct dpif *); >>> - >>> #define NR_QUEUE 1 >>> #define NR_PMD_THREADS 1 >>> >>> diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c >>> index 84e2bd8eaf58..5dbe847f61b0 100644 >>> --- a/lib/dpif-netlink.c >>> +++ b/lib/dpif-netlink.c >>> @@ -4589,6 +4589,7 @@ const struct dpif_class dpif_netlink_class = { >>> dpif_netlink_cache_get_name, >>> dpif_netlink_cache_get_size, >>> dpif_netlink_cache_set_size, >>> + NULL, /* is_userspace */ >>> }; >>> >>> static int >>> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h >>> index 520e21e68db2..e8a8d36a80c5 100644 >>> --- a/lib/dpif-provider.h >>> +++ b/lib/dpif-provider.h >>> @@ -683,6 +683,9 @@ struct dpif_class { >>> >>> /* Set cache size. */ >>> int (*cache_set_size)(struct dpif *dpif, uint32_t level, uint32_t >>> size); >>> + >>> + /* Return if dpif is a userspace datapath. */ >>> + bool (*is_userspace)(void); >>> }; >>> >>> extern const struct dpif_class dpif_netlink_class; >>> diff --git a/lib/dpif.c b/lib/dpif.c >>> index a064f717f1a6..e281e893c98e 100644 >>> --- a/lib/dpif.c >>> +++ b/lib/dpif.c >>> @@ -1930,17 +1930,23 @@ log_flow_get_message(const struct dpif *dpif, >>> } >>> } >>> >>> +static bool >>> +dpif_is_userspace(const struct dpif *dpif) >>> +{ >>> + return dpif->dpif_class->is_userspace && >>> dpif->dpif_class->is_userspace(); >>> +} >>> + >>> bool >>> dpif_supports_tnl_push_pop(const struct dpif *dpif) >>> { >>> - return dpif_is_netdev(dpif); >>> + return dpif_is_userspace(dpif); >>> } >>> >>> bool >>> dpif_may_support_explicit_drop_action(const struct dpif *dpif) >>> { >>> /* TC does not support offloading this action. */ >>> - return dpif_is_netdev(dpif) || !netdev_is_flow_api_enabled(); >>> + return dpif_is_userspace(dpif) || !netdev_is_flow_api_enabled(); >>> } >>> >>> bool >>> @@ -1950,14 +1956,14 @@ dpif_supports_lb_output_action(const struct dpif >>> *dpif) >>> * Balance-tcp optimization is currently supported in netdev >>> * datapath only. >>> */ >>> - return dpif_is_netdev(dpif); >>> + return dpif_is_userspace(dpif); >>> } >>> >>> bool >>> dpif_may_support_psample(const struct dpif *dpif) >>> { >>> /* Userspace datapath does not support this action. */ >>> - return !dpif_is_netdev(dpif); >>> + return !dpif_is_userspace(dpif); >>> } >>> >>> /* Meters */ >>> -- >>> 2.21.0 >>> >>> _______________________________________________ >>> dev mailing list >>> [email protected] >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
