Patchwork gianfar: reduce stack usage in gianfar_ethtool.c

login
register
mail settings
Submitter stufever@gmail.com
Date Aug. 9, 2011, 4:39 p.m.
Message ID <1312907945-1982-1-git-send-email-wangshaoyan.pt@taobao.com>
Download mbox | patch
Permalink /patch/109259/
State Superseded
Delegated to: David Miller
Headers show

Comments

stufever@gmail.com - Aug. 9, 2011, 4:39 p.m.
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(-)
Eric Dumazet - Aug. 9, 2011, 4:53 p.m.
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
Joe Perches - Aug. 9, 2011, 5:52 p.m.
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
Eric Dumazet - Aug. 9, 2011, 5:59 p.m.
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
Joe Perches - Aug. 9, 2011, 6:12 p.m.
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
Eric Dumazet - Aug. 9, 2011, 6:24 p.m.
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
Joe Perches - Aug. 9, 2011, 7:57 p.m.
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
stufever@gmail.com - Aug. 10, 2011, 2:20 a.m.
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.
>
>
>
>

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)