On 11/7/21 13:12, Roi Dayan via dev wrote: > From: Paul Blakey <[email protected]> > > Fragmented packets with offset=0 are defragmented by tc act_ct, and > only when assembled pass to next action, in ovs offload case, > a goto action. Since stats are overwritten on each action dump, > only the stats for last action in the tc filter action priority > list is taken, the stats on the goto action, which count > only the assembled packets. See below for example. > > Hardware updates just part of the actions (gact, ct, mirred) - those > that support stats_update() operation. Since datapath rules end > with either an output (mirred) or recirc/drop (both gact), tc rule > will at least have one action that supports it. For software packets, > the first action will have the max software packets count. > Tc dumps total packets (hw + sw) and hardware packets, then > software packets needs to be calculated from this (total - hw). > > To fix the above, get hardware packets and calculate software packets > for each action, take the max of each set, then combine back > to get the total packets that went through software and hardware. > > Example by running ping above MTU (ping <IP> -s 2000): > ct_state(-trk),recirc_id(0),...,ipv4(proto=1,frag=first), > packets:14, bytes:19544,..., actions:ct(zone=1),recirc(0x1) > ct_state(-trk),recirc_id(0),...,ipv4(proto=1,frag=later), > packets:14, bytes:28392,..., actions:ct(zone=1),recirc(0x1) > > Second rule should have had bytes=14*<size of 'later' frag>, but instead > it's bytes=14*<size of assembled packets - size of 'first' + 'later' > frags>. > > Fixes: 576126a931cd ("netdev-offload-tc: Add conntrack support") > Signed-off-by: Paul Blakey <[email protected]> > Reviewed-by: Roi Dayan <[email protected]> > --- > lib/netdev-offload-tc.c | 10 +++++----- > lib/tc.c | 34 ++++++++++++++++++++++++++++------ > lib/tc.h | 3 ++- > 3 files changed, 35 insertions(+), 12 deletions(-) > > diff --git a/lib/netdev-offload-tc.c b/lib/netdev-offload-tc.c > index 9845e8d3feae..3f7068c8e066 100644 > --- a/lib/netdev-offload-tc.c > +++ b/lib/netdev-offload-tc.c > @@ -585,8 +585,10 @@ parse_tc_flower_to_stats(struct tc_flower *flower, > } > > memset(stats, 0, sizeof *stats); > - stats->n_packets = get_32aligned_u64(&flower->stats.n_packets); > - stats->n_bytes = get_32aligned_u64(&flower->stats.n_bytes); > + stats->n_packets = get_32aligned_u64(&flower->stats_sw.n_packets); > + stats->n_packets += get_32aligned_u64(&flower->stats_hw.n_packets); > + stats->n_bytes = get_32aligned_u64(&flower->stats_sw.n_bytes); > + stats->n_bytes += get_32aligned_u64(&flower->stats_hw.n_bytes); > stats->used = flower->lastused; > } > > @@ -2015,9 +2017,7 @@ netdev_tc_flow_del(struct netdev *netdev OVS_UNUSED, > if (stats) { > memset(stats, 0, sizeof *stats); > if (!tc_get_flower(&id, &flower)) { > - stats->n_packets = get_32aligned_u64(&flower.stats.n_packets); > - stats->n_bytes = get_32aligned_u64(&flower.stats.n_bytes); > - stats->used = flower.lastused; > + parse_tc_flower_to_stats(&flower, stats); > } > } > > diff --git a/lib/tc.c b/lib/tc.c > index 38a1dfc0ebc8..961aef619815 100644 > --- a/lib/tc.c > +++ b/lib/tc.c > @@ -1702,6 +1702,9 @@ static const struct nl_policy stats_policy[] = { > [TCA_STATS_BASIC] = { .type = NL_A_UNSPEC, > .min_len = sizeof(struct gnet_stats_basic), > .optional = false, }, > + [TCA_STATS_BASIC_HW] = { .type = NL_A_UNSPEC, > + .min_len = sizeof(struct gnet_stats_basic), > + .optional = true, }, > }; > > static int > @@ -1714,8 +1717,11 @@ nl_parse_single_action(struct nlattr *action, struct > tc_flower *flower, > const char *act_kind; > struct nlattr *action_attrs[ARRAY_SIZE(act_policy)]; > struct nlattr *stats_attrs[ARRAY_SIZE(stats_policy)]; > - struct ovs_flow_stats *stats = &flower->stats; > - const struct gnet_stats_basic *bs; > + struct ovs_flow_stats *stats_sw = &flower->stats_sw; > + struct ovs_flow_stats *stats_hw = &flower->stats_hw; > + const struct gnet_stats_basic *bs_all = NULL; > + const struct gnet_stats_basic *bs_hw = NULL; > + struct gnet_stats_basic bs_sw = { .packets = 0, .bytes = 0, }; > int err = 0; > > if (!nl_parse_nested(action, act_policy, action_attrs, > @@ -1771,10 +1777,26 @@ nl_parse_single_action(struct nlattr *action, struct > tc_flower *flower, > return EPROTO; > } > > - bs = nl_attr_get_unspec(stats_attrs[TCA_STATS_BASIC], sizeof *bs); > - if (bs->packets) { > - put_32aligned_u64(&stats->n_packets, bs->packets); > - put_32aligned_u64(&stats->n_bytes, bs->bytes); > + bs_all = nl_attr_get_unspec(stats_attrs[TCA_STATS_BASIC], sizeof > *bs_all); > + if (stats_attrs[TCA_STATS_BASIC_HW]) { > + bs_hw = nl_attr_get_unspec(stats_attrs[TCA_STATS_BASIC_HW], > + sizeof *bs_hw); > + > + bs_sw.packets = bs_all->packets - bs_hw->packets; > + bs_sw.bytes = bs_all->bytes - bs_hw->bytes;
IIUC, there are kernels that supports some kind of TC offload, but has no support for TCA_STATS_BASIC_HW. If that right? In this case, I suppose, bs_hw will be NULL here and OVS will crash. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
