On 12/1/21 20:02, Paul Blakey wrote: > > > On Mon, 29 Nov 2021, Ilya Maximets wrote: > >> 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. >> > > Yes, but for such cases they will fail the check for "if > (stats_attrs[TCA_STATS_BASIC_HW]) {" and not access bs_hw. >
Hmm. You're right. I guess, I misread the if statement. Sorry for the noise. Bets regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
