[PATCH net-next 00/11] skb_put_zero() used to optimize code

From: yuan linyu <redacted>


yuan linyu (11):
  ipv6: skb_put_zero() used to optimize code
  ipv4: skb_put_zero() used to optimize code
  ncsi: skb_put_zero() used to optimize code
  dsa: skb_put_zero() used to optimize code
  key: skb_put_zero() used to optimize code
  openvswitch: skb_put_zero() used to optimize code
  batman-adv: skb_put_zero() used to optimize code
  bridge: skb_put_zero() used to optimize code
  sctp: skb_put_zero() used to optimize code
  atm: skb_put_zero() used to optimize code
  qrtr: skb_put_zero() used to optimize code

 net/atm/signaling.c                      |  3 +--
 net/batman-adv/bat_v_elp.c               |  3 +--
 net/bridge/netfilter/nft_reject_bridge.c |  6 ++----
 net/dsa/tag_trailer.c                    |  3 +--
 net/ipv4/ipconfig.c                      |  3 +--
 net/ipv4/netfilter/nf_reject_ipv4.c      |  3 +--
 net/ipv6/mcast.c                         |  3 +--
 net/ipv6/ndisc.c                         | 13 ++----------
 net/key/af_key.c                         | 12 ++++-------
 net/ncsi/ncsi-cmd.c                      | 36 +++++++++++---------------------
 net/openvswitch/datapath.c               |  2 +-
 net/qrtr/qrtr.c                          |  9 +++-----
 net/sctp/output.c                        |  2 +-
 13 files changed, 31 insertions(+), 67 deletions(-)

-- 
2.7.4

On Wed, 2017-06-14 at 22:30 +0800, yuan linyu wrote:

From: yuan linyu <redacted>


yuan linyu (11):
  ipv6: skb_put_zero() used to optimize code
  ipv4: skb_put_zero() used to optimize code
  ncsi: skb_put_zero() used to optimize code
  dsa: skb_put_zero() used to optimize code
  key: skb_put_zero() used to optimize code
  openvswitch: skb_put_zero() used to optimize code
  batman-adv: skb_put_zero() used to optimize code
  bridge: skb_put_zero() used to optimize code
  sctp: skb_put_zero() used to optimize code
  atm: skb_put_zero() used to optimize code
  qrtr: skb_put_zero() used to optimize code

Hmm. I guess my spatch missed these because of the casts present in
them.

Surely you didn't do them by hand, so maybe you should provide a single
big patch with the spatch?

johannes

On 三, 2017-06-14 at 16:42 +0200, Johannes Berg wrote:

On Wed, 2017-06-14 at 22:30 +0800, yuan linyu wrote:

quoted

From: yuan linyu <redacted>


yuan linyu (11):
  ipv6: skb_put_zero() used to optimize code
  ipv4: skb_put_zero() used to optimize code
  ncsi: skb_put_zero() used to optimize code
  dsa: skb_put_zero() used to optimize code
  key: skb_put_zero() used to optimize code
  openvswitch: skb_put_zero() used to optimize code
  batman-adv: skb_put_zero() used to optimize code
  bridge: skb_put_zero() used to optimize code
  sctp: skb_put_zero() used to optimize code
  atm: skb_put_zero() used to optimize code
  qrtr: skb_put_zero() used to optimize code

Hmm. I guess my spatch missed these because of the casts present in
them.

Surely you didn't do them by hand, so maybe you should provide a single
big patch with the spatch?

sorry, I do them by hand. 
last time when i try to change ndisc.c, david adhern suggest to use skb_put_zero().

johannes

On Wed, 2017-06-14 at 23:00 +0800, yuan linyu wrote:

sorry, I do them by hand. 

oh, ok.

This seems to work:

@@
identifier p;
expression len;
expression skb;
type t;
@@
-p = (t)skb_put(skb, len);
-memset(p, 0, len);
+p = (t)skb_put_zero(skb, len);

johannes

On Wed, 2017-06-14 at 18:29 +0200, Johannes Berg wrote:

On Wed, 2017-06-14 at 23:00 +0800, yuan linyu wrote:

quoted

sorry, I do them by hand. 

oh, ok.

This seems to work:

