[PATCH net-next 0/1] skbuff: Extend gso_type to unsigned int

[PATCH net-next 0/1] skbuff: Extend gso_type to unsigned int

[Steffen Klassert]

We need a GSO type for IPsec ESP to be able to integrate the IPsec
hardware offloading infrastructure. Unfortunately, all gso_type flags
are currently in use. This change extends gso_type from 'unsigned short'
to 'unsigned int'.

I'm sending this as a separate patch because it touches one of the
core networking structures, so I don't want to merge this together
with the IPsec hardware offloading infrastructure via the ipsec-next
tree.

[PATCH net-next 1/1] skbuff: Extend gso_type to unsigned int.

[Steffen Klassert]

All available gso_type flags are currently in use, so
extend gso_type from 'unsigned short' to 'unsigned int'
to be able to add further flags.

We also reorder the struct skb_shared_info to use
two bytes of the four byte hole before dataref.
All fields before _unused are cleared now, i.e.
two bytes more than before the change.

Structure layout on x86-64 before the change:

struct skb_shared_info {
	unsigned char              nr_frags;             /*     0     1 */
	__u8                       tx_flags;             /*     1     1 */
	short unsigned int         gso_size;             /*     2     2 */
	short unsigned int         gso_segs;             /*     4     2 */
	short unsigned int         gso_type;             /*     6     2 */
	struct sk_buff *           frag_list;            /*     8     8 */
	struct skb_shared_hwtstamps hwtstamps;           /*    16     8 */
	u32                        tskey;                /*    24     4 */
	__be32                     ip6_frag_id;          /*    28     4 */
	atomic_t                   dataref;              /*    32     4 */

	/* XXX 4 bytes hole, try to pack */

	void *                     destructor_arg;       /*    40     8 */
	skb_frag_t                 frags[17];            /*    48   272 */
	/* --- cacheline 5 boundary (320 bytes) --- */

	/* size: 320, cachelines: 5, members: 12 */
	/* sum members: 316, holes: 1, sum holes: 4 */
};

Structure layout on x86-64 after the change:

struct skb_shared_info {
	struct sk_buff *           frag_list;            /*     0     8 */
	struct skb_shared_hwtstamps hwtstamps;           /*     8     8 */
	u32                        tskey;                /*    16     4 */
	__be32                     ip6_frag_id;          /*    20     4 */
	unsigned int               gso_type;             /*    24     4 */
	unsigned char              nr_frags;             /*    28     1 */
	__u8                       tx_flags;             /*    29     1 */
	short unsigned int         gso_size;             /*    30     2 */
	short unsigned int         gso_segs;             /*    32     2 */
	short unsigned int         _unused;              /*    34     2 */
	atomic_t                   dataref;              /*    36     4 */
	void *                     destructor_arg;       /*    40     8 */
	skb_frag_t                 frags[17];            /*    48   272 */
	/* --- cacheline 5 boundary (320 bytes) --- */

