On 21 Dec 2022, at 17:57, 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 <[email protected]>
Hi Mike,
I verified the test in the lab, and the patch is working :)
508584.717783128 revalidator3 ufid:a91bc377-8682-4600-a355-000075cc2379
[nl_parse_action_stats__:exit] sw = 000004018, hw = 0fff7a8fa
508587.719798295 revalidator3 ufid:a91bc377-8682-4600-a355-000075cc2379
[nl_parse_action_stats__:exit] sw = 000004018, hw = 10002284a
recirc_id(0),in_port(2),eth(src=42:78:36:f6:6f:6a,dst=36:c2:88:9e:19:b1),eth_type(0x0800),ipv4(frag=no),
packets:4318783052, bytes:259126622144, used:0.320s, actions:3
However, I do have a handful of style changes, which I put in a diff below.
Please sent out a v6, so I can ack it.
Cheers,
Eelco
diff --git a/lib/tc.c b/lib/tc.c
index 2c85ba9f2..5c9d792ab 100644
--- a/lib/tc.c
+++ b/lib/tc.c
@@ -1865,28 +1865,28 @@ nl_parse_action_stats(struct nlattr *act_stats,
{
struct tc_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;
unsigned int seen = 0;
- uint64_t packets;
size_t left;
- /* Cannot use nl_parse_nested due to duplicate attributes */
+ /* 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)) {
const struct gnet_stats_basic *stats_basic;
+ const struct gnet_stats_queue *qs;
uint16_t type = nl_attr_type(nla);
+
seen |= 1 << type;
switch (type) {
case TCA_STATS_BASIC:
- stats_basic = nl_attr_get_unspec(nla, sizeof *stats_basic);
+ stats_basic = nl_attr_get_unspec(nla, sizeof *stats_basic);
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);
+ stats_basic = nl_attr_get_unspec(nla, sizeof *stats_basic);
s_hw.n_packets = get_unaligned_u32(&stats_basic->packets);
s_hw.n_bytes = get_unaligned_u64(&stats_basic->bytes);
break;
@@ -1895,7 +1895,7 @@ nl_parse_action_stats(struct nlattr *act_stats,
s_dropped = get_unaligned_u32(&qs->drops);
break;
case TCA_STATS_PKT64:
- packets = nl_attr_get_u64(nla);
+ uint64_t packets = nl_attr_get_u64(nla);
if (prev_type == TCA_STATS_BASIC) {
s_sw.n_packets = packets;
@@ -1921,9 +1921,10 @@ nl_parse_action_stats(struct nlattr *act_stats,
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);
+ 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);
> ---
>
> Since v1:
> - Retain support for pre-TCA_STATS_PKT64 kernels
> Since v2:
> - Added compat header
> Since v3:
> - Rebased on to current master
> Since v4:
> - Fixed alignment issue
> - Moved declarations and definitions
>
> ---
> lib/tc.c | 108 ++++++++++++++++++++++++++++++++++---------------------
> 1 file changed, 67 insertions(+), 41 deletions(-)
>
> diff --git a/lib/tc.c b/lib/tc.c
> index a66dc432f..2c85ba9f2 100644
> --- a/lib/tc.c
> +++ b/lib/tc.c
> @@ -84,6 +84,11 @@ struct flower_key_to_pedit {
> int boundary_shift;
> };
>
> +struct tc_flow_stats {
> + uint64_t n_packets;
> + uint64_t n_bytes;
> +};
> +
> static struct flower_key_to_pedit flower_pedit_map[] = {
> {
> TCA_PEDIT_KEY_EX_HDR_TYPE_IP4,
> @@ -1852,66 +1857,87 @@ 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, },
> -};
> -
> static int
> nl_parse_action_stats(struct nlattr *act_stats,
> struct ovs_flow_stats *stats_sw,
> 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;
> + struct tc_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;
> + unsigned int seen = 0;
> + uint64_t packets;
> + 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)) {
> + 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 = 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 = 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 = get_unaligned_u32(&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
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
