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