	/* size: 320, cachelines: 5, members: 13 */
};

Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
---
 include/linux/skbuff.h | 13 +++++++------
 net/core/skbuff.c      |  2 +-
 2 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index c776abd..cb36159 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -413,20 +413,21 @@ struct ubuf_info {
  * the end of the header data, ie. at skb->end.
  */
 struct skb_shared_info {
+	struct sk_buff	*frag_list;
+	struct skb_shared_hwtstamps hwtstamps;
+	u32		tskey;
+	__be32          ip6_frag_id;
+	unsigned int	gso_type;
 	unsigned char	nr_frags;
 	__u8		tx_flags;
 	unsigned short	gso_size;
 	/* Warning: this field is not always filled in (UFO)! */
 	unsigned short	gso_segs;
-	unsigned short  gso_type;
-	struct sk_buff	*frag_list;
-	struct skb_shared_hwtstamps hwtstamps;
-	u32		tskey;
-	__be32          ip6_frag_id;
 
 	/*
-	 * Warning : all fields before dataref are cleared in __alloc_skb()
+	 * Warning : all fields before _unused are cleared in __alloc_skb()
 	 */
+	unsigned short	_unused;
 	atomic_t	dataref;
 
 	/* Intermediate layers must ensure that destructor_arg
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 9f78109..8796b93 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -257,7 +257,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
 
 	/* make sure we initialize shinfo sequentially */
 	shinfo = skb_shinfo(skb);
-	memset(shinfo, 0, offsetof(struct skb_shared_info, dataref));
+	memset(shinfo, 0, offsetof(struct skb_shared_info, _unused));
 	atomic_set(&shinfo->dataref, 1);
 	kmemcheck_annotate_variable(shinfo->destructor_arg);
 
-- 
2.7.4

Re: [PATCH net-next 1/1] skbuff: Extend gso_type to unsigned int.

[David Miller]

From: Steffen Klassert <steffen.klassert@secunet.com>
Date: Mon, 3 Apr 2017 10:15:52 +0200

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 9f78109..8796b93 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -257,7 +257,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
>  
>  	/* make sure we initialize shinfo sequentially */
>  	shinfo = skb_shinfo(skb);
> -	memset(shinfo, 0, offsetof(struct skb_shared_info, dataref));
> +	memset(shinfo, 0, offsetof(struct skb_shared_info, _unused));
>  	atomic_set(&shinfo->dataref, 1);
>  	kmemcheck_annotate_variable(shinfo->destructor_arg);
>  

There is another memset() that needs this conversion in __build_skb().

RE: [PATCH net-next 1/1] skbuff: Extend gso_type to unsigned int.

[David Laight]

From: Steffen Klassert
> Sent: 03 April 2017 09:16
> All available gso_type flags are currently in use, so
> extend gso_type from 'unsigned short' to 'unsigned int'
> to be able to add further flags.
> 
> We also reorder the struct skb_shared_info to use
> two bytes of the four byte hole before dataref.
> All fields before _unused are cleared now, i.e.
> two bytes more than before the change.
...
> -	 * Warning : all fields before dataref are cleared in __alloc_skb()
> +	 * Warning : all fields before _unused are cleared in __alloc_skb()
>  	 */
> +	unsigned short	_unused;
>  	atomic_t	dataref;
> 
>  	/* Intermediate layers must ensure that destructor_arg
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 9f78109..8796b93 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -257,7 +257,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
> 
>  	/* make sure we initialize shinfo sequentially */
>  	shinfo = skb_shinfo(skb);
> -	memset(shinfo, 0, offsetof(struct skb_shared_info, dataref));
> +	memset(shinfo, 0, offsetof(struct skb_shared_info, _unused));
>  	atomic_set(&shinfo->dataref, 1);
>  	kmemcheck_annotate_variable(shinfo->destructor_arg);

Is it worth adding a specific field name for the end of the 'zero' area?
Either as a #define or anonymous union.

I think that changing the definition to:
	union {
		unsigned short	_unused;
		atomic_t	dataref;
	};
would require no other changes.

	David

Re: [PATCH net-next 1/1] skbuff: Extend gso_type to unsigned int.

[Alexander Duyck]

On Mon, Apr 3, 2017 at 1:15 AM, Steffen Klassert
<steffen.klassert@secunet.com> wrote:
> All available gso_type flags are currently in use, so
> extend gso_type from 'unsigned short' to 'unsigned int'
> to be able to add further flags.
>
> We also reorder the struct skb_shared_info to use
> two bytes of the four byte hole before dataref.
> All fields before _unused are cleared now, i.e.
> two bytes more than before the change.
>
> Structure layout on x86-64 before the change:
>
> struct skb_shared_info {
>         unsigned char              nr_frags;             /*     0     1 */
>         __u8                       tx_flags;             /*     1     1 */
>         short unsigned int         gso_size;             /*     2     2 */
>         short unsigned int         gso_segs;             /*     4     2 */
>         short unsigned int         gso_type;             /*     6     2 */
>         struct sk_buff *           frag_list;            /*     8     8 */
>         struct skb_shared_hwtstamps hwtstamps;           /*    16     8 */
>         u32                        tskey;                /*    24     4 */
>         __be32                     ip6_frag_id;          /*    28     4 */
>         atomic_t                   dataref;              /*    32     4 */
>
>         /* XXX 4 bytes hole, try to pack */
>
>         void *                     destructor_arg;       /*    40     8 */
>         skb_frag_t                 frags[17];            /*    48   272 */
>         /* --- cacheline 5 boundary (320 bytes) --- */
>
>         /* size: 320, cachelines: 5, members: 12 */
>         /* sum members: 316, holes: 1, sum holes: 4 */
> };
>
> Structure layout on x86-64 after the change:
>
> struct skb_shared_info {
>         struct sk_buff *           frag_list;            /*     0     8 */
>         struct skb_shared_hwtstamps hwtstamps;           /*     8     8 */
>         u32                        tskey;                /*    16     4 */
>         __be32                     ip6_frag_id;          /*    20     4 */
>         unsigned int               gso_type;             /*    24     4 */
>         unsigned char              nr_frags;             /*    28     1 */
>         __u8                       tx_flags;             /*    29     1 */
>         short unsigned int         gso_size;             /*    30     2 */
>         short unsigned int         gso_segs;             /*    32     2 */
>         short unsigned int         _unused;              /*    34     2 */
>         atomic_t                   dataref;              /*    36     4 */
>         void *                     destructor_arg;       /*    40     8 */
>         skb_frag_t                 frags[17];            /*    48   272 */
>         /* --- cacheline 5 boundary (320 bytes) --- */
>
>         /* size: 320, cachelines: 5, members: 13 */
> };
>
> Signed-off-by: Steffen Klassert <steffen.klassert@secunet.com>
> ---
>  include/linux/skbuff.h | 13 +++++++------
>  net/core/skbuff.c      |  2 +-
>  2 files changed, 8 insertions(+), 7 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index c776abd..cb36159 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -413,20 +413,21 @@ struct ubuf_info {
>   * the end of the header data, ie. at skb->end.
>   */
>  struct skb_shared_info {
> +       struct sk_buff  *frag_list;
> +       struct skb_shared_hwtstamps hwtstamps;
> +       u32             tskey;
> +       __be32          ip6_frag_id;
> +       unsigned int    gso_type;
>         unsigned char   nr_frags;
>         __u8            tx_flags;
>         unsigned short  gso_size;
>         /* Warning: this field is not always filled in (UFO)! */
>         unsigned short  gso_segs;
> -       unsigned short  gso_type;
> -       struct sk_buff  *frag_list;
> -       struct skb_shared_hwtstamps hwtstamps;
> -       u32             tskey;
> -       __be32          ip6_frag_id;
>
>         /*
> -        * Warning : all fields before dataref are cleared in __alloc_skb()
> +        * Warning : all fields before _unused are cleared in __alloc_skb()
>          */
> +       unsigned short  _unused;

You can probably just put a comment about a 2 byte hole here.

In most cases the cost here is the fact that you are adding bytes
beyond 32.  So if you are adding 2 bytes or 4 the cost should be about
the same for the memset since it is having to deal with something less
than or equal to a long. This all compiles out into 4 mov operations
on x86_64, and adding this regardless of adding 2, 4, or 8 will just
add one more. So adding _unused is just an unnecessary optimization.

Doing that would make the code changes below unneeded.

>         atomic_t        dataref;
>
>         /* Intermediate layers must ensure that destructor_arg
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 9f78109..8796b93 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -257,7 +257,7 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
>
>         /* make sure we initialize shinfo sequentially */
>         shinfo = skb_shinfo(skb);
> -       memset(shinfo, 0, offsetof(struct skb_shared_info, dataref));
> +       memset(shinfo, 0, offsetof(struct skb_shared_info, _unused));
>         atomic_set(&shinfo->dataref, 1);
>         kmemcheck_annotate_variable(shinfo->destructor_arg);
>
> --
> 2.7.4
>