[PATCH net-next 0/3] net_sched: convert packet counters to 64bit

[PATCH net-next 0/3] net_sched: convert packet counters to 64bit

[Eric Dumazet]

This small patch series add 64bit support for packet counts.

Fact that the counters were still 32bit has been quite painful.

tc -s -d qd sh dev eth0 | head -3
qdisc mq 1: root 
 Sent 665706335338 bytes 6526520373 pkt (dropped 2441, overlimits 0 requeues 91) 
 backlog 0b 0p requeues 91


Eric Dumazet (3):
  net_sched: do not export gnet_stats_basic_packed to uapi
  net_sched: extend packet counter to 64bit
  net_sched: add TCA_STATS_PKT64 attribute

 include/net/gen_stats.h        |  6 ++++++
 include/uapi/linux/gen_stats.h |  5 +----
 net/core/gen_stats.c           | 12 ++++++++----
 net/sched/act_api.c            |  2 ++
 net/sched/act_simple.c         |  2 +-
 5 files changed, 18 insertions(+), 9 deletions(-)

-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH net-next 1/3] net_sched: do not export gnet_stats_basic_packed to uapi

[Eric Dumazet]

gnet_stats_basic_packed was really meant to be private kernel structure.

If this proves to be a problem, we will have to rename the in-kernel
version.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/net/gen_stats.h        | 6 ++++++
 include/uapi/linux/gen_stats.h | 4 ----
 2 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/include/net/gen_stats.h b/include/net/gen_stats.h
index ca23860adbb956fcfff3605068fdedf59073ce1a..5f3889e7ec1bb8b5148e9c552dd222b7f1c077d8 100644
--- a/include/net/gen_stats.h
+++ b/include/net/gen_stats.h
@@ -7,6 +7,12 @@
 #include <linux/rtnetlink.h>
 #include <linux/pkt_sched.h>
 
