[ovs-dev,v4] tc: Add support for TCA_STATS_PKT64

Currently tc offload flow packet counters will roll over every ~4
billion packets. This is because the packet counter in struct
tc_stats provided by TCA_STATS_BASIC is a 32bit integer.

Now we check for the optional TCA_STATS_PKT64 attribute which provides
the full 64bit packet counter if the 32bit one has rolled over. Because
the TCA_STATS_PKT64 attribute may appear multiple times in a netlink
message, the method of parsing attributes was changed.

Fixes: f98e418fbdb6 ("tc: Add tc flower functions")
Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1776816
Signed-off-by: Mike Pattrick <mkp@redhat.com>

---

Since v1:
 - Retain support for pre-TCA_STATS_PKT64 kernels
Since v2:
 - Added compat header
Since v3:
- Rebased on to current master

Signed-off-by: Mike Pattrick <mkp@redhat.com>
---
 lib/tc.c | 105 ++++++++++++++++++++++++++++++++++---------------------
 1 file changed, 66 insertions(+), 39 deletions(-)

On 21 Dec 2022, at 3:12, Mike Pattrick wrote:

> Currently tc offload flow packet counters will roll over every ~4
> billion packets. This is because the packet counter in struct
> tc_stats provided by TCA_STATS_BASIC is a 32bit integer.
>
> Now we check for the optional TCA_STATS_PKT64 attribute which provides
> the full 64bit packet counter if the 32bit one has rolled over. Because
> the TCA_STATS_PKT64 attribute may appear multiple times in a netlink
> message, the method of parsing attributes was changed.
>
> Fixes: f98e418fbdb6 ("tc: Add tc flower functions")
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1776816
> Signed-off-by: Mike Pattrick <mkp@redhat.com>
>
> ---
>
> Since v1:
>  - Retain support for pre-TCA_STATS_PKT64 kernels
> Since v2:
>  - Added compat header
> Since v3:
> - Rebased on to current master
>
> Signed-off-by: Mike Pattrick <mkp@redhat.com>
> ---
>  lib/tc.c | 105 ++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 66 insertions(+), 39 deletions(-)
>
> diff --git a/lib/tc.c b/lib/tc.c
> index a66dc432f..56a83e2c4 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -1852,16 +1852,9 @@ static const struct nl_policy act_policy[] = {
>      [TCA_ACT_STATS] = { .type = NL_A_NESTED, .optional = false, },
>  };
>
> -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, },
> -    [TCA_STATS_QUEUE] = { .type = NL_A_UNSPEC,
> -                          .min_len = sizeof(struct gnet_stats_queue),
> -                          .optional = true, },
> +struct flow_stats {
> +    uint64_t n_packets;
> +    uint64_t n_bytes;
>  };
>
>  static int
> @@ -1870,48 +1863,82 @@ nl_parse_action_stats(struct nlattr *act_stats,
>                        struct ovs_flow_stats *stats_hw,
>                        struct ovs_flow_stats *stats_dropped)
>  {
> -    struct nlattr *stats_attrs[ARRAY_SIZE(stats_policy)];
> -    struct gnet_stats_basic bs_all, bs_sw, bs_hw;
> +    const struct gnet_stats_basic *stats_basic;
> +    struct flow_stats s_sw = {0}, s_hw = {0};
> +    uint16_t prev_type = __TCA_STATS_MAX;
>      const struct gnet_stats_queue *qs;
> +    const struct nlattr *nla;
> +    uint32_t s_dropped = 0;
> +    uint64_t packets;
> +    uint16_t type;
> +    int seen = 0;
> +    size_t left;
>
> -    if (!nl_parse_nested(act_stats, stats_policy, stats_attrs,
> -                         ARRAY_SIZE(stats_policy))) {
> -        VLOG_ERR_RL(&error_rl, "Failed to parse action stats policy");
> -        return EPROTO;
> -    }
> +    /* Cannot use nl_parse_nested due to duplicate attributes */
> +    NL_ATTR_FOR_EACH (nla, left, nl_attr_get(act_stats),
> +                      nl_attr_get_size(act_stats)) {
> +        type = nl_attr_type(nla);
> +        seen |= 1 << type;
>
> -    memcpy(&bs_all,
> -           nl_attr_get_unspec(stats_attrs[TCA_STATS_BASIC], sizeof bs_all),
> -           sizeof bs_all);
> -    if (stats_attrs[TCA_STATS_BASIC_HW]) {
> -        memcpy(&bs_hw, nl_attr_get_unspec(stats_attrs[TCA_STATS_BASIC_HW],
> -                                          sizeof bs_hw),
> -               sizeof bs_hw);
> +        switch (type) {
> +            case TCA_STATS_BASIC:
> +                stats_basic =  nl_attr_get_unspec(nla, sizeof *stats_basic);
> +                s_sw.n_packets = stats_basic->packets;
> +                s_sw.n_bytes = stats_basic->bytes;

I did not review the patch yet, but just noticed you are just assuming the stats_basic is aligned, but they are not :)
That’s why the memcpy was there, so you might need to use it again, or do something like get_unaligned_u64()/u32()

> +                break;
> +            case TCA_STATS_BASIC_HW:
> +                stats_basic =  nl_attr_get_unspec(nla, sizeof *stats_basic);
> +                s_hw.n_packets = stats_basic->packets;
> +                s_hw.n_bytes = stats_basic->bytes;
> +                break;
> +            case TCA_STATS_QUEUE:
> +                qs = nl_attr_get_unspec(nla, sizeof *qs);
> +                s_dropped = qs->drops;
> +                break;
> +            case TCA_STATS_PKT64:
> +                packets = nl_attr_get_u64(nla);
>
> -        bs_sw.packets = bs_all.packets - bs_hw.packets;
> -        bs_sw.bytes = bs_all.bytes - bs_hw.bytes;
> -    } else {
> -        bs_sw.packets = bs_all.packets;
> -        bs_sw.bytes = bs_all.bytes;
> +                if (prev_type == TCA_STATS_BASIC) {
> +                    s_sw.n_packets = packets;
> +                } else if (prev_type == TCA_STATS_BASIC_HW) {
> +                    s_hw.n_packets = packets;
> +                } else {
> +                    goto err;
> +                }
> +                break;
> +            default:
> +                break;
> +        }
> +        prev_type = type;
>      }
>
> -    if (bs_sw.packets > get_32aligned_u64(&stats_sw->n_packets)) {
> -        put_32aligned_u64(&stats_sw->n_packets, bs_sw.packets);
> -        put_32aligned_u64(&stats_sw->n_bytes, bs_sw.bytes);
> +    if (!(seen & 1 << TCA_STATS_BASIC)) {
> +        goto err;
>      }
>
> -    if (stats_attrs[TCA_STATS_BASIC_HW]
> -        && bs_hw.packets > get_32aligned_u64(&stats_hw->n_packets)) {
> -        put_32aligned_u64(&stats_hw->n_packets, bs_hw.packets);
> -        put_32aligned_u64(&stats_hw->n_bytes, bs_hw.bytes);
> +    if (seen & 1 << TCA_STATS_BASIC_HW) {
> +        s_sw.n_packets = s_sw.n_packets - s_hw.n_packets;
> +        s_sw.n_bytes = s_sw.n_bytes - s_hw.n_bytes;
> +
> +        if (s_hw.n_packets > get_32aligned_u64(&stats_hw->n_packets)) {
> +            put_32aligned_u64(&stats_hw->n_packets, s_hw.n_packets);
> +            put_32aligned_u64(&stats_hw->n_bytes,  s_hw.n_bytes);
> +        }
> +    }
> +    if (s_sw.n_packets > get_32aligned_u64(&stats_sw->n_packets)) {
> +        put_32aligned_u64(&stats_sw->n_packets, s_sw.n_packets);
> +        put_32aligned_u64(&stats_sw->n_bytes, s_sw.n_bytes);
>      }
>
> -    if (stats_dropped && stats_attrs[TCA_STATS_QUEUE]) {
> -        qs = nl_attr_get_unspec(stats_attrs[TCA_STATS_QUEUE], sizeof *qs);
> -        put_32aligned_u64(&stats_dropped->n_packets, qs->drops);
> +    if (stats_dropped && (seen & 1 << TCA_STATS_QUEUE)) {
> +        put_32aligned_u64(&stats_dropped->n_packets, s_dropped);
>      }
>
>      return 0;
> +
> +err:
> +    VLOG_ERR_RL(&error_rl, "Failed to parse action stats policy");
> +    return EPROTO;
>  }
>
>  static int
> -- 
> 2.31.1

On 21 Dec 2022, at 3:12, Mike Pattrick wrote:

> Currently tc offload flow packet counters will roll over every ~4
> billion packets. This is because the packet counter in struct
> tc_stats provided by TCA_STATS_BASIC is a 32bit integer.
>
> Now we check for the optional TCA_STATS_PKT64 attribute which provides
> the full 64bit packet counter if the 32bit one has rolled over. Because
> the TCA_STATS_PKT64 attribute may appear multiple times in a netlink
> message, the method of parsing attributes was changed.
>
> Fixes: f98e418fbdb6 ("tc: Add tc flower functions")
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1776816
> Signed-off-by: Mike Pattrick <mkp@redhat.com>
>
> ---

Decided to do a full review to speed things up. So some additional comments are below.

> Since v1:
>  - Retain support for pre-TCA_STATS_PKT64 kernels
> Since v2:
>  - Added compat header
> Since v3:
> - Rebased on to current master
>
> Signed-off-by: Mike Pattrick <mkp@redhat.com>
> ---
>  lib/tc.c | 105 ++++++++++++++++++++++++++++++++++---------------------
>  1 file changed, 66 insertions(+), 39 deletions(-)
>
> diff --git a/lib/tc.c b/lib/tc.c
> index a66dc432f..56a83e2c4 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -1852,16 +1852,9 @@ static const struct nl_policy act_policy[] = {
>      [TCA_ACT_STATS] = { .type = NL_A_NESTED, .optional = false, },
>  };
>
> -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, },
> -    [TCA_STATS_QUEUE] = { .type = NL_A_UNSPEC,
> -                          .min_len = sizeof(struct gnet_stats_queue),
> -                          .optional = true, },
> +struct flow_stats {
> +    uint64_t n_packets;
> +    uint64_t n_bytes;
>  };

You can use the existing ovs_flow_stats structure for this? Well, I tried it and it looks like we re-write the type definition :(

Can we at least call it tc_flow_stats and move it to the top of the file with the other definitions?

>  static int
> @@ -1870,48 +1863,82 @@ nl_parse_action_stats(struct nlattr *act_stats,
>                        struct ovs_flow_stats *stats_hw,
>                        struct ovs_flow_stats *stats_dropped)
>  {
> -    struct nlattr *stats_attrs[ARRAY_SIZE(stats_policy)];
> -    struct gnet_stats_basic bs_all, bs_sw, bs_hw;
> +    const struct gnet_stats_basic *stats_basic;

This can move to the loop

> +    struct flow_stats s_sw = {0}, s_hw = {0};

struct ovs_flow_stats s_sw = {0}, s_hw = {0};

> +    uint16_t prev_type = __TCA_STATS_MAX;
>      const struct gnet_stats_queue *qs;
> +    const struct nlattr *nla;
> +    uint32_t s_dropped = 0;
> +    uint64_t packets;
> +    uint16_t type;

Remove type here, and define it in the loop.

> +    int seen = 0;

As we use it as a bitmap, do we want to make it unsigned?

> +    size_t left;
>
> -    if (!nl_parse_nested(act_stats, stats_policy, stats_attrs,
> -                         ARRAY_SIZE(stats_policy))) {
> -        VLOG_ERR_RL(&error_rl, "Failed to parse action stats policy");
> -        return EPROTO;
> -    }
> +    /* Cannot use nl_parse_nested due to duplicate attributes */
> +    NL_ATTR_FOR_EACH (nla, left, nl_attr_get(act_stats),
> +                      nl_attr_get_size(act_stats)) {
> +        type = nl_attr_type(nla);
> +        seen |= 1 << type;

        const struct gnet_stats_basic *stats_basic;
        uint16_t type = nl_attr_type(nla);

        seen |= 1 << type;

>
> -    memcpy(&bs_all,
> -           nl_attr_get_unspec(stats_attrs[TCA_STATS_BASIC], sizeof bs_all),
> -           sizeof bs_all);
> -    if (stats_attrs[TCA_STATS_BASIC_HW]) {
> -        memcpy(&bs_hw, nl_attr_get_unspec(stats_attrs[TCA_STATS_BASIC_HW],
> -                                          sizeof bs_hw),
> -               sizeof bs_hw);
> +        switch (type) {
> +            case TCA_STATS_BASIC:
> +                stats_basic =  nl_attr_get_unspec(nla, sizeof *stats_basic);
> +                s_sw.n_packets = stats_basic->packets;
> +                s_sw.n_bytes = stats_basic->bytes;

                s_sw.n_packets = get_unaligned_u32(&stats_basic->packets);
                s_sw.n_bytes = get_unaligned_u64(&stats_basic->bytes);

> +                break;
> +            case TCA_STATS_BASIC_HW:
> +                stats_basic =  nl_attr_get_unspec(nla, sizeof *stats_basic);
> +                s_hw.n_packets = stats_basic->packets;
> +                s_hw.n_bytes = stats_basic->bytes;

                s_hw.n_packets = get_unaligned_u32(&stats_basic->packets);
                s_hw.n_bytes = get_unaligned_u64(&stats_basic->bytes);

> +                break;
> +            case TCA_STATS_QUEUE:
> +                qs = nl_attr_get_unspec(nla, sizeof *qs);
> +                s_dropped = qs->drops;
> +                break;

I think we can do the qa assignment in one go? Something like:

                if (stats_dropped) {
                    const struct gnet_stats_queue *qs;

                    qs = nl_attr_get_unspec(nla, sizeof *qs);
                    put_32aligned_u64(&stats_dropped->n_packets,
                                      get_unaligned_u32(&qs->drops));
                }
                break;

> +            case TCA_STATS_PKT64:
> +                packets = nl_attr_get_u64(nla);
                   uint64_t packets = nl_attr_get_u64(nla);

>
> -        bs_sw.packets = bs_all.packets - bs_hw.packets;
> -        bs_sw.bytes = bs_all.bytes - bs_hw.bytes;
> -    } else {
> -        bs_sw.packets = bs_all.packets;
> -        bs_sw.bytes = bs_all.bytes;
> +                if (prev_type == TCA_STATS_BASIC) {
> +                    s_sw.n_packets = packets;
> +                } else if (prev_type == TCA_STATS_BASIC_HW) {
> +                    s_hw.n_packets = packets;
> +                } else {
> +                    goto err;
> +                }
> +                break;
> +            default:
> +                break;
> +        }
> +        prev_type = type;
>      }
>
> -    if (bs_sw.packets > get_32aligned_u64(&stats_sw->n_packets)) {
> -        put_32aligned_u64(&stats_sw->n_packets, bs_sw.packets);
> -        put_32aligned_u64(&stats_sw->n_bytes, bs_sw.bytes);
> +    if (!(seen & 1 << TCA_STATS_BASIC)) {
> +        goto err;
>      }
>
> -    if (stats_attrs[TCA_STATS_BASIC_HW]
> -        && bs_hw.packets > get_32aligned_u64(&stats_hw->n_packets)) {
> -        put_32aligned_u64(&stats_hw->n_packets, bs_hw.packets);
> -        put_32aligned_u64(&stats_hw->n_bytes, bs_hw.bytes);
> +    if (seen & 1 << TCA_STATS_BASIC_HW) {
> +        s_sw.n_packets = s_sw.n_packets - s_hw.n_packets;
> +        s_sw.n_bytes = s_sw.n_bytes - s_hw.n_bytes;
> +
> +        if (s_hw.n_packets > get_32aligned_u64(&stats_hw->n_packets)) {
> +            put_32aligned_u64(&stats_hw->n_packets, s_hw.n_packets);
> +            put_32aligned_u64(&stats_hw->n_bytes,  s_hw.n_bytes);
> +        }
> +    }
> +    if (s_sw.n_packets > get_32aligned_u64(&stats_sw->n_packets)) {
> +        put_32aligned_u64(&stats_sw->n_packets, s_sw.n_packets);
> +        put_32aligned_u64(&stats_sw->n_bytes, s_sw.n_bytes);
>      }
>
> -    if (stats_dropped && stats_attrs[TCA_STATS_QUEUE]) {
> -        qs = nl_attr_get_unspec(stats_attrs[TCA_STATS_QUEUE], sizeof *qs);
> -        put_32aligned_u64(&stats_dropped->n_packets, qs->drops);
> +    if (stats_dropped && (seen & 1 << TCA_STATS_QUEUE)) {
> +        put_32aligned_u64(&stats_dropped->n_packets, s_dropped);
>      }
>
>      return 0;
> +
> +err:
> +    VLOG_ERR_RL(&error_rl, "Failed to parse action stats policy");
> +    return EPROTO;
>  }
>
>  static int
> -- 
> 2.31.1