Please help me reviewing the code, thanks very much.
At 2019-10-28 20:44:51, "zhaozhanxu" <[email protected]> wrote: Thanks for your reply. I already modified some of them, and I think the others need to discuss. V2===>V3: 1. Make the n_offload_packets to be a subset of n_packets, and n_offload_bytes to be a subset of n_bytes. 2. Add a new structure 'dpif_flow_detailed_stats' to store the offload statistics, so all the functions you mentioned don't need to modify. I don't think that function 'parse_tc_flower_to_match' should display new fields, all the commands called function 'parse_tc_flower_to_match' are display the datapath flows. The datapath flow would be either non-offloaded or offloaded, so that the statistics are offloaded or non-offloaded depends on datapath flows. The patch link: https://mail.openvswitch.org/pipermail/ovs-dev/2019-October/363942.html At 2019-10-25 08:53:35, "Ben Pfaff" <[email protected]> wrote: >On Tue, Oct 15, 2019 at 02:56:23PM +0800, zhaozhanxu wrote: >> Add argument '-m' for command ovs-appctl bridge/dump-flows >> to display the offloaded packets statistics. > >Thanks for the updated patch. > >Are n_offload_packets a subset of n_packets, or in addition to them? If >they are a subset, then n_offload_packets should always be less than or >equal to n_packets; if they are in addition, then there is no >relationship between the two. Either way, this should be explained in >comments on struct dpif_flow_stats. > >I guess that "subset" makes more sense--after all, offloaded packets are >still packets--but then we need to update a few places, like >rule_dpif_credit_stats__(). If we take the "additional" choice, then we >need to change other places: I think most places that n_packets is >referenced, we need to write n_packets + n_offload_packets instead. > >Please also update the documentation for bridge/dump-flows in >ovs-vswitchd(8). > >get_dpif_flow_stats(), in lib/dpif-netdev.c, should initialize '*stats'. >I don't think it initializes the new members, but it should. > >dpif_netdev_flow_put() adds together dpif_flow_stats. Currently I think >those dpif_flow_stats always have zero in n_offload_*, but it might be a >good idea to add them together anyway. > >Actually dpif_netdev_flow_del() does the same thing. Probably that >means it would be a good idea to factor out the code that adds these >things together into a new helper function since there's already >duplicate code and this adds more of it. > >dpif_netlink_flow_get_stats() needs to initialize the new fields. > >dpif_flow_stats_extract() needs to initialize the new fields. > >I imagine that dpif_flow_stats_format() should display the new >fields--perhaps only if they are nonzero? > >Should parse_tc_flower_to_match() set n_offload_* instead of packets and >bytes? (Or both of them to the same values, if n_offload_* is a subset >of the regular values.) > >Ditto for netdev_tc_flow_del(). > >upcall_xlate() should initialize n_offload_*. > >Ditto for revalidate_ukey() and push_dp_ops(). > >Thanks, > >Ben. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
