RE: [RFC] networking: convert many more places to skb_put_zero()
Ok, understand > -Original Message- > From: Johannes Berg [mailto:johan...@sipsolutions.net] > Sent: Thursday, June 15, 2017 3:45 PM > To: YUAN Linyu; netdev@vger.kernel.org > Subject: Re: [RFC] networking: convert many more places to skb_put_zero() > > On Thu, 2017-06-15 at 07:20 +, YUAN Linyu wrote: > > > > > In my opinion if spatch can do it even it found one place, keep it. > > Only leave difficult places like ndisc.c to me. > > It's not so simple - I'd have to tailor the spatch to it pretty much I > guess, spending far more time on the spatch than the single place > warrants. > > > > Btw, just made a patch to add and use "skb_put_data()", just doing > > > a > > > memcpy() into the skb_put() area also has lots of users. > > > > Yes, I also notice some places. > > I sent a patch. > > johannes
Re: [RFC] networking: convert many more places to skb_put_zero()
On Thu, 2017-06-15 at 07:20 +, YUAN Linyu wrote: > > > In my opinion if spatch can do it even it found one place, keep it. > Only leave difficult places like ndisc.c to me. It's not so simple - I'd have to tailor the spatch to it pretty much I guess, spending far more time on the spatch than the single place warrants. > > Btw, just made a patch to add and use "skb_put_data()", just doing > > a > > memcpy() into the skb_put() area also has lots of users. > > Yes, I also notice some places. I sent a patch. johannes
RE: [RFC] networking: convert many more places to skb_put_zero()
> -Original Message- > From: Johannes Berg [mailto:johan...@sipsolutions.net] > Sent: Thursday, June 15, 2017 3:12 PM > To: YUAN Linyu; netdev@vger.kernel.org > Subject: Re: [RFC] networking: convert many more places to skb_put_zero() > > On Thu, 2017-06-15 at 07:05 +, YUAN Linyu wrote: > > > @@ > > > type t; > > > expression skb, len; > > > identifier p; > > > @@ > > > t *p > > > - = skb_put(skb, len); > > > + = skb_put_zero(skb, len); > > > -memset(p, 0, len); > > > > > > and it can't figure out that it should remove the variable, without > > > much more work that's not really worth it for one instance :) > > > > Yes, I agree, > > it conflict with previous spatch which will keep "pad" variable, > > right? > > > > I can do it by hand if spatch not work > > I could teach spatch, but it's usually faster (for me) to post-process > the spatch changes to remove the extra variable - in this case though, > it's just not worth it at all since there's just a single change and > you already have a separate patch :) In my opinion if spatch can do it even it found one place, keep it. Only leave difficult places like ndisc.c to me. > > Btw, just made a patch to add and use "skb_put_data()", just doing a > memcpy() into the skb_put() area also has lots of users. Yes, I also notice some places. > > johannes
Re: [RFC] networking: convert many more places to skb_put_zero()
On Thu, 2017-06-15 at 07:05 +, YUAN Linyu wrote: > > @@ > > type t; > > expression skb, len; > > identifier p; > > @@ > > t *p > > - = skb_put(skb, len); > > + = skb_put_zero(skb, len); > > -memset(p, 0, len); > > > > and it can't figure out that it should remove the variable, without > > much more work that's not really worth it for one instance :) > > Yes, I agree, > it conflict with previous spatch which will keep "pad" variable, > right? > > I can do it by hand if spatch not work I could teach spatch, but it's usually faster (for me) to post-process the spatch changes to remove the extra variable - in this case though, it's just not worth it at all since there's just a single change and you already have a separate patch :) Btw, just made a patch to add and use "skb_put_data()", just doing a memcpy() into the skb_put() area also has lots of users. johannes
RE: [RFC] networking: convert many more places to skb_put_zero()
> -Original Message-
> From: Johannes Berg [mailto:johan...@sipsolutions.net]
> Sent: Thursday, June 15, 2017 2:58 PM
> To: YUAN Linyu; netdev@vger.kernel.org
> Subject: Re: [RFC] networking: convert many more places to skb_put_zero()
>
> On Thu, 2017-06-15 at 00:23 +, YUAN Linyu wrote:
> > Hi,
> > Indeed, it find more.
> > Compare with my patch, still lost pattern like below,
> > 1. sctp and openvswitch
> > --- a/net/sctp/output.c
> > +++ b/net/sctp/output.c
> > @@ -463,7 +463,7 @@ static int sctp_packet_pack(struct sctp_packet
> > *packet,
> >
> > padding = SCTP_PAD4(chunk->skb->len) -
> > chunk->skb->len;
> > if (padding)
> > - memset(skb_put(chunk->skb, padding),
> > 0, padding);
> > + skb_put_zero(chunk->skb, padding);
>
> Yep, good catch, this finds 18 instances thereof:
>
> @@
> expression skb, len;
> @@
> -memset(skb_put(skb, len), 0, len);
> +skb_put_zero(skb, len);
>
>
> > --- a/net/dsa/tag_trailer.c
> > +++ b/net/dsa/tag_trailer.c
> > @@ -43,8 +43,7 @@ static struct sk_buff *trailer_xmit(struct sk_buff
> > *skb, struct net_device *dev)
> > kfree_skb(skb);
> >
> > if (padlen) {
> > - u8 *pad = skb_put(nskb, padlen);
> > - memset(pad, 0, padlen);
> > + skb_put_zero(nskb, padlen);
>
> I'd have thought it finds this, but indeed it doesn't; there's only one
> instances this changes though:
>
> @@
> type t;
> expression skb, len;
> identifier p;
> @@
> t *p
> - = skb_put(skb, len);
> + = skb_put_zero(skb, len);
> -memset(p, 0, len);
>
> and it can't figure out that it should remove the variable, without
> much more work that's not really worth it for one instance :)
Yes, I agree,
it conflict with previous spatch which will keep "pad" variable, right?
I can do it by hand if spatch not work
>
> johannes
Re: [RFC] networking: convert many more places to skb_put_zero()
On Thu, 2017-06-15 at 00:23 +, YUAN Linyu wrote:
> Hi,
> Indeed, it find more.
> Compare with my patch, still lost pattern like below,
> 1. sctp and openvswitch
> --- a/net/sctp/output.c
> +++ b/net/sctp/output.c
> @@ -463,7 +463,7 @@ static int sctp_packet_pack(struct sctp_packet
> *packet,
>
> padding = SCTP_PAD4(chunk->skb->len) -
> chunk->skb->len;
> if (padding)
> - memset(skb_put(chunk->skb, padding),
> 0, padding);
> + skb_put_zero(chunk->skb, padding);
Yep, good catch, this finds 18 instances thereof:
@@
expression skb, len;
@@
-memset(skb_put(skb, len), 0, len);
+skb_put_zero(skb, len);
> --- a/net/dsa/tag_trailer.c
> +++ b/net/dsa/tag_trailer.c
> @@ -43,8 +43,7 @@ static struct sk_buff *trailer_xmit(struct sk_buff
> *skb, struct net_device *dev)
> kfree_skb(skb);
>
> if (padlen) {
> - u8 *pad = skb_put(nskb, padlen);
> - memset(pad, 0, padlen);
> + skb_put_zero(nskb, padlen);
I'd have thought it finds this, but indeed it doesn't; there's only one
instances this changes though:
@@
type t;
expression skb, len;
identifier p;
@@
t *p
- = skb_put(skb, len);
+ = skb_put_zero(skb, len);
-memset(p, 0, len);
and it can't figure out that it should remove the variable, without
much more work that's not really worth it for one instance :)
johannes
RE: [RFC] networking: convert many more places to skb_put_zero()
Hi,
Indeed, it find more.
Compare with my patch, still lost pattern like below,
1. sctp and openvswitch
--- a/net/sctp/output.c
+++ b/net/sctp/output.c
@@ -463,7 +463,7 @@ static int sctp_packet_pack(struct sctp_packet *packet,
padding = SCTP_PAD4(chunk->skb->len) - chunk->skb->len;
if (padding)
- memset(skb_put(chunk->skb, padding), 0,
padding);
+ skb_put_zero(chunk->skb, padding);
--- a/net/dsa/tag_trailer.c
+++ b/net/dsa/tag_trailer.c
@@ -43,8 +43,7 @@ static struct sk_buff *trailer_xmit(struct sk_buff *skb,
struct net_device *dev)
kfree_skb(skb);
if (padlen) {
- u8 *pad = skb_put(nskb, padlen);
- memset(pad, 0, padlen);
+ skb_put_zero(nskb, padlen);
I will send a separate patch for ipv6/ndisc.c once spatch done.
> -Original Message-
> From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org]
> On Behalf Of Johannes Berg
> Sent: Thursday, June 15, 2017 4:18 AM
> To: netdev@vger.kernel.org
> Cc: Johannes Berg
> Subject: [RFC] networking: convert many more places to skb_put_zero()
>
> From: Johannes Berg
>
> There were many places that my previous spatch didn't find,
> as pointed out by yuan linyu in various patches.
>
> The following spatch found many more and also removes the
> now unnecessary casts:
>
> @@
> identifier p, p2;
> expression len;
> expression skb;
> type t, t2;
> @@
> (
> -p = skb_put(skb, len);
> +p = skb_put_zero(skb, len);
> |
> -p = (t)skb_put(skb, len);
> +p = skb_put_zero(skb, len);
> )
> (
> p2 = (t2)p;
> -memset(p2, 0, len);
> |
> -memset(p, 0, len);
> )
>
> @@
> type t, t2;
> identifier p, p2;
> expression skb;
> @@
> t *p;
> ...
> (
> -p = skb_put(skb, sizeof(t));
> +p = skb_put_zero(skb, sizeof(t));
> |
> -p = (t *)skb_put(skb, sizeof(t));
> +p = skb_put_zero(skb, sizeof(t));
> )
> (
> p2 = (t2)p;
> -memset(p2, 0, sizeof(*p));
> |
> -memset(p, 0, sizeof(*p));
> )
>
