[ovs-dev] [PATCH V2] Add offload packets statistics

[zhaozhanxu]

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" <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
>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.