[ovs-dev] [PATCH V2] Add offload packets statistics
zhaozhanxu at 163.com
Mon Oct 28 12:44:51 UTC 2019
Thanks for your reply. I already modified some of them, and I think the others need to discuss.
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" <blp at ovn.org> 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
>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().
More information about the dev