| Message ID | 20220125005521.1945935-1-mkp@redhat.com |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [ovs-dev,v2] tc: Add support for TCA_STATS_PKT64 | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
On 2022-01-25 2:55 AM, 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. This > patch also changes the non-empty check to use bytes, in case the 32bit > packet counter has rolled over for this update. > > Since v1: > - Retain support for pre-TCA_STATS_PKT64 kernels > > Fixes: f98e418fbd ("tc: Add tc flower functions") > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1776816 > Signed-off-by: Mike Pattrick <mkp@redhat.com> > --- > lib/tc.c | 16 +++++++++++++++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/lib/tc.c b/lib/tc.c > index 38a1dfc0e..c710e78e1 100644 > --- a/lib/tc.c > +++ b/lib/tc.c > @@ -1702,6 +1702,11 @@ static const struct nl_policy stats_policy[] = { > [TCA_STATS_BASIC] = { .type = NL_A_UNSPEC, > .min_len = sizeof(struct gnet_stats_basic), > .optional = false, }, > +#ifdef TCA_STATS_PKT64 I'm not but how does this ifdef works? In newer header TCA_STATS_PKT64 is an enum member, so ifdef shouldn't match ever. So I don't think you ever get inside here? I think you should have a compat file for gen_stats.h in include/linux/ look for example the pkt_cls.h In automate we look for police pktrate and define if it exists and in the compat file we check if its defined and use that header or a compat header and define what we need. then in the C file there are no ifdefs as you will always have the TCA_STATS_PKT64 enum. > + [TCA_STATS_PKT64] = { .type = NL_A_U64, > + .min_len = sizeof(uint64_t), > + .optional = true, }, > +#endif > }; > > static int > @@ -1772,8 +1777,17 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower, > } > > bs = nl_attr_get_unspec(stats_attrs[TCA_STATS_BASIC], sizeof *bs); > - if (bs->packets) { > + if (bs->bytes) { > +#ifdef TCA_STATS_PKT64 > + if (stats_attrs[TCA_STATS_PKT64]) { > + uint64_t packets = nl_attr_get_u64(stats_attrs[TCA_STATS_PKT64]); > + put_32aligned_u64(&stats->n_packets, packets); > + } else { > + put_32aligned_u64(&stats->n_packets, bs->packets); > + } > +#else > put_32aligned_u64(&stats->n_packets, bs->packets); > +#endif > put_32aligned_u64(&stats->n_bytes, bs->bytes); > } >
diff --git a/lib/tc.c b/lib/tc.c index 38a1dfc0e..c710e78e1 100644 --- a/lib/tc.c +++ b/lib/tc.c @@ -1702,6 +1702,11 @@ static const struct nl_policy stats_policy[] = { [TCA_STATS_BASIC] = { .type = NL_A_UNSPEC, .min_len = sizeof(struct gnet_stats_basic), .optional = false, }, +#ifdef TCA_STATS_PKT64 + [TCA_STATS_PKT64] = { .type = NL_A_U64, + .min_len = sizeof(uint64_t), + .optional = true, }, +#endif }; static int @@ -1772,8 +1777,17 @@ nl_parse_single_action(struct nlattr *action, struct tc_flower *flower, } bs = nl_attr_get_unspec(stats_attrs[TCA_STATS_BASIC], sizeof *bs); - if (bs->packets) { + if (bs->bytes) { +#ifdef TCA_STATS_PKT64 + if (stats_attrs[TCA_STATS_PKT64]) { + uint64_t packets = nl_attr_get_u64(stats_attrs[TCA_STATS_PKT64]); + put_32aligned_u64(&stats->n_packets, packets); + } else { + put_32aligned_u64(&stats->n_packets, bs->packets); + } +#else put_32aligned_u64(&stats->n_packets, bs->packets); +#endif put_32aligned_u64(&stats->n_bytes, bs->bytes); }
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. This patch also changes the non-empty check to use bytes, in case the 32bit packet counter has rolled over for this update. Since v1: - Retain support for pre-TCA_STATS_PKT64 kernels Fixes: f98e418fbd ("tc: Add tc flower functions") Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1776816 Signed-off-by: Mike Pattrick <mkp@redhat.com> --- lib/tc.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)