On 2/23/22 09:20, Roi Dayan wrote: > > > On 2021-12-01 9:23 PM, Ilya Maximets wrote: >> 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. > > Hi Ilya, > > Any other comment on this patch?
Hmm. For some reason this patch was marked as 'changes requested' (I guess I forgot to switch the state back), sorry about that. I'll take another look. best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
