| Message ID | 1312907945-1982-1-git-send-email-wangshaoyan.pt@taobao.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
Le mercredi 10 août 2011 à 00:39 +0800, stufever@gmail.com a écrit : > From: Wang Shaoyan <wangshaoyan.pt@taobao.com> > > drivers/net/gianfar_ethtool.c:765: warning: the frame size of 2048 bytes is larger than 1024 bytes > > Signed-off-by: Wang Shaoyan <wangshaoyan.pt@taobao.com> > --- > drivers/net/gianfar_ethtool.c | 20 +++++++++++++++++--- > 1 files changed, 17 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/gianfar_ethtool.c b/drivers/net/gianfar_ethtool.c > index 6e35069..134fe1b 100644 > --- a/drivers/net/gianfar_ethtool.c > +++ b/drivers/net/gianfar_ethtool.c > @@ -686,10 +686,21 @@ static int gfar_ethflow_to_filer_table(struct gfar_private *priv, u64 ethflow, u > { > unsigned int last_rule_idx = priv->cur_filer_idx; > unsigned int cmp_rqfpr; > - unsigned int local_rqfpr[MAX_FILER_IDX + 1]; > - unsigned int local_rqfcr[MAX_FILER_IDX + 1]; > + unsigned int *local_rqfpr; > + unsigned int *local_rqfcr; > int i = 0x0, k = 0x0; > int j = MAX_FILER_IDX, l = 0x0; > + int ret = 1; > + > + local_rqfpr = kmalloc(sizeof(unsigned int) * (MAX_FILER_IDX + 1), > + GFP_KERNEL); > + local_rqfcr = kmalloc(sizeof(unsigned int) * (MAX_FILER_IDX + 1), > + GFP_KERNEL); > + if (!local_rqfpr || !local_rqfcr) { > + pr_err("Out of memory\n"); Please remove this pr_err(), kmalloc() will complain already. > + ret = 0; > + got err; > + } > > switch (class) { > case TCP_V4_FLOW: > @@ -765,7 +776,10 @@ static int gfar_ethflow_to_filer_table(struct gfar_private *priv, u64 ethflow, u > priv->cur_filer_idx = priv->cur_filer_idx - 1; > } > > - return 1; > +err: > + kfree(local_rqfcr); > + kfree(local_rqfpr); > + return ret; > } > You should now track all "return 0;" done in this function to make sure no memory leak is added by your patch. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2011-08-09 at 18:53 +0200, Eric Dumazet wrote: > Le mercredi 10 août 2011 à 00:39 +0800, stufever@gmail.com a écrit : [] > > + if (!local_rqfpr || !local_rqfcr) { > > + pr_err("Out of memory\n"); > Please remove this pr_err(), kmalloc() will complain already. Always? I know there's a trace option, but is it always on? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le mardi 09 août 2011 à 10:52 -0700, Joe Perches a écrit : > On Tue, 2011-08-09 at 18:53 +0200, Eric Dumazet wrote: > > Le mercredi 10 août 2011 à 00:39 +0800, stufever@gmail.com a écrit : > [] > > > + if (!local_rqfpr || !local_rqfcr) { > > > + pr_err("Out of memory\n"); > > Please remove this pr_err(), kmalloc() will complain already. > > Always? > I know there's a trace option, but is it always on? > Yes, unless caller added ___GFP_NOWARN in gfp : ptr = kmalloc(size, GFP_KERNEL | ___GFP_NOWARN); -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2011-08-09 at 19:59 +0200, Eric Dumazet wrote: > Le mardi 09 août 2011 à 10:52 -0700, Joe Perches a écrit : > > On Tue, 2011-08-09 at 18:53 +0200, Eric Dumazet wrote: > > > Le mercredi 10 août 2011 à 00:39 +0800, stufever@gmail.com a écrit : > > [] > > > > + if (!local_rqfpr || !local_rqfcr) { > > > > + pr_err("Out of memory\n"); > > > Please remove this pr_err(), kmalloc() will complain already. > > Always? > > I know there's a trace option, but is it always on? > Yes, unless caller added ___GFP_NOWARN in gfp : > ptr = kmalloc(size, GFP_KERNEL | ___GFP_NOWARN); Isn't that true only for slub? Do you know where this get emitted? I looked cursorily but I don't see it. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le mardi 09 août 2011 à 11:12 -0700, Joe Perches a écrit : > On Tue, 2011-08-09 at 19:59 +0200, Eric Dumazet wrote: > > Le mardi 09 août 2011 à 10:52 -0700, Joe Perches a écrit : > > > On Tue, 2011-08-09 at 18:53 +0200, Eric Dumazet wrote: > > > > Le mercredi 10 août 2011 à 00:39 +0800, stufever@gmail.com a écrit : > > > [] > > > > > + if (!local_rqfpr || !local_rqfcr) { > > > > > + pr_err("Out of memory\n"); > > > > Please remove this pr_err(), kmalloc() will complain already. > > > Always? > > > I know there's a trace option, but is it always on? > > Yes, unless caller added ___GFP_NOWARN in gfp : > > ptr = kmalloc(size, GFP_KERNEL | ___GFP_NOWARN); > > Isn't that true only for slub? > nope, this is page allocation, so common to slab/slub/... > Do you know where this get emitted? > I looked cursorily but I don't see it. > This is a bit beyond this thread Look at mm/page_alloc.c void warn_alloc_failed(gfp_t gfp_mask, int order, const char *fmt, ...) -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2011-08-09 at 20:24 +0200, Eric Dumazet wrote: > Le mardi 09 août 2011 à 11:12 -0700, Joe Perches a écrit : > > On Tue, 2011-08-09 at 19:59 +0200, Eric Dumazet wrote: > > > Le mardi 09 août 2011 à 10:52 -0700, Joe Perches a écrit : > > > > On Tue, 2011-08-09 at 18:53 +0200, Eric Dumazet wrote: > > > > > Le mercredi 10 août 2011 à 00:39 +0800, stufever@gmail.com a écrit : > > > > [] > > > > > > + if (!local_rqfpr || !local_rqfcr) { > > > > > > + pr_err("Out of memory\n"); > > > > > Please remove this pr_err(), kmalloc() will complain already. I think this is fine and should be kept until some general agreement is made that OOM messages should be removed generically. If these are really superfluous, which I doubt a little because these are emitted at different KERN_<LEVEL>, (the generic one emits at KERN_WARNING), there are _thousands_ of these OOM errors in drivers/ alone that could be removed. $ grep -rP --include=*.[ch] \ "(printk|\b[a-z]+_\w+)\s*\(.*\".*(alloc|mem)" drivers | \ wc -l 5147 call it 50% false positive, it's still a lot. I think one more won't hurt for awhile. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2011/8/10 Eric Dumazet <eric.dumazet@gmail.com>: > Le mercredi 10 août 2011 à 00:39 +0800, stufever@gmail.com a écrit : >> From: Wang Shaoyan <wangshaoyan.pt@taobao.com> >> >> drivers/net/gianfar_ethtool.c:765: warning: the frame size of 2048 bytes is larger than 1024 bytes >> >> Signed-off-by: Wang Shaoyan <wangshaoyan.pt@taobao.com> >> --- >> drivers/net/gianfar_ethtool.c | 20 +++++++++++++++++--- >> 1 files changed, 17 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/net/gianfar_ethtool.c b/drivers/net/gianfar_ethtool.c >> index 6e35069..134fe1b 100644 >> --- a/drivers/net/gianfar_ethtool.c >> +++ b/drivers/net/gianfar_ethtool.c >> @@ -686,10 +686,21 @@ static int gfar_ethflow_to_filer_table(struct gfar_private *priv, u64 ethflow, u >> { >> unsigned int last_rule_idx = priv->cur_filer_idx; >> unsigned int cmp_rqfpr; >> - unsigned int local_rqfpr[MAX_FILER_IDX + 1]; >> - unsigned int local_rqfcr[MAX_FILER_IDX + 1]; >> + unsigned int *local_rqfpr; >> + unsigned int *local_rqfcr; >> int i = 0x0, k = 0x0; >> int j = MAX_FILER_IDX, l = 0x0; >> + int ret = 1; >> + >> + local_rqfpr = kmalloc(sizeof(unsigned int) * (MAX_FILER_IDX + 1), >> + GFP_KERNEL); >> + local_rqfcr = kmalloc(sizeof(unsigned int) * (MAX_FILER_IDX + 1), >> + GFP_KERNEL); >> + if (!local_rqfpr || !local_rqfcr) { >> + pr_err("Out of memory\n"); > > Please remove this pr_err(), kmalloc() will complain already. > >> + ret = 0; >> + got err; >> + } >> >> switch (class) { >> case TCP_V4_FLOW: >> @@ -765,7 +776,10 @@ static int gfar_ethflow_to_filer_table(struct gfar_private *priv, u64 ethflow, u >> priv->cur_filer_idx = priv->cur_filer_idx - 1; >> } >> >> - return 1; >> +err: >> + kfree(local_rqfcr); >> + kfree(local_rqfpr); >> + return ret; >> } >> > Thanks, I will do it > You should now track all "return 0;" done in this function to make sure > no memory leak is added by your patch. > > > >
diff --git a/drivers/net/gianfar_ethtool.c b/drivers/net/gianfar_ethtool.c index 6e35069..134fe1b 100644 --- a/drivers/net/gianfar_ethtool.c +++ b/drivers/net/gianfar_ethtool.c @@ -686,10 +686,21 @@ static int gfar_ethflow_to_filer_table(struct gfar_private *priv, u64 ethflow, u { unsigned int last_rule_idx = priv->cur_filer_idx; unsigned int cmp_rqfpr; - unsigned int local_rqfpr[MAX_FILER_IDX + 1]; - unsigned int local_rqfcr[MAX_FILER_IDX + 1]; + unsigned int *local_rqfpr; + unsigned int *local_rqfcr; int i = 0x0, k = 0x0; int j = MAX_FILER_IDX, l = 0x0; + int ret = 1; + + local_rqfpr = kmalloc(sizeof(unsigned int) * (MAX_FILER_IDX + 1), + GFP_KERNEL); + local_rqfcr = kmalloc(sizeof(unsigned int) * (MAX_FILER_IDX + 1), + GFP_KERNEL); + if (!local_rqfpr || !local_rqfcr) { + pr_err("Out of memory\n"); + ret = 0; + got err; + } switch (class) { case TCP_V4_FLOW: @@ -765,7 +776,10 @@ static int gfar_ethflow_to_filer_table(struct gfar_private *priv, u64 ethflow, u priv->cur_filer_idx = priv->cur_filer_idx - 1; } - return 1; +err: + kfree(local_rqfcr); + kfree(local_rqfpr); + return ret; } static int gfar_set_hash_opts(struct gfar_private *priv, struct ethtool_rxnfc *cmd)