[ovs-dev] [PATCH 2/2] tc: Fix stats byte count on fragmented packets

Ilya Maximets i.maximets at ovn.org
Mon Nov 29 22:47:22 UTC 2021


On 11/7/21 13:12, Roi Dayan via dev wrote:
> From: Paul Blakey <paulb at nvidia.com>
> 
> 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 <paulb at nvidia.com>
> Reviewed-by: Roi Dayan <roid at nvidia.com>
> ---
>  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.


More information about the dev mailing list