[Patch net-next v3] net: clean up skb headers code

From: Cong Wang <redacted>

commit 1a37e412a0225fcba5587 (net: Use 16bits for *_headers
fields of struct skbuff) converts skb->*_header to u16,
some #if NET_SKBUFF_DATA_USES_OFFSET are now useless,
and to be safe, we could just use "X = ~(typeof(X))0;"
as suggested by David and Ben.

Cc: Ben Hutchings <redacted>
Cc: David S. Miller <davem@davemloft.net>
Cc: Simon Horman <horms@verge.net.au>
Signed-off-by: Cong Wang <redacted>

---
v2: use typeof
v3: use ~(typeof(X))0

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 5f93119..f3c18a0 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h

@@ -1593,7 +1593,7 @@ static inline void skb_set_inner_mac_header(struct sk_buff *skb,
 }
 static inline bool skb_transport_header_was_set(const struct sk_buff *skb)
 {
-	return skb->transport_header != ~0U;
+	return skb->transport_header != ~(typeof(skb->transport_header))0;
 }
 
 static inline unsigned char *skb_transport_header(const struct sk_buff *skb)

@@ -1636,7 +1636,7 @@ static inline unsigned char *skb_mac_header(const struct sk_buff *skb)
 
 static inline int skb_mac_header_was_set(const struct sk_buff *skb)
 {
-	return skb->mac_header != ~0U;
+	return skb->mac_header != ~(typeof(skb->mac_header))0;
 }
 
 static inline void skb_reset_mac_header(struct sk_buff *skb)

diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index f45de07..e121c4b 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c

@@ -199,9 +199,7 @@ struct sk_buff *__alloc_skb_head(gfp_t gfp_mask, int node)
 	skb->truesize = sizeof(struct sk_buff);
 	atomic_set(&skb->users, 1);
 
-#ifdef NET_SKBUFF_DATA_USES_OFFSET
-	skb->mac_header = (__u16) ~0U;
-#endif
+	skb->mac_header = ~(typeof(skb->mac_header))0;
 out:
 	return skb;
 }

@@ -275,10 +273,8 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
 	skb->data = data;
 	skb_reset_tail_pointer(skb);
 	skb->end = skb->tail + size;
-#ifdef NET_SKBUFF_DATA_USES_OFFSET
-	skb->mac_header = (__u16) ~0U;
-	skb->transport_header = (__u16) ~0U;
-#endif
+	skb->mac_header = ~(typeof(skb->mac_header))0;
+	skb->transport_header = ~(typeof(skb->transport_header))0;
 
 	/* make sure we initialize shinfo sequentially */
 	shinfo = skb_shinfo(skb);

@@ -344,10 +340,8 @@ struct sk_buff *build_skb(void *data, unsigned int frag_size)
 	skb->data = data;
 	skb_reset_tail_pointer(skb);
 	skb->end = skb->tail + size;
-#ifdef NET_SKBUFF_DATA_USES_OFFSET
-	skb->mac_header = (__u16) ~0U;
-	skb->transport_header = (__u16) ~0U;
-#endif
+	skb->mac_header = ~(typeof(skb->mac_header))0;
+	skb->transport_header = ~(typeof(skb->transport_header))0;
 
 	/* make sure we initialize shinfo sequentially */
 	shinfo = skb_shinfo(skb);

commit 1a37e412a0225fcba5587 (net: Use 16bits for *_headers
fields of struct skbuff) converts skb->*_header to u16,
some #if NET_SKBUFF_DATA_USES_OFFSET are now useless,
and to be safe, we could just use "X = ~(typeof(X))0;"
as suggested by David and Ben.

IIRC ~(unsigned short)0 is the same as ~0 and will be -1.
define an actual constant with value 0xffff

	David

From: "David Laight" <redacted>
Date: Thu, 30 May 2013 12:18:13 +0100

quoted

commit 1a37e412a0225fcba5587 (net: Use 16bits for *_headers
fields of struct skbuff) converts skb->*_header to u16,
some #if NET_SKBUFF_DATA_USES_OFFSET are now useless,
and to be safe, we could just use "X = ~(typeof(X))0;"
as suggested by David and Ben.