+/* Note: this used to be in include/uapi/linux/gen_stats.h */
+struct gnet_stats_basic_packed {
+	__u64	bytes;
+	__u32	packets;
+} __attribute__ ((packed));
+
 struct gnet_stats_basic_cpu {
 	struct gnet_stats_basic_packed bstats;
 	struct u64_stats_sync syncp;
diff --git a/include/uapi/linux/gen_stats.h b/include/uapi/linux/gen_stats.h
index 065408e16a807be59fec72ae7a9fec99b8fd383f..4eaacdf452e3b34f8f813046b801bfc1e6bdd2d4 100644
--- a/include/uapi/linux/gen_stats.h
+++ b/include/uapi/linux/gen_stats.h
@@ -26,10 +26,6 @@ struct gnet_stats_basic {
 	__u64	bytes;
 	__u32	packets;
 };
-struct gnet_stats_basic_packed {
-	__u64	bytes;
-	__u32	packets;
-} __attribute__ ((packed));
 
 /**
  * struct gnet_stats_rate_est - rate estimator
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH net-next 2/3] net_sched: extend packet counter to 64bit

[Eric Dumazet]

After this change, qdisc packet counter is no longer
a 32bit quantity. We still export 32bit values to user.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/net/gen_stats.h | 4 ++--
 net/core/gen_stats.c    | 3 +--
 net/sched/act_simple.c  | 2 +-
 3 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/include/net/gen_stats.h b/include/net/gen_stats.h
index 5f3889e7ec1bb8b5148e9c552dd222b7f1c077d8..1424e02cef90c0139a175933577f1b8537bce51a 100644
--- a/include/net/gen_stats.h
+++ b/include/net/gen_stats.h
@@ -10,8 +10,8 @@
 /* Note: this used to be in include/uapi/linux/gen_stats.h */
 struct gnet_stats_basic_packed {
 	__u64	bytes;
-	__u32	packets;
-} __attribute__ ((packed));
+	__u64	packets;
+};
 
 struct gnet_stats_basic_cpu {
 	struct gnet_stats_basic_packed bstats;
diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
index 36888f5e09eb9292eac289d80ced264d73040c50..fe33e2a9841e698dc1a0ac086086fa9832c0b514 100644
--- a/net/core/gen_stats.c
+++ b/net/core/gen_stats.c
@@ -123,8 +123,7 @@ __gnet_stats_copy_basic_cpu(struct gnet_stats_basic_packed *bstats,
 	for_each_possible_cpu(i) {
 		struct gnet_stats_basic_cpu *bcpu = per_cpu_ptr(cpu, i);
 		unsigned int start;
-		u64 bytes;
-		u32 packets;
+		u64 bytes, packets;
 
 		do {
 			start = u64_stats_fetch_begin_irq(&bcpu->syncp);
diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index 97639b259cd7d1995727385bf8c69a4ee4737fca..9813ca4006dd1de145a1ccdde5eb0a4217cf487e 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -35,7 +35,7 @@ static int tcf_simp_act(struct sk_buff *skb, const struct tc_action *a,
 	 * Example if this was the 3rd packet and the string was "hello"
 	 * then it would look like "hello_3" (without quotes)
 	 */
-	pr_info("simple: %s_%d\n",
+	pr_info("simple: %s_%llu\n",
 	       (char *)d->tcfd_defdata, d->tcf_bstats.packets);
 	spin_unlock(&d->tcf_lock);
 	return d->tcf_action;
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

[PATCH net-next 3/3] net_sched: add TCA_STATS_PKT64 attribute

[Eric Dumazet]

Now the kernel uses 64bit packet counters in scheduler layer,
we want to export these counters to user space.

Instead risking breaking user space by adding fields
to struct gnet_stats_basic, add a new TCA_STATS_PKT64.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 include/uapi/linux/gen_stats.h | 1 +
 net/core/gen_stats.c           | 9 +++++++--
 net/sched/act_api.c            | 2 ++
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/include/uapi/linux/gen_stats.h b/include/uapi/linux/gen_stats.h
index 4eaacdf452e3b34f8f813046b801bfc1e6bdd2d4..852f234f1fd634b5d4d5444fc8e2f2e45c4f0839 100644
--- a/include/uapi/linux/gen_stats.h
+++ b/include/uapi/linux/gen_stats.h
@@ -13,6 +13,7 @@ enum {
 	TCA_STATS_RATE_EST64,
 	TCA_STATS_PAD,
 	TCA_STATS_BASIC_HW,
+	TCA_STATS_PKT64,
 	__TCA_STATS_MAX,
 };
 #define TCA_STATS_MAX (__TCA_STATS_MAX - 1)
diff --git a/net/core/gen_stats.c b/net/core/gen_stats.c
index fe33e2a9841e698dc1a0ac086086fa9832c0b514..1d653fbfcf52a95f0c8acdeb1ce1b0b418177351 100644
--- a/net/core/gen_stats.c
+++ b/net/core/gen_stats.c
@@ -175,12 +175,17 @@ ___gnet_stats_copy_basic(const seqcount_t *running,
 
 	if (d->tail) {
 		struct gnet_stats_basic sb;
+		int res;
 
 		memset(&sb, 0, sizeof(sb));
 		sb.bytes = bstats.bytes;
 		sb.packets = bstats.packets;
-		return gnet_stats_copy(d, type, &sb, sizeof(sb),
-				       TCA_STATS_PAD);
+		res = gnet_stats_copy(d, type, &sb, sizeof(sb), TCA_STATS_PAD);
+		if (res < 0 || sb.packets == bstats.packets)
+			return res;
+		/* emit 64bit stats only if needed */
+		return gnet_stats_copy(d, TCA_STATS_PKT64, &bstats.packets,
+				       sizeof(bstats.packets), TCA_STATS_PAD);
 	}
 	return 0;
 }
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 6284c552e943d1ffa45d59a28eac2c37152e4875..bda1ba25c59e973709998b7e6279eee30589cbb8 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -188,6 +188,8 @@ static size_t tcf_action_shared_attrs_size(const struct tc_action *act)
 		+ nla_total_size(0) /* TCA_ACT_STATS nested */
 		/* TCA_STATS_BASIC */
 		+ nla_total_size_64bit(sizeof(struct gnet_stats_basic))
+		/* TCA_STATS_PKT64 */
+		+ nla_total_size_64bit(sizeof(u64))
 		/* TCA_STATS_QUEUE */
 		+ nla_total_size_64bit(sizeof(struct gnet_stats_queue))
 		+ nla_total_size(0) /* TCA_OPTIONS nested */
-- 
2.24.0.rc1.363.gb1bccd3e3d-goog

Re: [PATCH net-next 2/3] net_sched: extend packet counter to 64bit

[Jakub Kicinski]

On Mon,  4 Nov 2019 19:13:14 -0800, Eric Dumazet wrote:
> diff --git a/include/net/gen_stats.h b/include/net/gen_stats.h
> index 5f3889e7ec1bb8b5148e9c552dd222b7f1c077d8..1424e02cef90c0139a175933577f1b8537bce51a 100644
> --- a/include/net/gen_stats.h
> +++ b/include/net/gen_stats.h
> @@ -10,8 +10,8 @@
>  /* Note: this used to be in include/uapi/linux/gen_stats.h */
>  struct gnet_stats_basic_packed {
>  	__u64	bytes;
> -	__u32	packets;
> -} __attribute__ ((packed));
> +	__u64	packets;
> +};

nit: if there needs to be a respin for other reason perhaps worth
s/__u/u/ as this is no longer a uAPI structure?

Re: [PATCH net-next 2/3] net_sched: extend packet counter to 64bit

[Eric Dumazet]

On Tue, Nov 5, 2019 at 2:49 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Mon,  4 Nov 2019 19:13:14 -0800, Eric Dumazet wrote:
> > diff --git a/include/net/gen_stats.h b/include/net/gen_stats.h
> > index 5f3889e7ec1bb8b5148e9c552dd222b7f1c077d8..1424e02cef90c0139a175933577f1b8537bce51a 100644
> > --- a/include/net/gen_stats.h
> > +++ b/include/net/gen_stats.h
> > @@ -10,8 +10,8 @@
> >  /* Note: this used to be in include/uapi/linux/gen_stats.h */
> >  struct gnet_stats_basic_packed {
> >       __u64   bytes;
> > -     __u32   packets;
> > -} __attribute__ ((packed));
> > +     __u64   packets;
> > +};
>
> nit: if there needs to be a respin for other reason perhaps worth
> s/__u/u/ as this is no longer a uAPI structure?

Yes, I had this in my mind, we have __u32 all over the places, with a
mix of u32/__u32 in some classes :/

Thanks.

Re: [PATCH net-next 0/3] net_sched: convert packet counters to 64bit

[David Miller]

From: Eric Dumazet <edumazet@google.com>
Date: Mon,  4 Nov 2019 19:13:12 -0800

> This small patch series add 64bit support for packet counts.
> 
> Fact that the counters were still 32bit has been quite painful.
> 
> tc -s -d qd sh dev eth0 | head -3
> qdisc mq 1: root 
>  Sent 665706335338 bytes 6526520373 pkt (dropped 2441, overlimits 0 requeues 91) 
>  backlog 0b 0p requeues 91

Series applied.