On 1/14/26 1:42 PM, Eelco Chaudron wrote: > > > On 13 Jan 2026, at 23:55, Ilya Maximets wrote: > >> 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(). >>> --- > > [...] > >>> +/* 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 () are not looking >> particularly great. > > I think it would be good to clean this up, but I’d rather do this in a > separate patch once this series is applied due to the amount of churn in > the individual patches.
Sounds fine to me. Thanks. > > So for this series, I would like to go with the surgical approach: > > dpif_offload_tc_netdev_match_to_dpif_flow() --> tc_netdev_match_to_dpif_flow() > 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 > > Anything else for this series that you want changed? Some of the examples were randomly picked by me, so I'd suggest to keep at least some consistency, i.e. if we're renaming the dump_create, then also rename at least dump_next and dump_done, so it's not a complete mess. And then we could rename the rest in a separate change. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
