[RFC PATCH 0/3] gro: some minor optimizations

[RFC PATCH 0/3] gro: some minor optimizations

[Paolo Abeni]

This series collects a few small optimization for the GRO engine.
I measure a 10% performance improvements in micro-benchmarks
around dev_gro_receive(), but deltas are within noise range in tput
tests - ence the RFC tag.

Any feedback more then welcome.

Paolo Abeni (3):
  net: gro: avoid re-computing truesize twice on recycle
  net: gro: minor optimization for dev_gro_receive()
  net: gro: register gso and gro offload on separate lists

 include/linux/netdevice.h |   3 +-
 include/net/gro.h         |  13 +++--
 net/core/gro.c            | 107 +++++++++++++++++++++-----------------
 3 files changed, 70 insertions(+), 53 deletions(-)

-- 
2.34.1

[RFC PATCH 1/3] net: gro: avoid re-computing truesize twice on recycle

[Paolo Abeni]

After commit 5e10da5385d2 ("skbuff: allow 'slow_gro' for skb
carring sock reference") and commit af352460b465 ("net: fix GRO
skb truesize update") the truesize of freed skb is properly updated
by the GRO engine, we don't need anymore resetting it at recycle time.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 net/core/gro.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/net/core/gro.c b/net/core/gro.c
index a11b286d1495..d43d42215bdb 100644
--- a/net/core/gro.c
+++ b/net/core/gro.c
@@ -634,7 +634,6 @@ static void napi_reuse_skb(struct napi_struct *napi, struct sk_buff *skb)
 
 	skb->encapsulation = 0;
 	skb_shinfo(skb)->gso_type = 0;
-	skb->truesize = SKB_TRUESIZE(skb_end_offset(skb));
 	if (unlikely(skb->slow_gro)) {
 		skb_orphan(skb);
 		skb_ext_reset(skb);
-- 
2.34.1

[RFC PATCH 2/3] net: gro: minor optimization for dev_gro_receive()

[Paolo Abeni]

While inspecting some perf report, I noticed that the compiler
emits suboptimal code for the napi CB initialization, fetching
and storing multiple times the memory for flags bitfield.
This is with gcc 10.3.1, but I observed the same with older compiler
versions.

We can help the compiler to do a nicer work e.g. initially setting
all the bitfield to 0 using an u16 alias. The generated code is quite
smaller, with the same number of conditional

Before:
objdump -t net/core/gro.o | grep " F .text"
0000000000000bb0 l     F .text	0000000000000357 dev_gro_receive

After:
0000000000000bb0 l     F .text	000000000000033c dev_gro_receive

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 include/net/gro.h | 13 +++++++++----
 net/core/gro.c    | 16 +++++-----------
 2 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/include/net/gro.h b/include/net/gro.h
index 8f75802d50fd..a068b27d341f 100644
--- a/include/net/gro.h
+++ b/include/net/gro.h
@@ -29,14 +29,17 @@ struct napi_gro_cb {
 	/* Number of segments aggregated. */
 	u16	count;
 
-	/* Start offset for remote checksum offload */
-	u16	gro_remcsum_start;
+	/* Used in ipv6_gro_receive() and foo-over-udp */
+	u16	proto;
 
 	/* jiffies when first packet was created/queued */
 	unsigned long age;
 
-	/* Used in ipv6_gro_receive() and foo-over-udp */
-	u16	proto;
+	/* portion of the cb set to zero at every gro iteration */
+	u32	zeroed_start[0];
+
+	/* Start offset for remote checksum offload */
+	u16	gro_remcsum_start;
 
 	/* This is non-zero if the packet may be of the same flow. */
 	u8	same_flow:1;
@@ -70,6 +73,8 @@ struct napi_gro_cb {
 	/* GRO is done by frag_list pointer chaining. */
 	u8	is_flist:1;
 
+	u32	zeroed_end[0];
+
 	/* used to support CHECKSUM_COMPLETE for tunneling protocols */
 	__wsum	csum;
 
diff --git a/net/core/gro.c b/net/core/gro.c
index d43d42215bdb..b9ebe9298731 100644
--- a/net/core/gro.c
+++ b/net/core/gro.c
@@ -435,6 +435,9 @@ static void gro_flush_oldest(struct napi_struct *napi, struct list_head *head)
 	napi_gro_complete(napi, oldest);
 }
 
+#define zeroed_len	(offsetof(struct napi_gro_cb, zeroed_end) - \
+			 offsetof(struct napi_gro_cb, zeroed_start))
+
 static enum gro_result dev_gro_receive(struct napi_struct *napi, struct sk_buff *skb)
 {
 	u32 bucket = skb_get_hash_raw(skb) & (GRO_HASH_BUCKETS - 1);
@@ -459,29 +462,20 @@ static enum gro_result dev_gro_receive(struct napi_struct *napi, struct sk_buff
 
 		skb_set_network_header(skb, skb_gro_offset(skb));
 		skb_reset_mac_len(skb);
-		NAPI_GRO_CB(skb)->same_flow = 0;
+		BUILD_BUG_ON(zeroed_len != sizeof(NAPI_GRO_CB(skb)->zeroed_start[0]));
+		NAPI_GRO_CB(skb)->zeroed_start[0] = 0;
 		NAPI_GRO_CB(skb)->flush = skb_is_gso(skb) || skb_has_frag_list(skb);
-		NAPI_GRO_CB(skb)->free = 0;
-		NAPI_GRO_CB(skb)->encap_mark = 0;
-		NAPI_GRO_CB(skb)->recursion_counter = 0;
-		NAPI_GRO_CB(skb)->is_fou = 0;
 		NAPI_GRO_CB(skb)->is_atomic = 1;
-		NAPI_GRO_CB(skb)->gro_remcsum_start = 0;
 
 		/* Setup for GRO checksum validation */
 		switch (skb->ip_summed) {
 		case CHECKSUM_COMPLETE:
 			NAPI_GRO_CB(skb)->csum = skb->csum;
 			NAPI_GRO_CB(skb)->csum_valid = 1;
-			NAPI_GRO_CB(skb)->csum_cnt = 0;
 			break;
 		case CHECKSUM_UNNECESSARY:
 			NAPI_GRO_CB(skb)->csum_cnt = skb->csum_level + 1;
-			NAPI_GRO_CB(skb)->csum_valid = 0;
 			break;
-		default:
-			NAPI_GRO_CB(skb)->csum_cnt = 0;
-			NAPI_GRO_CB(skb)->csum_valid = 0;
 		}
 
 		pp = INDIRECT_CALL_INET(ptype->callbacks.gro_receive,
-- 
2.34.1

[RFC PATCH 3/3] net: gro: register gso and gro offload on separate lists

[Paolo Abeni]

So that we know each element in gro_list has valid gro callbacks
(and the same for gso). This allows dropping a bunch of conditional
in fastpath.

Before:
objdump -t net/core/gro.o | grep " F .text"
0000000000000bb0 l     F .text  000000000000033c dev_gro_receive

After:
0000000000000bb0 l     F .text  0000000000000325 dev_gro_receive

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 include/linux/netdevice.h |  3 +-
 net/core/gro.c            | 90 +++++++++++++++++++++++----------------
 2 files changed, 56 insertions(+), 37 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 3213c7227b59..406cb457d788 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -2564,7 +2564,8 @@ struct packet_offload {
 	__be16			 type;	/* This is really htons(ether_type). */
 	u16			 priority;
 	struct offload_callbacks callbacks;
-	struct list_head	 list;
+	struct list_head	 gro_list;
+	struct list_head	 gso_list;
 };
 
 /* often modified stats are per-CPU, other are shared (netdev->stats) */
diff --git a/net/core/gro.c b/net/core/gro.c
index b9ebe9298731..5d7bc6813a7d 100644
--- a/net/core/gro.c
+++ b/net/core/gro.c
@@ -10,10 +10,21 @@
 #define GRO_MAX_HEAD (MAX_HEADER + 128)
 
 static DEFINE_SPINLOCK(offload_lock);
-static struct list_head offload_base __read_mostly = LIST_HEAD_INIT(offload_base);
+static struct list_head gro_offload_base __read_mostly = LIST_HEAD_INIT(gro_offload_base);
+static struct list_head gso_offload_base __read_mostly = LIST_HEAD_INIT(gso_offload_base);
 /* Maximum number of GRO_NORMAL skbs to batch up for list-RX */
 int gro_normal_batch __read_mostly = 8;
 
+#define offload_list_insert(head, poff, list)		\
+({							\
+	struct packet_offload *elem;			\
+	list_for_each_entry(elem, head, list) {		\
+		if ((poff)->priority < elem->priority)	\
+			break;				\
+	}						\
+	list_add_rcu(&(poff)->list, elem->list.prev);	\
+})
+
 /**
  *	dev_add_offload - register offload handlers
  *	@po: protocol offload declaration
@@ -28,18 +39,33 @@ int gro_normal_batch __read_mostly = 8;
  */
 void dev_add_offload(struct packet_offload *po)
 {
-	struct packet_offload *elem;
-
 	spin_lock(&offload_lock);
-	list_for_each_entry(elem, &offload_base, list) {
-		if (po->priority < elem->priority)
-			break;
-	}
-	list_add_rcu(&po->list, elem->list.prev);
+	if (po->callbacks.gro_receive && po->callbacks.gro_complete)
+		offload_list_insert(&gro_offload_base, po, gro_list);
+	else if (po->callbacks.gro_complete)
+		pr_warn("missing gro_receive callback");
+	else if (po->callbacks.gro_receive)
+		pr_warn("missing gro_complete callback");
+
+	if (po->callbacks.gso_segment)
+		offload_list_insert(&gso_offload_base, po, gso_list);
 	spin_unlock(&offload_lock);
 }
 EXPORT_SYMBOL(dev_add_offload);
 
+#define offload_list_remove(type, head, poff, list)	\
+({							\
+	struct packet_offload *elem;			\
+	list_for_each_entry(elem, head, list) {		\
+		if ((poff) == elem) {			\
+			list_del_rcu(&(poff)->list);	\
+			break;				\
+		}					\
+	}						\
+	if (elem != (poff))				\
+		pr_warn("dev_remove_offload: %p not found in %s list\n", (poff), type); \
+})
+
 /**
  *	__dev_remove_offload	 - remove offload handler
  *	@po: packet offload declaration
@@ -55,20 +81,12 @@ EXPORT_SYMBOL(dev_add_offload);
  */
 static void __dev_remove_offload(struct packet_offload *po)
 {
-	struct list_head *head = &offload_base;
-	struct packet_offload *po1;
-
 	spin_lock(&offload_lock);
+	if (po->callbacks.gro_receive)
+		offload_list_remove("gro", &gro_offload_base, po, gso_list);
 
-	list_for_each_entry(po1, head, list) {
-		if (po == po1) {
-			list_del_rcu(&po->list);
-			goto out;
-		}
-	}
-
-	pr_warn("dev_remove_offload: %p not found\n", po);
-out:
+	if (po->callbacks.gso_segment)
+		offload_list_remove("gso", &gso_offload_base, po, gro_list);
 	spin_unlock(&offload_lock);
 }
 
@@ -111,8 +129,8 @@ struct sk_buff *skb_mac_gso_segment(struct sk_buff *skb,
 	__skb_pull(skb, vlan_depth);
 
 	rcu_read_lock();
-	list_for_each_entry_rcu(ptype, &offload_base, list) {
-		if (ptype->type == type && ptype->callbacks.gso_segment) {
+	list_for_each_entry_rcu(ptype, &gso_offload_base, gso_list) {
+		if (ptype->type == type) {
 			segs = ptype->callbacks.gso_segment(skb, features);
 			break;
 		}
@@ -250,7 +268,7 @@ static void napi_gro_complete(struct napi_struct *napi, struct sk_buff *skb)
 {
 	struct packet_offload *ptype;
 	__be16 type = skb->protocol;
-	struct list_head *head = &offload_base;
+	struct list_head *head = &gro_offload_base;
 	int err = -ENOENT;
 
 	BUILD_BUG_ON(sizeof(struct napi_gro_cb) > sizeof(skb->cb));
@@ -261,8 +279,8 @@ static void napi_gro_complete(struct napi_struct *napi, struct sk_buff *skb)
 	}
 
 	rcu_read_lock();
-	list_for_each_entry_rcu(ptype, head, list) {
-		if (ptype->type != type || !ptype->callbacks.gro_complete)
+	list_for_each_entry_rcu(ptype, head, gro_list) {
+		if (ptype->type != type)
 			continue;
 
 		err = INDIRECT_CALL_INET(ptype->callbacks.gro_complete,
@@ -273,7 +291,7 @@ static void napi_gro_complete(struct napi_struct *napi, struct sk_buff *skb)
 	rcu_read_unlock();
 
 	if (err) {
-		WARN_ON(&ptype->list == head);
+		WARN_ON(&ptype->gro_list == head);
 		kfree_skb(skb);
 		return;
 	}
@@ -442,7 +460,7 @@ static enum gro_result dev_gro_receive(struct napi_struct *napi, struct sk_buff
 {
 	u32 bucket = skb_get_hash_raw(skb) & (GRO_HASH_BUCKETS - 1);
 	struct gro_list *gro_list = &napi->gro_hash[bucket];
-	struct list_head *head = &offload_base;
+	struct list_head *head = &gro_offload_base;
 	struct packet_offload *ptype;
 	__be16 type = skb->protocol;
 	struct sk_buff *pp = NULL;
@@ -456,8 +474,8 @@ static enum gro_result dev_gro_receive(struct napi_struct *napi, struct sk_buff
 	gro_list_prepare(&gro_list->list, skb);
 
 	rcu_read_lock();
-	list_for_each_entry_rcu(ptype, head, list) {
-		if (ptype->type != type || !ptype->callbacks.gro_receive)
+	list_for_each_entry_rcu(ptype, head, gro_list) {
+		if (ptype->type != type)
 			continue;
 
 		skb_set_network_header(skb, skb_gro_offset(skb));
@@ -485,7 +503,7 @@ static enum gro_result dev_gro_receive(struct napi_struct *napi, struct sk_buff
 	}
 	rcu_read_unlock();
 
-	if (&ptype->list == head)
+	if (&ptype->gro_list == head)
 		goto normal;
 
 	if (PTR_ERR(pp) == -EINPROGRESS) {
@@ -541,11 +559,11 @@ static enum gro_result dev_gro_receive(struct napi_struct *napi, struct sk_buff
 
 struct packet_offload *gro_find_receive_by_type(__be16 type)
 {
-	struct list_head *offload_head = &offload_base;
+	struct list_head *offload_head = &gro_offload_base;
 	struct packet_offload *ptype;
 
-	list_for_each_entry_rcu(ptype, offload_head, list) {
-		if (ptype->type != type || !ptype->callbacks.gro_receive)
+	list_for_each_entry_rcu(ptype, offload_head, gro_list) {
+		if (ptype->type != type)
 			continue;
 		return ptype;
 	}
@@ -555,11 +573,11 @@ EXPORT_SYMBOL(gro_find_receive_by_type);
 
 struct packet_offload *gro_find_complete_by_type(__be16 type)
 {
-	struct list_head *offload_head = &offload_base;
+	struct list_head *offload_head = &gro_offload_base;
 	struct packet_offload *ptype;
 
-	list_for_each_entry_rcu(ptype, offload_head, list) {
-		if (ptype->type != type || !ptype->callbacks.gro_complete)
+	list_for_each_entry_rcu(ptype, offload_head, gro_list) {
+		if (ptype->type != type)
 			continue;
 		return ptype;
 	}
-- 
2.34.1

Re: [RFC PATCH 2/3] net: gro: minor optimization for dev_gro_receive()

[Alexander Lobakin]

From: Paolo Abeni <pabeni@redhat.com>
Date: Tue, 18 Jan 2022 16:24:19 +0100

> While inspecting some perf report, I noticed that the compiler
> emits suboptimal code for the napi CB initialization, fetching
> and storing multiple times the memory for flags bitfield.
> This is with gcc 10.3.1, but I observed the same with older compiler
> versions.
> 
> We can help the compiler to do a nicer work e.g. initially setting
> all the bitfield to 0 using an u16 alias. The generated code is quite
> smaller, with the same number of conditional
> 
> Before:
> objdump -t net/core/gro.o | grep " F .text"
> 0000000000000bb0 l     F .text	0000000000000357 dev_gro_receive
> 
> After:
> 0000000000000bb0 l     F .text	000000000000033c dev_gro_receive
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>  include/net/gro.h | 13 +++++++++----
>  net/core/gro.c    | 16 +++++-----------
>  2 files changed, 14 insertions(+), 15 deletions(-)
> 
> diff --git a/include/net/gro.h b/include/net/gro.h
> index 8f75802d50fd..a068b27d341f 100644
> --- a/include/net/gro.h
> +++ b/include/net/gro.h
> @@ -29,14 +29,17 @@ struct napi_gro_cb {
>  	/* Number of segments aggregated. */
>  	u16	count;
>  
> -	/* Start offset for remote checksum offload */
> -	u16	gro_remcsum_start;
> +	/* Used in ipv6_gro_receive() and foo-over-udp */
> +	u16	proto;
>  
>  	/* jiffies when first packet was created/queued */
>  	unsigned long age;
>  
> -	/* Used in ipv6_gro_receive() and foo-over-udp */
> -	u16	proto;
> +	/* portion of the cb set to zero at every gro iteration */
> +	u32	zeroed_start[0];
> +
> +	/* Start offset for remote checksum offload */
> +	u16	gro_remcsum_start;
>  
>  	/* This is non-zero if the packet may be of the same flow. */
>  	u8	same_flow:1;
> @@ -70,6 +73,8 @@ struct napi_gro_cb {
>  	/* GRO is done by frag_list pointer chaining. */
>  	u8	is_flist:1;
>  
> +	u32	zeroed_end[0];

This should be wrapped in struct_group() I believe, or compilers
will start complaining soon. See [0] for the details.
Adding Kees to the CCs.

> +
>  	/* used to support CHECKSUM_COMPLETE for tunneling protocols */
>  	__wsum	csum;
>  
> diff --git a/net/core/gro.c b/net/core/gro.c
> index d43d42215bdb..b9ebe9298731 100644
> --- a/net/core/gro.c
> +++ b/net/core/gro.c
> @@ -435,6 +435,9 @@ static void gro_flush_oldest(struct napi_struct *napi, struct list_head *head)
>  	napi_gro_complete(napi, oldest);
>  }
>  
> +#define zeroed_len	(offsetof(struct napi_gro_cb, zeroed_end) - \
> +			 offsetof(struct napi_gro_cb, zeroed_start))
> +
>  static enum gro_result dev_gro_receive(struct napi_struct *napi, struct sk_buff *skb)
>  {
>  	u32 bucket = skb_get_hash_raw(skb) & (GRO_HASH_BUCKETS - 1);
> @@ -459,29 +462,20 @@ static enum gro_result dev_gro_receive(struct napi_struct *napi, struct sk_buff
>  
>  		skb_set_network_header(skb, skb_gro_offset(skb));
>  		skb_reset_mac_len(skb);
> -		NAPI_GRO_CB(skb)->same_flow = 0;
> +		BUILD_BUG_ON(zeroed_len != sizeof(NAPI_GRO_CB(skb)->zeroed_start[0]));
> +		NAPI_GRO_CB(skb)->zeroed_start[0] = 0;
>  		NAPI_GRO_CB(skb)->flush = skb_is_gso(skb) || skb_has_frag_list(skb);
> -		NAPI_GRO_CB(skb)->free = 0;
> -		NAPI_GRO_CB(skb)->encap_mark = 0;
> -		NAPI_GRO_CB(skb)->recursion_counter = 0;
> -		NAPI_GRO_CB(skb)->is_fou = 0;
>  		NAPI_GRO_CB(skb)->is_atomic = 1;
> -		NAPI_GRO_CB(skb)->gro_remcsum_start = 0;
>  
>  		/* Setup for GRO checksum validation */
>  		switch (skb->ip_summed) {
>  		case CHECKSUM_COMPLETE:
>  			NAPI_GRO_CB(skb)->csum = skb->csum;
>  			NAPI_GRO_CB(skb)->csum_valid = 1;
> -			NAPI_GRO_CB(skb)->csum_cnt = 0;
>  			break;
>  		case CHECKSUM_UNNECESSARY:
>  			NAPI_GRO_CB(skb)->csum_cnt = skb->csum_level + 1;
> -			NAPI_GRO_CB(skb)->csum_valid = 0;
>  			break;
> -		default:
> -			NAPI_GRO_CB(skb)->csum_cnt = 0;
> -			NAPI_GRO_CB(skb)->csum_valid = 0;
>  		}
>  
>  		pp = INDIRECT_CALL_INET(ptype->callbacks.gro_receive,
> -- 
> 2.34.1

[0] https://lore.kernel.org/linux-hardening/20220112220652.3952944-1-keescook@chromium.org

Thanks,
Al

Re: [RFC PATCH 2/3] net: gro: minor optimization for dev_gro_receive()

[Paolo Abeni]

On Tue, 2022-01-18 at 16:56 +0100, Alexander Lobakin wrote:
> From: Paolo Abeni <pabeni@redhat.com>
> Date: Tue, 18 Jan 2022 16:24:19 +0100
> 
> > While inspecting some perf report, I noticed that the compiler
> > emits suboptimal code for the napi CB initialization, fetching
> > and storing multiple times the memory for flags bitfield.
> > This is with gcc 10.3.1, but I observed the same with older compiler
> > versions.
> > 
> > We can help the compiler to do a nicer work e.g. initially setting
> > all the bitfield to 0 using an u16 alias. The generated code is quite
> > smaller, with the same number of conditional
> > 
> > Before:
> > objdump -t net/core/gro.o | grep " F .text"
> > 0000000000000bb0 l     F .text	0000000000000357 dev_gro_receive
> > 
> > After:
> > 0000000000000bb0 l     F .text	000000000000033c dev_gro_receive
> > 
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> >  include/net/gro.h | 13 +++++++++----
> >  net/core/gro.c    | 16 +++++-----------
> >  2 files changed, 14 insertions(+), 15 deletions(-)
> > 
> > diff --git a/include/net/gro.h b/include/net/gro.h
> > index 8f75802d50fd..a068b27d341f 100644
> > --- a/include/net/gro.h
> > +++ b/include/net/gro.h
> > @@ -29,14 +29,17 @@ struct napi_gro_cb {
> >  	/* Number of segments aggregated. */
> >  	u16	count;
> >  
> > -	/* Start offset for remote checksum offload */
> > -	u16	gro_remcsum_start;
> > +	/* Used in ipv6_gro_receive() and foo-over-udp */
> > +	u16	proto;
> >  
> >  	/* jiffies when first packet was created/queued */
> >  	unsigned long age;
> >  
> > -	/* Used in ipv6_gro_receive() and foo-over-udp */
> > -	u16	proto;
> > +	/* portion of the cb set to zero at every gro iteration */
> > +	u32	zeroed_start[0];
> > +
> > +	/* Start offset for remote checksum offload */
> > +	u16	gro_remcsum_start;
> >  
> >  	/* This is non-zero if the packet may be of the same flow. */
> >  	u8	same_flow:1;
> > @@ -70,6 +73,8 @@ struct napi_gro_cb {
> >  	/* GRO is done by frag_list pointer chaining. */
> >  	u8	is_flist:1;
> >  
> > +	u32	zeroed_end[0];
> 
> This should be wrapped in struct_group() I believe, or compilers
> will start complaining soon. See [0] for the details.
> Adding Kees to the CCs.

Thank you for the reference. That really slipped-off my mind. 

This patch does not use memcpy() or similar, just a single direct
assignement. Would that still require struct_group()?

Thanks!

Paolo

Re: [RFC PATCH 2/3] net: gro: minor optimization for dev_gro_receive()

[Alexander Lobakin]

From: Paolo Abeni <pabeni@redhat.com>
Date: Tue, 18 Jan 2022 17:31:00 +0100

> On Tue, 2022-01-18 at 16:56 +0100, Alexander Lobakin wrote:
> > From: Paolo Abeni <pabeni@redhat.com>
> > Date: Tue, 18 Jan 2022 16:24:19 +0100
> > 
> > > While inspecting some perf report, I noticed that the compiler
> > > emits suboptimal code for the napi CB initialization, fetching
> > > and storing multiple times the memory for flags bitfield.
> > > This is with gcc 10.3.1, but I observed the same with older compiler
> > > versions.
> > > 
> > > We can help the compiler to do a nicer work e.g. initially setting
> > > all the bitfield to 0 using an u16 alias. The generated code is quite
> > > smaller, with the same number of conditional
> > > 
> > > Before:
> > > objdump -t net/core/gro.o | grep " F .text"
> > > 0000000000000bb0 l     F .text	0000000000000357 dev_gro_receive
> > > 
> > > After:
> > > 0000000000000bb0 l     F .text	000000000000033c dev_gro_receive
> > > 
> > > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > > ---
> > >  include/net/gro.h | 13 +++++++++----
> > >  net/core/gro.c    | 16 +++++-----------
> > >  2 files changed, 14 insertions(+), 15 deletions(-)
> > > 
> > > diff --git a/include/net/gro.h b/include/net/gro.h
> > > index 8f75802d50fd..a068b27d341f 100644
> > > --- a/include/net/gro.h
> > > +++ b/include/net/gro.h
> > > @@ -29,14 +29,17 @@ struct napi_gro_cb {
> > >  	/* Number of segments aggregated. */
> > >  	u16	count;
> > >  
> > > -	/* Start offset for remote checksum offload */
> > > -	u16	gro_remcsum_start;
> > > +	/* Used in ipv6_gro_receive() and foo-over-udp */
> > > +	u16	proto;
> > >  
> > >  	/* jiffies when first packet was created/queued */
> > >  	unsigned long age;
> > >  
> > > -	/* Used in ipv6_gro_receive() and foo-over-udp */
> > > -	u16	proto;
> > > +	/* portion of the cb set to zero at every gro iteration */
> > > +	u32	zeroed_start[0];
> > > +
> > > +	/* Start offset for remote checksum offload */
> > > +	u16	gro_remcsum_start;
> > >  
> > >  	/* This is non-zero if the packet may be of the same flow. */
> > >  	u8	same_flow:1;
> > > @@ -70,6 +73,8 @@ struct napi_gro_cb {
> > >  	/* GRO is done by frag_list pointer chaining. */
> > >  	u8	is_flist:1;
> > >  
> > > +	u32	zeroed_end[0];
> > 
> > This should be wrapped in struct_group() I believe, or compilers
> > will start complaining soon. See [0] for the details.
> > Adding Kees to the CCs.
> 
> Thank you for the reference. That really slipped-off my mind.
> 
> This patch does not use memcpy() or similar, just a single direct
> assignement. Would that still require struct_group()?

Oof, sorry, I saw start/end and overlooked that it's only for
a single assignment.
Then it shouldn't cause warnings, but maybe use an anonymous
union instead?

	union {
		u32 zeroed;
		struct {
			u16 gro_remcsum_start;
			...
		};
	};
	__wsum csum;

Use can still use a BUILD_BUG_ON() in this case, like
sizeof(zeroed) != offsetof(csum) - offsetof(zeroed).

> 
> Thanks!
> 
> Paolo

Thanks,
Al

Re: [RFC PATCH 2/3] net: gro: minor optimization for dev_gro_receive()

[Paolo Abeni]

Hello,

On Tue, 2022-01-18 at 18:39 +0100, Alexander Lobakin wrote:
> From: Paolo Abeni <pabeni@redhat.com>
> Date: Tue, 18 Jan 2022 17:31:00 +0100
> 
> > On Tue, 2022-01-18 at 16:56 +0100, Alexander Lobakin wrote:
> > > From: Paolo Abeni <pabeni@redhat.com>
> > > Date: Tue, 18 Jan 2022 16:24:19 +0100
> > > 
> > > > While inspecting some perf report, I noticed that the compiler
> > > > emits suboptimal code for the napi CB initialization, fetching
> > > > and storing multiple times the memory for flags bitfield.
> > > > This is with gcc 10.3.1, but I observed the same with older compiler
> > > > versions.
> > > > 
> > > > We can help the compiler to do a nicer work e.g. initially setting
> > > > all the bitfield to 0 using an u16 alias. The generated code is quite
> > > > smaller, with the same number of conditional
> > > > 
> > > > Before:
> > > > objdump -t net/core/gro.o | grep " F .text"
> > > > 0000000000000bb0 l     F .text	0000000000000357 dev_gro_receive
> > > > 
> > > > After:
> > > > 0000000000000bb0 l     F .text	000000000000033c dev_gro_receive
> > > > 
> > > > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > > > ---
> > > >  include/net/gro.h | 13 +++++++++----
> > > >  net/core/gro.c    | 16 +++++-----------
> > > >  2 files changed, 14 insertions(+), 15 deletions(-)
> > > > 
> > > > diff --git a/include/net/gro.h b/include/net/gro.h
> > > > index 8f75802d50fd..a068b27d341f 100644
> > > > --- a/include/net/gro.h
> > > > +++ b/include/net/gro.h
> > > > @@ -29,14 +29,17 @@ struct napi_gro_cb {
> > > >  	/* Number of segments aggregated. */
> > > >  	u16	count;
> > > >  
> > > > -	/* Start offset for remote checksum offload */
> > > > -	u16	gro_remcsum_start;
> > > > +	/* Used in ipv6_gro_receive() and foo-over-udp */
> > > > +	u16	proto;
> > > >  
> > > >  	/* jiffies when first packet was created/queued */
> > > >  	unsigned long age;
> > > >  
> > > > -	/* Used in ipv6_gro_receive() and foo-over-udp */
> > > > -	u16	proto;
> > > > +	/* portion of the cb set to zero at every gro iteration */
> > > > +	u32	zeroed_start[0];
> > > > +
> > > > +	/* Start offset for remote checksum offload */
> > > > +	u16	gro_remcsum_start;
> > > >  
> > > >  	/* This is non-zero if the packet may be of the same flow. */
> > > >  	u8	same_flow:1;
> > > > @@ -70,6 +73,8 @@ struct napi_gro_cb {
> > > >  	/* GRO is done by frag_list pointer chaining. */
> > > >  	u8	is_flist:1;
> > > >  
> > > > +	u32	zeroed_end[0];
> > > 
> > > This should be wrapped in struct_group() I believe, or compilers
> > > will start complaining soon. See [0] for the details.
> > > Adding Kees to the CCs.
> > 
> > Thank you for the reference. That really slipped-off my mind.
> > 
> > This patch does not use memcpy() or similar, just a single direct
> > assignement. Would that still require struct_group()?
> 
> Oof, sorry, I saw start/end and overlooked that it's only for
> a single assignment.
> Then it shouldn't cause warnings, but maybe use an anonymous
> union instead?
> 
> 	union {
> 		u32 zeroed;
> 		struct {
> 			u16 gro_remcsum_start;
> 			...
> 		};
> 	};
> 	__wsum csum;
> 
> Use can still use a BUILD_BUG_ON() in this case, like
> sizeof(zeroed) != offsetof(csum) - offsetof(zeroed).

Please forgive me for the very long delay. I'm looking again at this
stuff for formal non-rfc submission.

I like the anonymous union less, because it will move around much more
code - making the patch less readable - and will be more fragile e.g.
some comment alike "please don't move around 'csum'" would be needed.

No strong opinion anyway, so if you really prefer that way I can adapt.
Please let me know.

Thanks!

Paolo

Re: [RFC PATCH 2/3] net: gro: minor optimization for dev_gro_receive()

[Alexander Lobakin]

From: Paolo Abeni <pabeni@redhat.com>
Date: Wed, 02 Feb 2022 11:09:41 +0100

> Hello,

Hi!

> 
> On Tue, 2022-01-18 at 18:39 +0100, Alexander Lobakin wrote:
> > From: Paolo Abeni <pabeni@redhat.com>
> > Date: Tue, 18 Jan 2022 17:31:00 +0100
> > 
> > > On Tue, 2022-01-18 at 16:56 +0100, Alexander Lobakin wrote:
> > > > From: Paolo Abeni <pabeni@redhat.com>
> > > > Date: Tue, 18 Jan 2022 16:24:19 +0100
> > > > 
> > > > > While inspecting some perf report, I noticed that the compiler
> > > > > emits suboptimal code for the napi CB initialization, fetching
> > > > > and storing multiple times the memory for flags bitfield.
> > > > > This is with gcc 10.3.1, but I observed the same with older compiler
> > > > > versions.
> > > > > 
> > > > > We can help the compiler to do a nicer work e.g. initially setting
> > > > > all the bitfield to 0 using an u16 alias. The generated code is quite
> > > > > smaller, with the same number of conditional
> > > > > 
> > > > > Before:
> > > > > objdump -t net/core/gro.o | grep " F .text"
> > > > > 0000000000000bb0 l     F .text	0000000000000357 dev_gro_receive
> > > > > 
> > > > > After:
> > > > > 0000000000000bb0 l     F .text	000000000000033c dev_gro_receive
> > > > > 
> > > > > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > > > > ---
> > > > >  include/net/gro.h | 13 +++++++++----
> > > > >  net/core/gro.c    | 16 +++++-----------
> > > > >  2 files changed, 14 insertions(+), 15 deletions(-)
> > > > > 
> > > > > diff --git a/include/net/gro.h b/include/net/gro.h
> > > > > index 8f75802d50fd..a068b27d341f 100644
> > > > > --- a/include/net/gro.h
> > > > > +++ b/include/net/gro.h
> > > > > @@ -29,14 +29,17 @@ struct napi_gro_cb {
> > > > >  	/* Number of segments aggregated. */
> > > > >  	u16	count;
> > > > >  
> > > > > -	/* Start offset for remote checksum offload */
> > > > > -	u16	gro_remcsum_start;
> > > > > +	/* Used in ipv6_gro_receive() and foo-over-udp */
> > > > > +	u16	proto;
> > > > >  
> > > > >  	/* jiffies when first packet was created/queued */
> > > > >  	unsigned long age;
> > > > >  
> > > > > -	/* Used in ipv6_gro_receive() and foo-over-udp */
> > > > > -	u16	proto;
> > > > > +	/* portion of the cb set to zero at every gro iteration */
> > > > > +	u32	zeroed_start[0];
> > > > > +
> > > > > +	/* Start offset for remote checksum offload */
> > > > > +	u16	gro_remcsum_start;
> > > > >  
> > > > >  	/* This is non-zero if the packet may be of the same flow. */
> > > > >  	u8	same_flow:1;
> > > > > @@ -70,6 +73,8 @@ struct napi_gro_cb {
> > > > >  	/* GRO is done by frag_list pointer chaining. */
> > > > >  	u8	is_flist:1;
> > > > >  
> > > > > +	u32	zeroed_end[0];
> > > > 
> > > > This should be wrapped in struct_group() I believe, or compilers
> > > > will start complaining soon. See [0] for the details.
> > > > Adding Kees to the CCs.
> > > 
> > > Thank you for the reference. That really slipped-off my mind.
> > > 
> > > This patch does not use memcpy() or similar, just a single direct
> > > assignement. Would that still require struct_group()?
> > 
> > Oof, sorry, I saw start/end and overlooked that it's only for
> > a single assignment.
> > Then it shouldn't cause warnings, but maybe use an anonymous
> > union instead?
> > 
> > 	union {
> > 		u32 zeroed;
> > 		struct {
> > 			u16 gro_remcsum_start;
> > 			...
> > 		};
> > 	};
> > 	__wsum csum;
> > 
> > Use can still use a BUILD_BUG_ON() in this case, like
> > sizeof(zeroed) != offsetof(csum) - offsetof(zeroed).
> 
> Please forgive me for the very long delay. I'm looking again at this
> stuff for formal non-rfc submission.

Sure, not a problem at all (:

> 
> I like the anonymous union less, because it will move around much more
> code - making the patch less readable - and will be more fragile e.g.
> some comment alike "please don't move around 'csum'" would be needed.

We still need comments around zeroed_{start,end}[0] for now.
I used offsetof(csum) as offsetofend(is_flist) which I'd prefer here
unfortunately expands to offsetof(is_flist) + sizeof(is_flist), and
the latter causes an error of using sizeof() against a bitfield.

> 
> No strong opinion anyway, so if you really prefer that way I can adapt.
> Please let me know.

I don't really have a strong preference here, I just suspect that
zero-length array will produce or already produce -Warray-bounds
warnings, and empty-struct constructs like

	struct { } zeroed_start;
	u16 gro_remcsum_start;
	...
	struct { } zeroed_end;

	memset(NAPI_GRO_CB(skb)->zeroed_start, 0,
	       offsetofend(zeroed_end) - offsetsof(zeroed_start));

will trigger Fortify compile-time errors from Kees' KSPP tree.

I think we could use

	__struct_group(/* no tag */, zeroed, /* no attrs */,
		u16 gro_remcsum_start;
		...
		u8 is_flist:1;
	);
	__wsum csum;

	BUILD_BUG_ON(sizeof_field(struct napi_gro_cb, zeroed) != sizeof(u32));
	BUILD_BUG_ON(!IS_ALIGNED(offsetof(struct napi_gro_cb, zeroed),
				 sizeof(u32))); /* Avoid slow unaligned acc */

	*(u32 *)&NAPI_GRO_CB(skb)->zeroed = 0;

This doesn't depend on `csum`, doesn't need `struct { }` or
`struct zero[0]` markers and still uses a direct assignment.

Also adding Gustavo, maybe he'd like to leave a comment here.

> 
> Thanks!
> 
> Paolo

Thanks,
Al

Re: [RFC PATCH 2/3] net: gro: minor optimization for dev_gro_receive()

[Kees Cook]

On Wed, Feb 02, 2022 at 01:08:27PM +0100, Alexander Lobakin wrote:
> From: Paolo Abeni <pabeni@redhat.com>
> Date: Wed, 02 Feb 2022 11:09:41 +0100
> 
> > Hello,
> 
> Hi!
> 
> > 
> > On Tue, 2022-01-18 at 18:39 +0100, Alexander Lobakin wrote:
> > > From: Paolo Abeni <pabeni@redhat.com>
> > > Date: Tue, 18 Jan 2022 17:31:00 +0100
> > > 
> > > > On Tue, 2022-01-18 at 16:56 +0100, Alexander Lobakin wrote:
> > > > > From: Paolo Abeni <pabeni@redhat.com>
> > > > > Date: Tue, 18 Jan 2022 16:24:19 +0100
> > > > > 
> > > > > > While inspecting some perf report, I noticed that the compiler
> > > > > > emits suboptimal code for the napi CB initialization, fetching
> > > > > > and storing multiple times the memory for flags bitfield.
> > > > > > This is with gcc 10.3.1, but I observed the same with older compiler
> > > > > > versions.
> > > > > > 
> > > > > > We can help the compiler to do a nicer work e.g. initially setting
> > > > > > all the bitfield to 0 using an u16 alias. The generated code is quite
> > > > > > smaller, with the same number of conditional
> > > > > > 
> > > > > > Before:
> > > > > > objdump -t net/core/gro.o | grep " F .text"
> > > > > > 0000000000000bb0 l     F .text	0000000000000357 dev_gro_receive
> > > > > > 
> > > > > > After:
> > > > > > 0000000000000bb0 l     F .text	000000000000033c dev_gro_receive
> > > > > > 
> > > > > > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > > > > > ---
> > > > > >  include/net/gro.h | 13 +++++++++----
> > > > > >  net/core/gro.c    | 16 +++++-----------
> > > > > >  2 files changed, 14 insertions(+), 15 deletions(-)
> > > > > > 
> > > > > > diff --git a/include/net/gro.h b/include/net/gro.h
> > > > > > index 8f75802d50fd..a068b27d341f 100644
> > > > > > --- a/include/net/gro.h
> > > > > > +++ b/include/net/gro.h
> > > > > > @@ -29,14 +29,17 @@ struct napi_gro_cb {
> > > > > >  	/* Number of segments aggregated. */
> > > > > >  	u16	count;
> > > > > >  
> > > > > > -	/* Start offset for remote checksum offload */
> > > > > > -	u16	gro_remcsum_start;
> > > > > > +	/* Used in ipv6_gro_receive() and foo-over-udp */
> > > > > > +	u16	proto;
> > > > > >  
> > > > > >  	/* jiffies when first packet was created/queued */
> > > > > >  	unsigned long age;
> > > > > >  
> > > > > > -	/* Used in ipv6_gro_receive() and foo-over-udp */
> > > > > > -	u16	proto;
> > > > > > +	/* portion of the cb set to zero at every gro iteration */
> > > > > > +	u32	zeroed_start[0];
> > > > > > +
> > > > > > +	/* Start offset for remote checksum offload */
> > > > > > +	u16	gro_remcsum_start;
> > > > > >  
> > > > > >  	/* This is non-zero if the packet may be of the same flow. */
> > > > > >  	u8	same_flow:1;
> > > > > > @@ -70,6 +73,8 @@ struct napi_gro_cb {
> > > > > >  	/* GRO is done by frag_list pointer chaining. */
> > > > > >  	u8	is_flist:1;
> > > > > >  
> > > > > > +	u32	zeroed_end[0];
> > > > > 
> > > > > This should be wrapped in struct_group() I believe, or compilers
> > > > > will start complaining soon. See [0] for the details.
> > > > > Adding Kees to the CCs.

Hi! Sorry I missed the original email sent my way. :)

> > > > 
> > > > Thank you for the reference. That really slipped-off my mind.
> > > > 
> > > > This patch does not use memcpy() or similar, just a single direct
> > > > assignement. Would that still require struct_group()?
> > > 
> > > Oof, sorry, I saw start/end and overlooked that it's only for
> > > a single assignment.
> > > Then it shouldn't cause warnings, but maybe use an anonymous
> > > union instead?
> > > 
> > > 	union {
> > > 		u32 zeroed;
> > > 		struct {
> > > 			u16 gro_remcsum_start;
> > > 			...
> > > 		};
> > > 	};
> > > 	__wsum csum;
> > > 
> > > Use can still use a BUILD_BUG_ON() in this case, like
> > > sizeof(zeroed) != offsetof(csum) - offsetof(zeroed).
> > 
> > Please forgive me for the very long delay. I'm looking again at this
> > stuff for formal non-rfc submission.
> 
> Sure, not a problem at all (:
> 
> > 
> > I like the anonymous union less, because it will move around much more
> > code - making the patch less readable - and will be more fragile e.g.
> > some comment alike "please don't move around 'csum'" would be needed.
> 
> We still need comments around zeroed_{start,end}[0] for now.
> I used offsetof(csum) as offsetofend(is_flist) which I'd prefer here
> unfortunately expands to offsetof(is_flist) + sizeof(is_flist), and
> the latter causes an error of using sizeof() against a bitfield.
> 
> > 
> > No strong opinion anyway, so if you really prefer that way I can adapt.
> > Please let me know.
> 
> I don't really have a strong preference here, I just suspect that
> zero-length array will produce or already produce -Warray-bounds
> warnings, and empty-struct constructs like

Yes, -Warray-bounds would yell very loudly about an 0-element array
having its first element assigned. :)

> 
> 	struct { } zeroed_start;
> 	u16 gro_remcsum_start;
> 	...
> 	struct { } zeroed_end;
> 
> 	memset(NAPI_GRO_CB(skb)->zeroed_start, 0,
> 	       offsetofend(zeroed_end) - offsetsof(zeroed_start));
> 
> will trigger Fortify compile-time errors from Kees' KSPP tree.
> 
> I think we could use
> 
> 	__struct_group(/* no tag */, zeroed, /* no attrs */,

Since this isn't UAPI, you can just do:

	struct_group(zeroed,

> 		u16 gro_remcsum_start;
> 		...
> 		u8 is_flist:1;
> 	);
> 	__wsum csum;
> 
> 	BUILD_BUG_ON(sizeof_field(struct napi_gro_cb, zeroed) != sizeof(u32));
> 	BUILD_BUG_ON(!IS_ALIGNED(offsetof(struct napi_gro_cb, zeroed),
> 				 sizeof(u32))); /* Avoid slow unaligned acc */
> 
> 	*(u32 *)&NAPI_GRO_CB(skb)->zeroed = 0;
> 
> This doesn't depend on `csum`, doesn't need `struct { }` or
> `struct zero[0]` markers and still uses a direct assignment.

Given the kernel's -O2 use, a constant-sized u32 memcpy() and a direct
assignment will resolve to the same machine code:

https://godbolt.org/z/b9qKexx6q

void zero_var(struct object *p)
{
    p->zero_var = 0;
}

void zero_group(struct object *p)
{
    memset(&p->zero_group, 0, sizeof(p->zero_group));
}

zero_var:
  movl $0, (%rdi)
  ret
zero_group:
  movl $0, (%rdi)
  ret

And assigning them individually results in careful "and"ing to avoid the
padding:

zero_each:
  andl $-134217728, (%rdi)
  ret

I would have recommended struct_group() + memset() here, just because
you don't need the casts. But since you're doing BUILD_BUG_ON() size
verification, it's fine either way.

If you want to avoid the cast and avoid the memset(), you could use the
earlier suggestion of anonymous union and anonymous struct. It's
basically an open-coded struct_group, but you get to pick the type of
the overlapping variable. :)

-Kees

> Also adding Gustavo, maybe he'd like to leave a comment here.
> 
> > 
> > Thanks!
> > 
> > Paolo
> 
> Thanks,
> Al

-- 
Kees Cook