On 1/12/26 12:20 PM, Eelco Chaudron wrote:
> Relocate the tc flow dump functionality from netdev-offload
> to dpif-offload, centralizing flow dump handling in the
> dpif-offload layer.
>
> Acked-by: Eli Britstein <elibr.nvidia.com>
> Signed-off-by: Eelco Chaudron <[email protected]>
> ---
>
> v2 changes:
> - Use ovs_mutex_destroy() instead of ovs_mutex_init() in
> dpif_offload_tc_flow_dump_destroy().
> ---
> lib/dpif-offload-provider.h | 1 +
> lib/dpif-offload-tc.c | 248 ++++++++++++++++++++++++++++++++++--
> lib/dpif-offload.c | 6 +
> lib/netdev-offload-tc.c | 34 +++--
> lib/netdev-offload-tc.h | 9 ++
> 5 files changed, 265 insertions(+), 33 deletions(-)
>
> diff --git a/lib/dpif-offload-provider.h b/lib/dpif-offload-provider.h
> index 4061f6e02..8840441c0 100644
> --- a/lib/dpif-offload-provider.h
> +++ b/lib/dpif-offload-provider.h
> @@ -281,6 +281,7 @@ bool dpif_offload_port_mgr_add(struct
> dpif_offload_port_mgr *,
> struct dpif_offload_port_mgr_port *dpif_offload_port_mgr_remove(
> struct dpif_offload_port_mgr *, odp_port_t, bool keep_netdev_ref);
> void dpif_offload_port_mgr_uninit(struct dpif_offload_port_mgr *);
> +size_t dpif_offload_port_mgr_port_count(struct dpif_offload_port_mgr *);
> struct dpif_offload_port_mgr_port *dpif_offload_port_mgr_find_by_ifindex(
> struct dpif_offload_port_mgr *, int ifindex);
> struct dpif_offload_port_mgr_port *dpif_offload_port_mgr_find_by_netdev(
> diff --git a/lib/dpif-offload-tc.c b/lib/dpif-offload-tc.c
> index d60919f2a..78f3028e4 100644
> --- a/lib/dpif-offload-tc.c
> +++ b/lib/dpif-offload-tc.c
> @@ -19,13 +19,16 @@
>
> #include "dpif-offload.h"
> #include "dpif-offload-provider.h"
> +#include "netdev-offload.h"
> #include "netdev-offload-tc.h"
> #include "netdev-provider.h"
> #include "netdev-vport.h"
> +#include "odp-util.h"
> #include "tc.h"
> #include "util.h"
>
> #include "openvswitch/json.h"
> +#include "openvswitch/match.h"
> #include "openvswitch/vlog.h"
>
> VLOG_DEFINE_THIS_MODULE(dpif_offload_tc);
> @@ -39,6 +42,30 @@ struct dpif_offload_tc {
> struct ovsthread_once once_enable; /* Track first-time enablement. */
> };
>
> +/* tc's flow dump specific data structures. */
> +struct dpif_offload_tc_flow_dump {
This is not used outside this module. Can we call it tc_flow_dump?
In general, the naming between dpif-offload-tc.c and dpif-offload-tc-netdev.c
later in the set is a little hard to understand. First of all, there are
many functions and structures that are not used outside of dpif-offload-tc*
and so can have shorter names. Secondly, the netdev_offload_tc_* namnig
is confusing, at least while reviewing this set. ANd it gets in the way
when just reading the code as well.
So, I'd suggest to name things that are only used within the dpif-offload-tc-*
modules with the tc_ prefix and use extra identification for sub-modules,
e.g. tc_netdev_* for local functions in dpif-offload-tc-netdev.c.
Note, this applies to provider class implementation methods that are
technically local to the module as well.
So, for example we'd have:
struct dpif_offload_tc_flow_dump --> struct tc_flow_dump
struct dpif_offload_tc_flow_dump_thread --> struct tc_flow_dump_thread
struct netdev_tc_flow_dump --> struct tc_netdev_flow_dump
dpif_offload_tc_flow_dump_create --> tc_flow_dump_create
dpif_offload_tc_netdev_match_to_dpif_flow --> tc_match_to_dpif_flow
dpif_offload_tc_flow_dump_next --> tc_flow_dump_next
netdev_offload_tc_flow_dump_next --> tc_netdev_flow_dump_next
netdev_offload_tc_flow_flush --> tc_netdev_flow_flush
WDYT?
Same applies to other offlaod providers.
There is a slight chance for the clash with lib/tc.{c,h}, but all the
names above have 'flow' or could have 'flow' in them, that would
differenciate.
Alternatively, we can go with a more surgical approach and just fix
some of the most scary one, e.g. dpif_offload_tc_netdev_match_to_dpif_flow,
as the function calls in dpif_offload_tc_flow_dump_next() are not looking
particularly great.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
