* [Patch net-next v3] net: clean up skb headers code
@ 2013-05-30 2:06 Cong Wang
2013-05-30 11:18 ` David Laight
2013-05-31 0:25 ` Simon Horman
0 siblings, 2 replies; 6+ messages in thread
From: Cong Wang @ 2013-05-30 2:06 UTC (permalink / raw)
To: netdev; +Cc: Ben Hutchings, David S. Miller, Simon Horman, Cong Wang
From: Cong Wang <amwang@redhat.com>
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 <bhutchings@solarflare.com>
Cc: David S. Miller <davem@davemloft.net>
Cc: Simon Horman <horms@verge.net.au>
Signed-off-by: Cong Wang <amwang@redhat.com>
---
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);
^ permalink raw reply related [flat|nested] 6+ messages in thread* RE: [Patch net-next v3] net: clean up skb headers code
2013-05-30 2:06 [Patch net-next v3] net: clean up skb headers code Cong Wang
@ 2013-05-30 11:18 ` David Laight
2013-05-30 20:16 ` David Miller
2013-05-31 0:25 ` Simon Horman
1 sibling, 1 reply; 6+ messages in thread
From: David Laight @ 2013-05-30 11:18 UTC (permalink / raw)
To: Cong Wang, netdev; +Cc: Ben Hutchings, David S. Miller, Simon Horman
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch net-next v3] net: clean up skb headers code
2013-05-30 11:18 ` David Laight
@ 2013-05-30 20:16 ` David Miller
2013-05-30 21:05 ` Ben Hutchings
0 siblings, 1 reply; 6+ messages in thread
From: David Miller @ 2013-05-30 20:16 UTC (permalink / raw)
To: David.Laight; +Cc: amwang, netdev, bhutchings, horms
From: "David Laight" <David.Laight@ACULAB.COM>
Date: Thu, 30 May 2013 12:18:13 +0100
>> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch net-next v3] net: clean up skb headers code
2013-05-30 20:16 ` David Miller
@ 2013-05-30 21:05 ` Ben Hutchings
2013-05-31 0:18 ` Simon Horman
0 siblings, 1 reply; 6+ messages in thread
From: Ben Hutchings @ 2013-05-30 21:05 UTC (permalink / raw)
To: David Miller; +Cc: David.Laight, amwang, netdev, horms
On Thu, 2013-05-30 at 13:16 -0700, David Miller wrote:
> From: "David Laight" <David.Laight@ACULAB.COM>
> Date: Thu, 30 May 2013 12:18:13 +0100
>
> >> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch net-next v3] net: clean up skb headers code
2013-05-30 21:05 ` Ben Hutchings
@ 2013-05-31 0:18 ` Simon Horman
0 siblings, 0 replies; 6+ messages in thread
From: Simon Horman @ 2013-05-31 0:18 UTC (permalink / raw)
To: Ben Hutchings; +Cc: David Miller, David.Laight, amwang, netdev
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:
> > From: "David Laight" <David.Laight@ACULAB.COM>
> > Date: Thu, 30 May 2013 12:18:13 +0100
> >
> > >> 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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Patch net-next v3] net: clean up skb headers code
2013-05-30 2:06 [Patch net-next v3] net: clean up skb headers code Cong Wang
2013-05-30 11:18 ` David Laight
@ 2013-05-31 0:25 ` Simon Horman
1 sibling, 0 replies; 6+ messages in thread
From: Simon Horman @ 2013-05-31 0:25 UTC (permalink / raw)
To: Cong Wang; +Cc: netdev, Ben Hutchings, David S. Miller
On Thu, May 30, 2013 at 10:06:15AM +0800, Cong Wang wrote:
> From: Cong Wang <amwang@redhat.com>
>
> 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 <bhutchings@solarflare.com>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Simon Horman <horms@verge.net.au>
> Signed-off-by: Cong Wang <amwang@redhat.com>
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.
>
> ---
> 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);
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-05-31 0:24 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-05-30 2:06 [Patch net-next v3] net: clean up skb headers code Cong Wang
2013-05-30 11:18 ` David Laight
2013-05-30 20:16 ` David Miller
2013-05-30 21:05 ` Ben Hutchings
2013-05-31 0:18 ` Simon Horman
2013-05-31 0:25 ` Simon Horman
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox