[PATCH] skbuff: make skb_put_zero() return void

[PATCH] skbuff: make skb_put_zero() return void

[Johannes Berg]

From: Johannes Berg <johannes.berg@intel.com>

It's nicer to return void, since then there's no need to
cast to any structures. Currently none of the users have
a cast, but a number of future conversions do.

Signed-off-by: Johannes Berg <johannes.berg@intel.com>
---
 include/linux/skbuff.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 1151b50892d1..01ea64d0783a 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1904,9 +1904,9 @@ static inline unsigned char *__skb_put(struct sk_buff *skb, unsigned int len)
 	return tmp;
 }
 
-static inline unsigned char *skb_put_zero(struct sk_buff *skb, unsigned int len)
+static inline void *skb_put_zero(struct sk_buff *skb, unsigned int len)
 {
-	unsigned char *tmp = skb_put(skb, len);
+	void *tmp = skb_put(skb, len);
 
 	memset(tmp, 0, len);
 
-- 
2.11.0

Re: [PATCH] skbuff: make skb_put_zero() return void

[Joe Perches]

On Wed, 2017-06-14 at 22:17 +0200, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
> 
> It's nicer to return void, since then there's no need to
> cast to any structures. Currently none of the users have
> a cast, but a number of future conversions do.
> 
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
>  include/linux/skbuff.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 1151b50892d1..01ea64d0783a 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -1904,9 +1904,9 @@ static inline unsigned char *__skb_put(struct sk_buff *skb, unsigned int len)
>  	return tmp;
>  }
>  
> -static inline unsigned char *skb_put_zero(struct sk_buff *skb, unsigned int len)
> +static inline void *skb_put_zero(struct sk_buff *skb, unsigned int len)
>  {
> -	unsigned char *tmp = skb_put(skb, len);
> +	void *tmp = skb_put(skb, len);
>  
>  	memset(tmp, 0, len);

Given you are adding a lot of these, it might be better
to add an exported function that duplicates most of
skb_put with a memset at the end.

That would probably create a smaller kernel and might
be a bit faster.

Perhaps:
---
 include/linux/skbuff.h | 10 +---------
 net/core/skbuff.c      | 22 ++++++++++++++++++++++
 2 files changed, 23 insertions(+), 9 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 78213b3f9552..f725cbe30c9c 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1895,6 +1895,7 @@ static inline void skb_set_tail_pointer(struct sk_buff *skb, const int offset)
  */
 unsigned char *pskb_put(struct sk_buff *skb, struct sk_buff *tail, int len);
 unsigned char *skb_put(struct sk_buff *skb, unsigned int len);
+void *skb_put_zero(struct sk_buff *skb, unsigned int len);
 static inline unsigned char *__skb_put(struct sk_buff *skb, unsigned int len)
 {
 	unsigned char *tmp = skb_tail_pointer(skb);
@@ -1904,15 +1905,6 @@ static inline unsigned char *__skb_put(struct sk_buff *skb, unsigned int len)
 	return tmp;
 }
 
-static inline unsigned char *skb_put_zero(struct sk_buff *skb, unsigned int len)
-{
-	unsigned char *tmp = skb_put(skb, len);
-
-	memset(tmp, 0, len);
-
-	return tmp;
-}
-
 unsigned char *skb_push(struct sk_buff *skb, unsigned int len);
 static inline unsigned char *__skb_push(struct sk_buff *skb, unsigned int len)
 {
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 304602784c3b..e70aa414b139 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1454,6 +1454,28 @@ unsigned char *skb_put(struct sk_buff *skb, unsigned int len)
 EXPORT_SYMBOL(skb_put);
 
 /**
+ *	skb_put_zero - add zeroed data to a buffer
+ *	@skb: buffer to use
+ *	@len: amount of zeroed data to add
+ *
+ *	This function extends the used data area of the buffer. If this would
+ *	exceed the total buffer size the kernel will panic. A pointer to the
+ *	first byte of the extra data is returned.
+ */
+void *skb_put_zero(struct sk_buff *skb, unsigned int len)
+{
+	unsigned char *tmp = skb_tail_pointer(skb);
+
+	SKB_LINEAR_ASSERT(skb);
+	skb->tail += len;
+	skb->len  += len;
+	if (unlikely(skb->tail > skb->end))
+		skb_over_panic(skb, len, __builtin_return_address(0));
+	return memset(tmp, 0, len);
+}
+EXPORT_SYMBOL(skb_put_zero);
+
+/**
  *	skb_push - add data to the start of a buffer
  *	@skb: buffer to use
  *	@len: amount of data to add

Re: [PATCH] skbuff: make skb_put_zero() return void

[Johannes Berg]

On Wed, 2017-06-14 at 13:36 -0700, Joe Perches wrote:
> 
> Given you are adding a lot of these, it might be better
> to add an exported function that duplicates most of
> skb_put with a memset at the end.

Yeah, could be done. I'm not sure why you'd want to duplicate it rather
than call it though? To make it about as fast?

Anyway, no objections really - just originally thought of it more as a
typing aid than an optimisation.

johannes

Re: [PATCH] skbuff: make skb_put_zero() return void

[Joe Perches]

On Wed, 2017-06-14 at 22:40 +0200, Johannes Berg wrote:
> On Wed, 2017-06-14 at 13:36 -0700, Joe Perches wrote:
> > 
> > Given you are adding a lot of these, it might be better
> > to add an exported function that duplicates most of
> > skb_put with a memset at the end.
> 
> Yeah, could be done. I'm not sure why you'd want to duplicate it rather
> than call it though? To make it about as fast?

Yeah, that and reduced stack use.

Dunno how performance sensitive these uses really are
but it seems some might be for slow cpu wireless APs in
both the rx and tx paths.

Re: [PATCH] skbuff: make skb_put_zero() return void

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Wed, 14 Jun 2017 22:17:20 +0200

> From: Johannes Berg <johannes.berg@intel.com>
> 
> It's nicer to return void, since then there's no need to
> cast to any structures. Currently none of the users have
> a cast, but a number of future conversions do.
> 
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>

Applied.

Although a bit disruptive, it might be nice to convert all of the
other "char *" related data pointers in skbuff based interfaces.

No, I'm not asking you specifically to work on this, relax :)

Re: [PATCH] skbuff: make skb_put_zero() return void

[Johannes Berg]

On Thu, 2017-06-15 at 12:18 -0400, David Miller wrote:

> Although a bit disruptive, it might be nice to convert all of the
> other "char *" related data pointers in skbuff based interfaces.

I think it'd actually be pretty easy, since there are very few cases
where you need non-void, e.g.

*skb_put(skb, 1) = 'x';

Seems pretty unlikely we have that, and in any case the compiler would
warn (error?) there if skb_put() becomes void.

> No, I'm not asking you specifically to work on this, relax :)

:-)

johannes

Re: [PATCH] skbuff: make skb_put_zero() return void

[David Miller]

From: Johannes Berg <johannes@sipsolutions.net>
Date: Thu, 15 Jun 2017 21:28:32 +0200

> On Thu, 2017-06-15 at 12:18 -0400, David Miller wrote:
> 
>> Although a bit disruptive, it might be nice to convert all of the
>> other "char *" related data pointers in skbuff based interfaces.
> 
> I think it'd actually be pretty easy, since there are very few cases
> where you need non-void, e.g.
> 
> *skb_put(skb, 1) = 'x';
> 
> Seems pretty unlikely we have that, and in any case the compiler would
> warn (error?) there if skb_put() becomes void.

Actually I am pretty sure I've seen a pattern like that somewhere. :-)

Re: [PATCH] skbuff: make skb_put_zero() return void

[Johannes Berg]

On Thu, 2017-06-15 at 17:26 -0400, David Miller wrote:
> 
> > *skb_put(skb, 1) = 'x';
> > 
> > Seems pretty unlikely we have that, and in any case the compiler
> would
> > warn (error?) there if skb_put() becomes void.
> 
> Actually I am pretty sure I've seen a pattern like that somewhere. :-
> )

Yeah, there are actually a ton of them, and oddly enough my spatch is
failing to catch _one_ of them?? Still refining it :)

johannes

Re: [PATCH] skbuff: make skb_put_zero() return void

[Joe Perches]

On Thu, 2017-06-15 at 23:28 +0200, Johannes Berg wrote:
> On Thu, 2017-06-15 at 17:26 -0400, David Miller wrote:
> > 
> > > *skb_put(skb, 1) = 'x';
> > >  
> > > Seems pretty unlikely we have that, and in any case the compiler
> > 
> > would
> > > warn (error?) there if skb_put() becomes void.
> > 
> > Actually I am pretty sure I've seen a pattern like that somewhere. :-
> > )
> 
> Yeah, there are actually a ton of them, and oddly enough my spatch is
> failing to catch _one_ of them?? Still refining it :)

I suggest changing those to skb_put_char(skb, char)
in a first pass and then doing the other bits later.

Here's a script that does the conversion.

$ /usr/bin/git grep -P --name-only "\*\s*skb_put\s*\(\s*([\w\.\[\]\>\-]+)\s*,\s*1\s*\)\s*=\s*([^;]+);" | \
  xargs perl -p -i -e 's/\*\s*skb_put\s*\(\s*([\w\.\[\]\>\-]+)\s*,\s*1\s*\)\s*=\s*([^;]+);/skb_put_char(\1, \2);/'

Re: [PATCH] skbuff: make skb_put_zero() return void

[Johannes Berg]

On Wed, 2017-06-14 at 14:18 -0700, Joe Perches wrote:
> On Wed, 2017-06-14 at 22:40 +0200, Johannes Berg wrote:
> > On Wed, 2017-06-14 at 13:36 -0700, Joe Perches wrote:
> > > 
> > > Given you are adding a lot of these, it might be better
> > > to add an exported function that duplicates most of
> > > skb_put with a memset at the end.
> > 
> > Yeah, could be done. I'm not sure why you'd want to duplicate it
> > rather
> > than call it though? To make it about as fast?
> 
> Yeah, that and reduced stack use.
> 
> Dunno how performance sensitive these uses really are
> but it seems some might be for slow cpu wireless APs in
> both the rx and tx paths.

I haven't really checked now, but the wireless (mac80211) ones I saw
weren't in the data TX/RX, only for management SKBs which are pretty
much a slowpath.

Anyway, I guess you know how to propose a patch with this :-)

However, I think in that case there should be something like
skb_pull_inline, so that the skb_put code here isn't all copied around,
but just lives in a single place that gets inlined into skb_put() and
skb_put_zero().

johannes

Re: [PATCH] skbuff: make skb_put_zero() return void

[Johannes Berg]

On Thu, 2017-06-15 at 15:17 -0700, Joe Perches wrote:

> I suggest changing those to skb_put_char(skb, char)

That might be something to think of, but you can't really know for sure
that they're not using len > 1 and don't yet care about the other bytes
or something. That'd probably be another bug, but ... dunno

And anyway, I think

*(u8 *)skb_put(skb, 1) = c;

isn't really that bad. Obviously that could be converted further to
skb_put_char(), using a simple spatch:

@@
expression SKB, C;
@@
- *(u8 *)skb_put(SKB, 1) = C;
+ skb_put_char(SKB, C);


> Here's a script that does the conversion.
> 
> $ /usr/bin/git grep -P --name-only
> "\*\s*skb_put\s*\(\s*([\w\.\[\]\>\-]+)\s*,\s*1\s*\)\s*=\s*([^;]+);" |
> \
>   xargs perl -p -i -e 's/\*\s*skb_put\s*\(\s*([\w\.\[\]\>\-
> ]+)\s*,\s*1\s*\)\s*=\s*([^;]+);/skb_put_char(\1, \2);/'

Uh, I think you're using the wrong tool for the job :-)

johannes

Re: [PATCH] skbuff: make skb_put_zero() return void

[Johannes Berg]

On Thu, 2017-06-15 at 15:17 -0700, Joe Perches wrote:

> Here's a script that does the conversion.
> 
> $ /usr/bin/git grep -P --name-only
> "\*\s*skb_put\s*\(\s*([\w\.\[\]\>\-]+)\s*,\s*1\s*\)\s*=\s*([^;]+);" |
> \
>   xargs perl -p -i -e 's/\*\s*skb_put\s*\(\s*([\w\.\[\]\>\-
> ]+)\s*,\s*1\s*\)\s*=\s*([^;]+);/skb_put_char(\1, \2);/'

Btw, this is incomplete - you have "\*\s*" at the beginning, but there
are cases like

 *(skb_put(skb, 1)) = c;

where you have extra parentheses. By just adding them to the spatch, it
finds both cases trivially.

I'm much more comfortable using spatch to do things like this.

johannes

Re: [PATCH] skbuff: make skb_put_zero() return void

[Joe Perches]

On Fri, 2017-06-16 at 00:21 +0200, Johannes Berg wrote:
> On Thu, 2017-06-15 at 15:17 -0700, Joe Perches wrote:
> 
> > I suggest changing those to skb_put_char(skb, char)
> 
> That might be something to think of, but you can't really know for sure
> that they're not using len > 1 and don't yet care about the other bytes
> or something. That'd probably be another bug, but ... dunno
> 
> And anyway, I think
> 
> *(u8 *)skb_put(skb, 1) = c;
> 
> isn't really that bad. Obviously that could be converted further to
> skb_put_char(), using a simple spatch:
> 
> @@
> expression SKB, C;
> @@
> - *(u8 *)skb_put(SKB, 1) = C;
> + skb_put_char(SKB, C);
> 
> 
> > Here's a script that does the conversion.
> > 
> > $ /usr/bin/git grep -P --name-only
> > "\*\s*skb_put\s*\(\s*([\w\.\[\]\>\-]+)\s*,\s*1\s*\)\s*=\s*([^;]+);" |
> > \
> >   xargs perl -p -i -e 's/\*\s*skb_put\s*\(\s*([\w\.\[\]\>\-
> > ]+)\s*,\s*1\s*\)\s*=\s*([^;]+);/skb_put_char(\1, \2);/'
> 
> Uh, I think you're using the wrong tool for the job :-)

I'm familiar with both.

It depends on how much you want to wait.

The thing I wrote finished in about 2 seconds
on my little laptop.

cheers, Joe

Re: [PATCH] skbuff: make skb_put_zero() return void

[Joe Perches]

On Fri, 2017-06-16 at 00:23 +0200, Johannes Berg wrote:
> On Thu, 2017-06-15 at 15:17 -0700, Joe Perches wrote:
> 
> > Here's a script that does the conversion.
> > 
> > $ /usr/bin/git grep -P --name-only
> > "\*\s*skb_put\s*\(\s*([\w\.\[\]\>\-]+)\s*,\s*1\s*\)\s*=\s*([^;]+);" |
> > \
> >   xargs perl -p -i -e 's/\*\s*skb_put\s*\(\s*([\w\.\[\]\>\-
> > ]+)\s*,\s*1\s*\)\s*=\s*([^;]+);/skb_put_char(\1, \2);/'
> 
> Btw, this is incomplete - you have "\*\s*" at the beginning, but there
> are cases like
> 
>  *(skb_put(skb, 1)) = c;
> 
> where you have extra parentheses. By just adding them to the spatch, it
> finds both cases trivially.
> 
> I'm much more comfortable using spatch to do things like this.

Knock your self out.
Whatever floats your boat.
Have at it.
Go get 'em.

etc...

There are also some uses like:

	memcpy(skb_put(h5->rx_skb, 1), byte, 1);

that could also be converted.

cheers, Joe

Re: [PATCH] skbuff: make skb_put_zero() return void

[Joe Perches]

On Fri, 2017-06-16 at 00:17 +0200, Johannes Berg wrote:
> On Wed, 2017-06-14 at 14:18 -0700, Joe Perches wrote:
> > On Wed, 2017-06-14 at 22:40 +0200, Johannes Berg wrote:
> > > On Wed, 2017-06-14 at 13:36 -0700, Joe Perches wrote:
> > > > 
> > > > Given you are adding a lot of these, it might be better
> > > > to add an exported function that duplicates most of
> > > > skb_put with a memset at the end.
> > > 
> > > Yeah, could be done. I'm not sure why you'd want to duplicate it
> > > rather
> > > than call it though? To make it about as fast?
> > 
> > Yeah, that and reduced stack use.
> > 
> > Dunno how performance sensitive these uses really are
> > but it seems some might be for slow cpu wireless APs in
> > both the rx and tx paths.
> 
> I haven't really checked now, but the wireless (mac80211) ones I saw
> weren't in the data TX/RX, only for management SKBs which are pretty
> much a slowpath.
> 
> Anyway, I guess you know how to propose a patch with this :-)

I'll wait as I don't want to cause patch conflicts.

> However, I think in that case there should be something like
> skb_pull_inline, so that the skb_put code here isn't all copied around,
> but just lives in a single place that gets inlined into skb_put() and
> skb_put_zero().

Seems sensible.