IIRC ~(unsigned short)0 is the same as ~0 and will be -1.
define an actual constant with value 0xffff

The problem with doing that is that if this type is changed again,
we'lll go through this same circus changing the constant everywhere
and missing some cases.

I absolutely want to see a version of this fix that makes sure that if
the type changes, either the constantly automatically change (that's
the "typeof(X)" approach) or the build breaks spectacularly until the
value is fixed up.

On Thu, 2013-05-30 at 13:16 -0700, David Miller wrote:

From: "David Laight" <redacted>
Date: Thu, 30 May 2013 12:18:13 +0100

quoted

quoted

commit 1a37e412a0225fcba5587 (net: Use 16bits for *_headers
fields of struct skbuff) converts skb->*_header to u16,
some #if NET_SKBUFF_DATA_USES_OFFSET are now useless,
and to be safe, we could just use "X = ~(typeof(X))0;"
as suggested by David and Ben.

IIRC ~(unsigned short)0 is the same as ~0 and will be -1.
define an actual constant with value 0xffff

The problem with doing that is that if this type is changed again,
we'lll go through this same circus changing the constant everywhere
and missing some cases.

I absolutely want to see a version of this fix that makes sure that if
the type changes, either the constantly automatically change (that's
the "typeof(X)" approach) or the build breaks spectacularly until the
value is fixed up.

Let's just give the 'invalid' value a name and define it right next to
those fields.  It might be worth reintroducing sk_buff_data_t too, since
there was at least one place which wanted to save and restore offsets in
local variables.

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

On Thu, May 30, 2013 at 10:05:18PM +0100, Ben Hutchings wrote:

On Thu, 2013-05-30 at 13:16 -0700, David Miller wrote:

quoted

From: "David Laight" <redacted>
Date: Thu, 30 May 2013 12:18:13 +0100

quoted

quoted

commit 1a37e412a0225fcba5587 (net: Use 16bits for *_headers
fields of struct skbuff) converts skb->*_header to u16,
some #if NET_SKBUFF_DATA_USES_OFFSET are now useless,
and to be safe, we could just use "X = ~(typeof(X))0;"
as suggested by David and Ben.

IIRC ~(unsigned short)0 is the same as ~0 and will be -1.
define an actual constant with value 0xffff

The problem with doing that is that if this type is changed again,
we'lll go through this same circus changing the constant everywhere
and missing some cases.

I absolutely want to see a version of this fix that makes sure that if
the type changes, either the constantly automatically change (that's
the "typeof(X)" approach) or the build breaks spectacularly until the
value is fixed up.

Let's just give the 'invalid' value a name and define it right next to
those fields.  It might be worth reintroducing sk_buff_data_t too, since
there was at least one place which wanted to save and restore offsets in
local variables.

sk_buff_data_t was not removed, its just used for less fields.

On Thu, May 30, 2013 at 10:06:15AM +0800, Cong Wang wrote:

From: Cong Wang <redacted>

commit 1a37e412a0225fcba5587 (net: Use 16bits for *_headers
fields of struct skbuff) converts skb->*_header to u16,
some #if NET_SKBUFF_DATA_USES_OFFSET are now useless,
and to be safe, we could just use "X = ~(typeof(X))0;"
as suggested by David and Ben.

Cc: Ben Hutchings <redacted>
Cc: David S. Miller <davem@davemloft.net>
Cc: Simon Horman <horms@verge.net.au>
Signed-off-by: Cong Wang <redacted>

FWIW, I would prefer to see a patch that just removes the instances
of #ifdef NET_SKBUFF_DATA_USES_OFFSET below, and then a separate
patch to clean-up the use of ~0U once some consensus is reached on
the best approach.

The reason is that I suspect that the #ifdef NET_SKBUFF_DATA_USES_OFFSET
is a bug fix for cases where it is not defined. Whereas the ~0 discussion
is more cosmetic.