@@
identifier p;
expression len;
expression skb;
type t;
@@
-p = (t)skb_put(skb, len);
-memset(p, 0, len);
+p = (t)skb_put_zero(skb, len);

Nope, still didn't find everything - not the batman and atm cases for
example. But still a few more, I think that you didn't find, e.g. in
mac80211.

Anyway - I think none of this is really critical.

johannes

From: Johannes Berg <johannes@sipsolutions.net>
Date: Wed, 14 Jun 2017 18:31:48 +0200

On Wed, 2017-06-14 at 18:29 +0200, Johannes Berg wrote:

quoted

On Wed, 2017-06-14 at 23:00 +0800, yuan linyu wrote:

quoted

sorry, I do them by hand. 

oh, ok.

This seems to work:

@@
identifier p;
expression len;
expression skb;
type t;
@@
-p = (t)skb_put(skb, len);
-memset(p, 0, len);
+p = (t)skb_put_zero(skb, len);

Nope, still didn't find everything - not the batman and atm cases for
example. But still a few more, I think that you didn't find, e.g. in
mac80211.

Anyway - I think none of this is really critical.

What do you think about Stephen Hemminger's feedback to make skb_put_zero()
return "void *".  It will avoid a lot of casts and make the conversions
look nicer.

I'll also defer on this patch series until we have the spatch thing
which can catch all of them.

Thanks.

On Wed, 2017-06-14 at 15:38 -0400, David Miller wrote:

What do you think about Stephen Hemminger's feedback to make
skb_put_zero() return "void *".  It will avoid a lot of casts and
make the conversions look nicer.

I think it's a good idea. I can send a patch, and remove some casts
where they exist - I guess we could also do it even for skb_put()
eventually.

I'll also defer on this patch series until we have the spatch thing
which can catch all of them.

I think I have something that covers most cases, will send out an RFC
in a minute (since I'm not sure I was able to build it all yet).

Note that some of the cases like in ipv6/ndisc.c certainly can't be
done automatically, they look correct but are more complicated because
there's not just a single big memset(), but they're doing it piece by
piece (fill some bytes, clear some others, etc.) Might be worth
checking which aren't covered by the spatch.

johannes

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

On Wed, 2017-06-14 at 15:38 -0400, David Miller wrote:

quoted

What do you think about Stephen Hemminger's feedback to make
skb_put_zero() return "void *".  It will avoid a lot of casts and
make the conversions look nicer.

I think it's a good idea. I can send a patch, and remove some casts
where they exist - I guess we could also do it even for skb_put()
eventually.

quoted

I'll also defer on this patch series until we have the spatch thing
which can catch all of them.

I think I have something that covers most cases, will send out an RFC
in a minute (since I'm not sure I was able to build it all yet).

All sounds great.

Note that some of the cases like in ipv6/ndisc.c certainly can't be
done automatically, they look correct but are more complicated because
there's not just a single big memset(), but they're doing it piece by
piece (fill some bytes, clear some others, etc.) Might be worth
checking which aren't covered by the spatch.

Understood.

-----Original Message-----
From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org]
On Behalf Of Johannes Berg
Sent: Thursday, June 15, 2017 12:32 AM
To: cugyly@163.com; netdev@vger.kernel.org
Cc: David S . Miller; YUAN Linyu
Subject: Re: [PATCH net-next 00/11] skb_put_zero() used to optimize code

On Wed, 2017-06-14 at 18:29 +0200, Johannes Berg wrote:

quoted

On Wed, 2017-06-14 at 23:00 +0800, yuan linyu wrote:

quoted

sorry, I do them by hand.

oh, ok.

This seems to work:

@@
identifier p;
expression len;
expression skb;
type t;
@@
-p = (t)skb_put(skb, len);
-memset(p, 0, len);
+p = (t)skb_put_zero(skb, len);

Nope, still didn't find everything - not the batman and atm cases for
example. But still a few more, I think that you didn't find, e.g. in
mac80211.

Yes, I thought you already done in mac80211

Anyway - I think none of this is really critical.

johannes

On Wed, 2017-06-14 at 23:45 +0000, YUAN Linyu wrote:

quoted

But still a few more, I think that you didn't find, e.g.
in mac80211.

Yes, I thought you already done in mac80211

So did I, but I also only applied my spatch :)

johannes