Message ID | 8729016553E3654398EA69218DA29EEF15BC8D22@cnshjmbx02 |
---|---|
State | RFC, archived |
Delegated to: | David Miller |
Headers | show |
On Thu, 2017-06-15 at 00:23 +0000, 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
> -----Original Message----- > From: Johannes Berg [mailto:johannes@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 +0000, 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
On Thu, 2017-06-15 at 07:05 +0000, 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
> -----Original Message----- > From: Johannes Berg [mailto:johannes@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 +0000, 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
On Thu, 2017-06-15 at 07:20 +0000, 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
Ok, understand > -----Original Message----- > From: Johannes Berg [mailto:johannes@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 +0000, 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
--- 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)