On 14 Jan 2026, at 14:10, Ilya Maximets wrote:
> 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. Ok, I did the following for this patch to make the functions align better: dpif_offload_tc_netdev_match_to_dpif_flow() -> tc_netdev_match_to_dpif_flow() netdev_offload_tc_flow_dump_create() --> tc_flow_dump_create() netdev_offload_tc_flow_dump_destroy() --> tc_flow_dump_destroy() netdev_offload_tc_flow_dump_next() --> tc_flow_dump_next() netdev_offload_tc_flow_flush() --> tc_flow_flush I will handle the removal of the dpif_offload_ prefix for local functions and structures as part of a separate patch. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
