[PATCH] gianfar: reduce stack usage in gianfar_ethtool.c

[PATCH] gianfar: reduce stack usage in gianfar_ethtool.c

[stufever]

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 |   26 +++++++++++++++++++++-----
 1 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/net/gianfar_ethtool.c b/drivers/net/gianfar_ethtool.c
index 6e35069..a4777d2 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:
@@ -706,7 +717,8 @@ static int gfar_ethflow_to_filer_table(struct gfar_private *priv, u64 ethflow, u
 		break;
 	default:
 		pr_err("Right now this class is not supported\n");
-		return 0;
+		ret = 0;
+		goto err;
 	}
 
 	for (i = 0; i < MAX_FILER_IDX + 1; i++) {
@@ -721,7 +733,8 @@ static int gfar_ethflow_to_filer_table(struct gfar_private *priv, u64 ethflow, u
 
 	if (i == MAX_FILER_IDX + 1) {
 		pr_err("No parse rule found, can't create hash rules\n");
-		return 0;
+		ret = 0;
+		goto err;
 	}
 
 	/* If a match was found, then it begins the starting of a cluster rule
@@ -765,7 +778,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)
-- 
1.7.4.1

Re: [PATCH] gianfar: reduce stack usage in gianfar_ethtool.c

[Joe Perches]

On Wed, 2011-08-10 at 10:29 +0800, stufever@gmail.com wrote:
> Signed-off-by: Wang Shaoyan <wangshaoyan.pt@taobao.com>
[]
> diff --git 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
[]
> +	if (!local_rqfpr || !local_rqfcr) {
> +		pr_err("Out of memory\n");
> +		ret = 0;
> +		got err;

"got err" is true.

Please compile test your patches before sending them.

Re: [PATCH] gianfar: reduce stack usage in gianfar_ethtool.c

[Wang Shaoyan]

Yes, I want to test, but I can't open CONFIG_GIANFAR, because I can't
find FSL_SOC, am i miss something?
> "got err" is true.
>
> Please compile test your patches before sending them.
>
>
>



-- 
Wang Shaoyan

Re: [PATCH] gianfar: reduce stack usage in gianfar_ethtool.c

[Joe Perches]

On Wed, 2011-08-10 at 11:50 +0800, Wang Shaoyan wrote:
> Yes, I want to test, but I can't open CONFIG_GIANFAR, because I can't
> find FSL_SOC, am i miss something?

A cross compiler for powerpc?

You can get one from here:
ftp://ftp.kernel.org/pub/tools/crosstool/files/bin/i686/

Pick an appropriate gcc for you.

Then read the process docs in Documentation/development-process
or google linux cross compiling.

Re: [PATCH] gianfar: reduce stack usage in gianfar_ethtool.c

[David Miller]

From: Joe Perches <joe@perches.com>
Date: Tue, 09 Aug 2011 21:07:17 -0700

> On Wed, 2011-08-10 at 11:50 +0800, Wang Shaoyan wrote:
>> Yes, I want to test, but I can't open CONFIG_GIANFAR, because I can't
>> find FSL_SOC, am i miss something?
> 
> A cross compiler for powerpc?

This is really misleading, because in the commit message the stack
usage message is reported but the patch submitted obviously didn't
generate that message since they can't even build test this change.

I'm not going to apply this until someone both build and functionally
tests this patch.

[PATCH] gianfar: reduce stack usage in gianfar_ethtool.c

[stufever]

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 |   26 +++++++++++++++++++++-----
 1 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/drivers/net/gianfar_ethtool.c b/drivers/net/gianfar_ethtool.c
index 6e35069..25a8c2a 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;
+		goto err;
+	}
 
 	switch (class) {
 	case TCP_V4_FLOW:
@@ -706,7 +717,8 @@ static int gfar_ethflow_to_filer_table(struct gfar_private *priv, u64 ethflow, u
 		break;
 	default:
 		pr_err("Right now this class is not supported\n");
-		return 0;
+		ret = 0;
+		goto err;
 	}
 
 	for (i = 0; i < MAX_FILER_IDX + 1; i++) {
@@ -721,7 +733,8 @@ static int gfar_ethflow_to_filer_table(struct gfar_private *priv, u64 ethflow, u
 
 	if (i == MAX_FILER_IDX + 1) {
 		pr_err("No parse rule found, can't create hash rules\n");
-		return 0;
+		ret = 0;
+		goto err;
 	}
 
 	/* If a match was found, then it begins the starting of a cluster rule
@@ -765,7 +778,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)
-- 
1.7.4.1

[PATCH] gianfar: reduce stack usage in gianfar_ethtool.c

[stufever]

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");
+		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)
-- 
1.7.0.4

Re: [PATCH] gianfar: reduce stack usage in gianfar_ethtool.c

[Eric Dumazet]

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.

Re: [PATCH] gianfar: reduce stack usage in gianfar_ethtool.c

[Joe Perches]

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?

Re: [PATCH] gianfar: reduce stack usage in gianfar_ethtool.c

[Eric Dumazet]

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);

Re: [PATCH] gianfar: reduce stack usage in gianfar_ethtool.c

[Joe Perches]

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.

Re: [PATCH] gianfar: reduce stack usage in gianfar_ethtool.c

[Eric Dumazet]

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, ...)

Re: [PATCH] gianfar: reduce stack usage in gianfar_ethtool.c

[Joe Perches]

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.

Re: [PATCH] gianfar: reduce stack usage in gianfar_ethtool.c

[Wang Shaoyan]

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.
>
>
>
>



-- 
Wang Shaoyan

[PATCH] gianfar: reduce stack usage in gianfar_ethtool.c

[stufever]

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 |   26 +++++++++++++++++++++++---
 1 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/net/gianfar_ethtool.c b/drivers/net/gianfar_ethtool.c
index 6e35069..9bca11c 100644
--- a/drivers/net/gianfar_ethtool.c
+++ b/drivers/net/gianfar_ethtool.c
@@ -686,10 +686,26 @@ 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);
+	if (!local_rqfpr) {
+		pr_err("Out of memory\n");
+		ret = 0;
+		got err;
+	}
+	local_rqfcr = kmalloc(sizeof(unsigned int) * (MAX_FILER_IDX + 1),
+		GFP_KERNEL);
+	if (!local_rqfcr) {
+		pr_err("Out of memory\n");
+		ret = 0;
+		goto err1;
+	}
 
 	switch (class) {
 	case TCP_V4_FLOW:
@@ -765,7 +781,11 @@ 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;
+	kfree(local_rqfcr);
+err1:
+	kfree(local_rqfpr);
+err:
+	return ret;
 }
 
 static int gfar_set_hash_opts(struct gfar_private *priv, struct ethtool_rxnfc *cmd)
-- 
1.7.0.4

Re: [PATCH] gianfar: reduce stack usage in gianfar_ethtool.c

[David Miller]

From: stufever@gmail.com
Date: Tue,  9 Aug 2011 23:45:11 +0800

> 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>

Would you be so kind as to actually acknowledge in some way the person
who found all the bugs in your first version of this patch? :-/

Re: [PATCH] gianfar: reduce stack usage in gianfar_ethtool.c

[Wang Shaoyan]

Thanks for remainding, I just want to cover up my careless asap:-)

2011/8/9 David Miller <davem@davemloft.net>:
> From: stufever@gmail.com
> Date: Tue,  9 Aug 2011 23:45:11 +0800
>
>> 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>
>
> Would you be so kind as to actually acknowledge in some way the person
> who found all the bugs in your first version of this patch? :-/
>
>



-- 
Wang Shaoyan

Re: [PATCH] gianfar: reduce stack usage in gianfar_ethtool.c

[Joe Perches]

On Tue, 2011-08-09 at 23:45 +0800, stufever@gmail.com wrote:
> 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
[]
> diff --git a/drivers/net/gianfar_ethtool.c b/drivers/net/gianfar_ethtool.c
[]
> @@ -686,10 +686,26 @@ 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);
> +	if (!local_rqfpr) {
> +		pr_err("Out of memory\n");
> +		ret = 0;
> +		got err;
> +	}
> +	local_rqfcr = kmalloc(sizeof(unsigned int) * (MAX_FILER_IDX + 1),
> +		GFP_KERNEL);
> +	if (!local_rqfcr) {
> +		pr_err("Out of memory\n");
> +		ret = 0;
> +		goto err1;
> +	}

Perhaps it'd be clearer to use:

	local_rqfpr = kmalloc(...)
	local_rqfcr = kmalloc(...)
	if (!local_rqfpr || !local_rqfcr) {
		pr_err(...)
		ret = -ENOMEM;
		goto err;
	}

[...]

err:
	kfree(local_rqfpr);
	kfree(local_rqfcr);
	return ret;

Is the "local_" prefix useful?
It seems like visual noise to me.

Re: [PATCH] gianfar: reduce stack usage in gianfar_ethtool.c

[Wang Shaoyan]

I thought kfree should not call, when kmalloc is failed. Now I think
is fine, thanks, it is much clearer

> Perhaps it'd be clearer to use:
>
>        local_rqfpr = kmalloc(...)
>        local_rqfcr = kmalloc(...)
>        if (!local_rqfpr || !local_rqfcr) {
>                pr_err(...)
>                ret = -ENOMEM;
>                goto err;
>        }
>
> [...]
>
> err:
>        kfree(local_rqfpr);
>        kfree(local_rqfcr);
>        return ret;
>
> Is the "local_" prefix useful?
> It seems like visual noise to me.

They are some temporary variable, we better to left them

>
>



-- 
Wang Shaoyan

[PATCH] gianfar: reduce stack usage in gianfar_ethtool.c

[stufever]

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 |   26 +++++++++++++++++++++++---
 1 files changed, 23 insertions(+), 3 deletions(-)

diff --git a/drivers/net/gianfar_ethtool.c b/drivers/net/gianfar_ethtool.c
index 6e35069..039e9c3 100644
--- a/drivers/net/gianfar_ethtool.c
+++ b/drivers/net/gianfar_ethtool.c
@@ -686,10 +686,26 @@ 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);
+	if (local_rqfpr) {
+		pr_err("Out of memory\n");
+		ret = 0;
+		got err;
+	}
+	local_rqfcr = kmalloc(sizeof(unsigned int) * (MAX_FILER_IDX + 1),
+		GFP_KERNEL);
+	if (local_rqfcr) {
+		pr_err("Out of memory\n");
+		ret = 0;
+		goto err1;
+	}
 
 	switch (class) {
 	case TCP_V4_FLOW:
@@ -765,7 +781,11 @@ 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;
+	kfree(local_rqfcr);
+err1:
+	kfree(local_rqfpr);
+err:
+	return ret;
 }
 
 static int gfar_set_hash_opts(struct gfar_private *priv, struct ethtool_rxnfc *cmd)
-- 
1.7.0.4

Re: [PATCH] gianfar: reduce stack usage in gianfar_ethtool.c

[Eric Dumazet]

Le mardi 09 août 2011 à 23:18 +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 |   26 +++++++++++++++++++++++---
>  1 files changed, 23 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/gianfar_ethtool.c b/drivers/net/gianfar_ethtool.c
> index 6e35069..039e9c3 100644
> --- a/drivers/net/gianfar_ethtool.c
> +++ b/drivers/net/gianfar_ethtool.c
> @@ -686,10 +686,26 @@ 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);
> +	if (local_rqfpr) {

	if (!local_rqfpr) {

> +		pr_err("Out of memory\n");
> +		ret = 0;
> +		got err;
> +	}
> +	local_rqfcr = kmalloc(sizeof(unsigned int) * (MAX_FILER_IDX + 1),
> +		GFP_KERNEL);
> +	if (local_rqfcr) {

same here : if (!local_rqfcr) ...

> +		pr_err("Out of memory\n");
> +		ret = 0;
> +		goto err1;
> +	}
>  
>  	switch (class) {
>  	case TCP_V4_FLOW:
> @@ -765,7 +781,11 @@ 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;
> +	kfree(local_rqfcr);
> +err1:
> +	kfree(local_rqfpr);
> +err:
> +	return ret;
>  }
>  
>  static int gfar_set_hash_opts(struct gfar_private *priv, struct ethtool_rxnfc *cmd)

Re: [PATCH] gianfar: reduce stack usage in gianfar_ethtool.c

[Wang Shaoyan]

Thanks, I'm sorry to my careless.

2011/8/9 Eric Dumazet <eric.dumazet@gmail.com>:
> Le mardi 09 août 2011 à 23:18 +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 |   26 +++++++++++++++++++++++---
>>  1 files changed, 23 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/net/gianfar_ethtool.c b/drivers/net/gianfar_ethtool.c
>> index 6e35069..039e9c3 100644
>> --- a/drivers/net/gianfar_ethtool.c
>> +++ b/drivers/net/gianfar_ethtool.c
>> @@ -686,10 +686,26 @@ 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);
>> +     if (local_rqfpr) {
>
>        if (!local_rqfpr) {
>
>> +             pr_err("Out of memory\n");
>> +             ret = 0;
>> +             got err;
>> +     }
>> +     local_rqfcr = kmalloc(sizeof(unsigned int) * (MAX_FILER_IDX + 1),
>> +             GFP_KERNEL);
>> +     if (local_rqfcr) {
>
> same here : if (!local_rqfcr) ...
>
>> +             pr_err("Out of memory\n");
>> +             ret = 0;
>> +             goto err1;
>> +     }
>>
>>       switch (class) {
>>       case TCP_V4_FLOW:
>> @@ -765,7 +781,11 @@ 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;
>> +     kfree(local_rqfcr);
>> +err1:
>> +     kfree(local_rqfpr);
>> +err:
>> +     return ret;
>>  }
>>
>>  static int gfar_set_hash_opts(struct gfar_private *priv, struct ethtool_rxnfc *cmd)
>
>
>



-- 
Wang Shaoyan