linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH][trivial] crypto: tcrypt - reduce stack size
@ 2009-02-25 13:48 Frank Seidel
  2009-02-25 14:06 ` Thiago Galesi
                   ` (3 more replies)
  0 siblings, 4 replies; 22+ messages in thread
From: Frank Seidel @ 2009-02-25 13:48 UTC (permalink / raw)
  To: linux kernel
  Cc: linux-crypto, Frank Seidel, Frank Seidel, akpm, Herbert Xu,
	David S. Miller, nhorman, lho, kaber, darrenrjenkins

From: Frank Seidel <frank@f-seidel.de>

Applying kernel janitors todos (printk calls need KERN_*
constants on linebeginnings, reduce stack footprint where
possible) to tcrypts test_hash_speed (where stacks
memory footprint was very high (on i386 1184 bytes to
292 now).

Signed-off-by: Frank Seidel <frank@f-seidel.de>
---
 crypto/tcrypt.c |   29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -396,26 +396,34 @@ static void test_hash_speed(const char *
 	struct scatterlist sg[TVMEMSIZE];
 	struct crypto_hash *tfm;
 	struct hash_desc desc;
-	char output[1024];
+	char *output;
+	size_t output_size = 1024;
 	int i;
 	int ret;
 
-	printk("\ntesting speed of %s\n", algo);
+	printk(KERN_INFO "\ntesting speed of %s\n", algo);
+
+	output = kmalloc(output_size * sizeof(*output), GFP_KERNEL);
+	if (!output) {
+		printk(KERN_ERR "tcrypt: failed to allocate outputbuffer\n");
+		return;
+	}
 
 	tfm = crypto_alloc_hash(algo, 0, CRYPTO_ALG_ASYNC);
 
 	if (IS_ERR(tfm)) {
-		printk("failed to load transform for %s: %ld\n", algo,
+		printk(KERN_ERR "failed to load transform for %s: %ld\n", algo,
 		       PTR_ERR(tfm));
+		kfree(output);
 		return;
 	}
 
 	desc.tfm = tfm;
 	desc.flags = 0;
 
-	if (crypto_hash_digestsize(tfm) > sizeof(output)) {
-		printk("digestsize(%u) > outputbuffer(%zu)\n",
-		       crypto_hash_digestsize(tfm), sizeof(output));
+	if (crypto_hash_digestsize(tfm) > output_size) {
+		printk(KERN_ERR "digestsize(%u) > outputbuffer(%zu)\n",
+		       crypto_hash_digestsize(tfm), output_size);
 		goto out;
 	}
 
@@ -427,12 +435,14 @@ static void test_hash_speed(const char *
 
 	for (i = 0; speed[i].blen != 0; i++) {
 		if (speed[i].blen > TVMEMSIZE * PAGE_SIZE) {
-			printk("template (%u) too big for tvmem (%lu)\n",
+			printk(KERN_ERR
+			       "template (%u) too big for tvmem (%lu)\n",
 			       speed[i].blen, TVMEMSIZE * PAGE_SIZE);
 			goto out;
 		}
 
-		printk("test%3u (%5u byte blocks,%5u bytes per update,%4u updates): ",
+		printk(KERN_INFO "test%3u "
+		       "(%5u byte blocks,%5u bytes per update,%4u updates): ",
 		       i, speed[i].blen, speed[i].plen, speed[i].blen / speed[i].plen);
 
 		if (sec)
@@ -443,13 +453,14 @@ static void test_hash_speed(const char *
 					       speed[i].plen, output);
 
 		if (ret) {
-			printk("hashing failed ret=%d\n", ret);
+			printk(KERN_ERR "hashing failed ret=%d\n", ret);
 			break;
 		}
 	}
 
 out:
 	crypto_free_hash(tfm);
+	kfree(output);
 }
 
 static void test_available(void)

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH][trivial] crypto: tcrypt - reduce stack size
  2009-02-25 13:48 [PATCH][trivial] crypto: tcrypt - reduce stack size Frank Seidel
@ 2009-02-25 14:06 ` Thiago Galesi
  2009-02-25 14:06 ` Neil Horman
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 22+ messages in thread
From: Thiago Galesi @ 2009-02-25 14:06 UTC (permalink / raw)
  To: Frank Seidel
  Cc: linux kernel, linux-crypto, Frank Seidel, akpm, Herbert Xu,
	David S. Miller, nhorman, lho, kaber, darrenrjenkins

>        int i;
>        int ret;
>
> -       printk("\ntesting speed of %s\n", algo);
> +       printk(KERN_INFO "\ntesting speed of %s\n", algo);

Wouldn't it be better to take the first \n out as well??


-- 
-
Thiago Galesi

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH][trivial] crypto: tcrypt - reduce stack size
  2009-02-25 13:48 [PATCH][trivial] crypto: tcrypt - reduce stack size Frank Seidel
  2009-02-25 14:06 ` Thiago Galesi
@ 2009-02-25 14:06 ` Neil Horman
  2009-02-25 14:20 ` Herbert Xu
  2009-02-25 14:24 ` [PATCHv2][trivial] " Frank Seidel
  3 siblings, 0 replies; 22+ messages in thread
From: Neil Horman @ 2009-02-25 14:06 UTC (permalink / raw)
  To: Frank Seidel
  Cc: linux kernel, linux-crypto, Frank Seidel, akpm, Herbert Xu,
	David S. Miller, lho, kaber, darrenrjenkins

On Wed, Feb 25, 2009 at 02:48:19PM +0100, Frank Seidel wrote:
> From: Frank Seidel <frank@f-seidel.de>
> 
> Applying kernel janitors todos (printk calls need KERN_*
> constants on linebeginnings, reduce stack footprint where
> possible) to tcrypts test_hash_speed (where stacks
> memory footprint was very high (on i386 1184 bytes to
> 292 now).
> 
> Signed-off-by: Frank Seidel <frank@f-seidel.de>
> ---
>  
>  	if (IS_ERR(tfm)) {
> -		printk("failed to load transform for %s: %ld\n", algo,
> +		printk(KERN_ERR "failed to load transform for %s: %ld\n", algo,
>  		       PTR_ERR(tfm));
> +		kfree(output);
>  		return;
Its just a stylistic nit, but I think I'd rather see this be a goto to the kfree
at the end of the function.  You can move the out label down to just the kfree,
and rename the current out usage to out_free_tfm.  I think if you're going to
add the kfree at the bottom, you might as well make use of it for all cases.

If you change that, I'll ack this.

Thanks & Regards
Neil

>  	}
>  
>  	desc.tfm = tfm;
>  	desc.flags = 0;
>  
> -	if (crypto_hash_digestsize(tfm) > sizeof(output)) {
> -		printk("digestsize(%u) > outputbuffer(%zu)\n",
> -		       crypto_hash_digestsize(tfm), sizeof(output));
> +	if (crypto_hash_digestsize(tfm) > output_size) {
> +		printk(KERN_ERR "digestsize(%u) > outputbuffer(%zu)\n",
> +		       crypto_hash_digestsize(tfm), output_size);
>  		goto out;
>  	}
>  
> @@ -427,12 +435,14 @@ static void test_hash_speed(const char *
>  
>  	for (i = 0; speed[i].blen != 0; i++) {
>  		if (speed[i].blen > TVMEMSIZE * PAGE_SIZE) {
> -			printk("template (%u) too big for tvmem (%lu)\n",
> +			printk(KERN_ERR
> +			       "template (%u) too big for tvmem (%lu)\n",
>  			       speed[i].blen, TVMEMSIZE * PAGE_SIZE);
>  			goto out;
>  		}
>  
> -		printk("test%3u (%5u byte blocks,%5u bytes per update,%4u updates): ",
> +		printk(KERN_INFO "test%3u "
> +		       "(%5u byte blocks,%5u bytes per update,%4u updates): ",
>  		       i, speed[i].blen, speed[i].plen, speed[i].blen / speed[i].plen);
>  
>  		if (sec)
> @@ -443,13 +453,14 @@ static void test_hash_speed(const char *
>  					       speed[i].plen, output);
>  
>  		if (ret) {
> -			printk("hashing failed ret=%d\n", ret);
> +			printk(KERN_ERR "hashing failed ret=%d\n", ret);
>  			break;
>  		}
>  	}
>  
>  out:
>  	crypto_free_hash(tfm);
> +	kfree(output);
>  }
>  
>  static void test_available(void)
> 

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH][trivial] crypto: tcrypt - reduce stack size
  2009-02-25 13:48 [PATCH][trivial] crypto: tcrypt - reduce stack size Frank Seidel
  2009-02-25 14:06 ` Thiago Galesi
  2009-02-25 14:06 ` Neil Horman
@ 2009-02-25 14:20 ` Herbert Xu
  2009-02-25 14:24   ` Frank Seidel
  2009-02-25 14:24 ` [PATCHv2][trivial] " Frank Seidel
  3 siblings, 1 reply; 22+ messages in thread
From: Herbert Xu @ 2009-02-25 14:20 UTC (permalink / raw)
  To: Frank Seidel
  Cc: linux kernel, linux-crypto, Frank Seidel, akpm, David S. Miller,
	nhorman, lho, kaber, darrenrjenkins

On Wed, Feb 25, 2009 at 02:48:19PM +0100, Frank Seidel wrote:
> From: Frank Seidel <frank@f-seidel.de>
> 
> Applying kernel janitors todos (printk calls need KERN_*
> constants on linebeginnings, reduce stack footprint where
> possible) to tcrypts test_hash_speed (where stacks
> memory footprint was very high (on i386 1184 bytes to
> 292 now).
> 
> Signed-off-by: Frank Seidel <frank@f-seidel.de>
> ---
>  crypto/tcrypt.c |   29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> --- a/crypto/tcrypt.c
> +++ b/crypto/tcrypt.c
> @@ -396,26 +396,34 @@ static void test_hash_speed(const char *
>  	struct scatterlist sg[TVMEMSIZE];
>  	struct crypto_hash *tfm;
>  	struct hash_desc desc;
> -	char output[1024];

How about just making it static?

Thanks,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH][trivial] crypto: tcrypt - reduce stack size
  2009-02-25 14:20 ` Herbert Xu
@ 2009-02-25 14:24   ` Frank Seidel
  2009-02-25 14:27     ` Herbert Xu
  2009-02-25 14:29     ` Thiago Galesi
  0 siblings, 2 replies; 22+ messages in thread
From: Frank Seidel @ 2009-02-25 14:24 UTC (permalink / raw)
  To: Herbert Xu
  Cc: linux kernel, linux-crypto, Frank Seidel, akpm, David S. Miller,
	nhorman, lho, kaber, darrenrjenkins

Herbert Xu wrote:
>> -	char output[1024];
> 
> How about just making it static?

Well, it was static before. But there
are efforts to reduce current stack memory
footprints.

Frank


^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCHv2][trivial] crypto: tcrypt - reduce stack size
  2009-02-25 13:48 [PATCH][trivial] crypto: tcrypt - reduce stack size Frank Seidel
                   ` (2 preceding siblings ...)
  2009-02-25 14:20 ` Herbert Xu
@ 2009-02-25 14:24 ` Frank Seidel
  2009-02-25 14:53   ` [PATCHv3][trivial] " Frank Seidel
  3 siblings, 1 reply; 22+ messages in thread
From: Frank Seidel @ 2009-02-25 14:24 UTC (permalink / raw)
  To: Frank Seidel
  Cc: linux kernel, linux-crypto, akpm, Herbert Xu, David S. Miller,
	nhorman, lho, kaber, darrenrjenkins, Frank Seidel

From: Frank Seidel <frank@f-seidel.de>

Applying kernel janitors todos (printk calls need KERN_*
constants on linebeginnings, reduce stack footprint where
possible) to tcrypts test_hash_speed (where stacks
memory footprint was very high (on i386 1184 bytes to
164 now).

Signed-off-by: Frank Seidel <frank@f-seidel.de>
---
 crypto/tcrypt.c |   37 ++++++++++++++++++++++++-------------
 1 file changed, 24 insertions(+), 13 deletions(-)

--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -396,27 +396,34 @@ static void test_hash_speed(const char *
 	struct scatterlist sg[TVMEMSIZE];
 	struct crypto_hash *tfm;
 	struct hash_desc desc;
-	char output[1024];
+	char *output;
+	size_t output_size = 1024;
 	int i;
 	int ret;
 
-	printk("\ntesting speed of %s\n", algo);
+	printk(KERN_INFO "\ntesting speed of %s\n", algo);
+
+	output = kmalloc(output_size * sizeof(*output), GFP_KERNEL);
+	if (!output) {
+		printk(KERN_ERR "tcrypt: failed to allocate outputbuffer\n");
+		return;
+	}
 
 	tfm = crypto_alloc_hash(algo, 0, CRYPTO_ALG_ASYNC);
 
 	if (IS_ERR(tfm)) {
-		printk("failed to load transform for %s: %ld\n", algo,
+		printk(KERN_ERR "failed to load transform for %s: %ld\n", algo,
 		       PTR_ERR(tfm));
-		return;
+		goto out;
 	}
 
 	desc.tfm = tfm;
 	desc.flags = 0;
 
-	if (crypto_hash_digestsize(tfm) > sizeof(output)) {
-		printk("digestsize(%u) > outputbuffer(%zu)\n",
-		       crypto_hash_digestsize(tfm), sizeof(output));
-		goto out;
+	if (crypto_hash_digestsize(tfm) > output_size) {
+		printk(KERN_ERR "digestsize(%u) > outputbuffer(%zu)\n",
+		       crypto_hash_digestsize(tfm), output_size);
+		goto out_free_tfm;
 	}
 
 	sg_init_table(sg, TVMEMSIZE);
@@ -427,12 +434,14 @@ static void test_hash_speed(const char *
 
 	for (i = 0; speed[i].blen != 0; i++) {
 		if (speed[i].blen > TVMEMSIZE * PAGE_SIZE) {
-			printk("template (%u) too big for tvmem (%lu)\n",
+			printk(KERN_ERR
+			       "template (%u) too big for tvmem (%lu)\n",
 			       speed[i].blen, TVMEMSIZE * PAGE_SIZE);
-			goto out;
+			goto out_free_tfm;
 		}
 
-		printk("test%3u (%5u byte blocks,%5u bytes per update,%4u updates): ",
+		printk(KERN_INFO "test%3u "
+		       "(%5u byte blocks,%5u bytes per update,%4u updates): ",
 		       i, speed[i].blen, speed[i].plen, speed[i].blen / speed[i].plen);
 
 		if (sec)
@@ -443,13 +452,15 @@ static void test_hash_speed(const char *
 					       speed[i].plen, output);
 
 		if (ret) {
-			printk("hashing failed ret=%d\n", ret);
+			printk(KERN_ERR "hashing failed ret=%d\n", ret);
 			break;
 		}
 	}
 
-out:
+out_free_tfm:
 	crypto_free_hash(tfm);
+out:
+	kfree(output);
 }
 
 static void test_available(void)


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH][trivial] crypto: tcrypt - reduce stack size
  2009-02-25 14:24   ` Frank Seidel
@ 2009-02-25 14:27     ` Herbert Xu
  2009-02-25 14:31       ` Geert Uytterhoeven
  2009-02-25 14:29     ` Thiago Galesi
  1 sibling, 1 reply; 22+ messages in thread
From: Herbert Xu @ 2009-02-25 14:27 UTC (permalink / raw)
  To: Frank Seidel
  Cc: linux kernel, linux-crypto, Frank Seidel, akpm, David S. Miller,
	nhorman, lho, kaber, darrenrjenkins

On Wed, Feb 25, 2009 at 03:24:25PM +0100, Frank Seidel wrote:
> Herbert Xu wrote:
> >> -	char output[1024];
> > 
> > How about just making it static?
> 
> Well, it was static before. But there
> are efforts to reduce current stack memory
> footprints.

Well if it's static it won't be using any stack memory.

Cheers,
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH][trivial] crypto: tcrypt - reduce stack size
  2009-02-25 14:24   ` Frank Seidel
  2009-02-25 14:27     ` Herbert Xu
@ 2009-02-25 14:29     ` Thiago Galesi
  2009-02-25 14:34       ` Frank Seidel
  1 sibling, 1 reply; 22+ messages in thread
From: Thiago Galesi @ 2009-02-25 14:29 UTC (permalink / raw)
  To: Frank Seidel
  Cc: Herbert Xu, linux kernel, linux-crypto, Frank Seidel, akpm,
	David S. Miller, nhorman, lho, kaber, darrenrjenkins

On Wed, Feb 25, 2009 at 11:24 AM, Frank Seidel <fseidel@suse.de> wrote:
> Herbert Xu wrote:
>>> -    char output[1024];
>>
>> How about just making it static?
>
> Well, it was static before. But there
> are efforts to reduce current stack memory
> footprints.

If you write static char output[1024]; (even inside a function) it's
not allocated on the stack.

-- 
-
Thiago Galesi

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH][trivial] crypto: tcrypt - reduce stack size
  2009-02-25 14:27     ` Herbert Xu
@ 2009-02-25 14:31       ` Geert Uytterhoeven
  2009-02-25 14:36         ` Frank Seidel
  0 siblings, 1 reply; 22+ messages in thread
From: Geert Uytterhoeven @ 2009-02-25 14:31 UTC (permalink / raw)
  To: Herbert Xu
  Cc: Frank Seidel, linux kernel, linux-crypto, Frank Seidel, akpm,
	David S. Miller, nhorman, lho, kaber, darrenrjenkins

On Wed, 25 Feb 2009, Herbert Xu wrote:
> On Wed, Feb 25, 2009 at 03:24:25PM +0100, Frank Seidel wrote:
> > Herbert Xu wrote:
> > >> -	char output[1024];
> > > 
> > > How about just making it static?
> > 
> > Well, it was static before. But there
> > are efforts to reduce current stack memory
> > footprints.
> 
> Well if it's static it won't be using any stack memory.

Probably people should be reminded tcrypt can only be a module, not built-in,
so it won't waste precious unswappable kernel memory if it's static.

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH][trivial] crypto: tcrypt - reduce stack size
  2009-02-25 14:29     ` Thiago Galesi
@ 2009-02-25 14:34       ` Frank Seidel
  2009-02-25 19:40         ` Greg KH
  0 siblings, 1 reply; 22+ messages in thread
From: Frank Seidel @ 2009-02-25 14:34 UTC (permalink / raw)
  To: Thiago Galesi
  Cc: Herbert Xu, linux kernel, linux-crypto, Frank Seidel, akpm,
	David S. Miller, nhorman, lho, kaber, darrenrjenkins, Greg KH

Thiago Galesi wrote:
> If you write static char output[1024]; (even inside a function) it's
> not allocated on the stack.

Oh, yes i misunderstood Herbert, sorry. But anyway isn't
it preferred to kmalloc such arrays?
Greg, i thought it was you who told me so, is that right?

Thanks,
Frank


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH][trivial] crypto: tcrypt - reduce stack size
  2009-02-25 14:31       ` Geert Uytterhoeven
@ 2009-02-25 14:36         ` Frank Seidel
  2009-02-25 14:44           ` Geert Uytterhoeven
  0 siblings, 1 reply; 22+ messages in thread
From: Frank Seidel @ 2009-02-25 14:36 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Herbert Xu, linux kernel, linux-crypto, Frank Seidel, akpm,
	David S. Miller, nhorman, lho, kaber, darrenrjenkins, Greg KH

Geert Uytterhoeven wrote:
> Probably people should be reminded tcrypt can only be a module, not built-in,
> so it won't waste precious unswappable kernel memory if it's static.

Thats right, but it also doesn't hurt to simply use kmalloc, does it? :-)
I just stumbled over tcrypt on the make checkstack output and as also
the kerneljanitors todo advises to reduce this footprint where possible
i just wanted to help out here.

Thanks,
Frank

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH][trivial] crypto: tcrypt - reduce stack size
  2009-02-25 14:36         ` Frank Seidel
@ 2009-02-25 14:44           ` Geert Uytterhoeven
  2009-02-25 14:54             ` Frank Seidel
  0 siblings, 1 reply; 22+ messages in thread
From: Geert Uytterhoeven @ 2009-02-25 14:44 UTC (permalink / raw)
  To: Frank Seidel
  Cc: Herbert Xu, linux kernel, linux-crypto, Frank Seidel, akpm,
	David S. Miller, nhorman, lho, kaber, darrenrjenkins, Greg KH

On Wed, 25 Feb 2009, Frank Seidel wrote:
> Geert Uytterhoeven wrote:
> > Probably people should be reminded tcrypt can only be a module, not built-in,
> > so it won't waste precious unswappable kernel memory if it's static.
> 
> Thats right, but it also doesn't hurt to simply use kmalloc, does it? :-)

Wel...

Using kmalloc() increases code size, makes the code more complex, and increases
the risk of introducing a memory leak now or later.

> I just stumbled over tcrypt on the make checkstack output and as also
> the kerneljanitors todo advises to reduce this footprint where possible
> i just wanted to help out here.

Reducing stack usage is fine. However, for a loadable test module without
concurrency issues it's far easier to do that by just making the data static.

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010

^ permalink raw reply	[flat|nested] 22+ messages in thread

* [PATCHv3][trivial] crypto: tcrypt - reduce stack size
  2009-02-25 14:24 ` [PATCHv2][trivial] " Frank Seidel
@ 2009-02-25 14:53   ` Frank Seidel
  2009-02-25 15:32     ` Neil Horman
  2009-02-25 15:54     ` Geert Uytterhoeven
  0 siblings, 2 replies; 22+ messages in thread
From: Frank Seidel @ 2009-02-25 14:53 UTC (permalink / raw)
  To: Frank Seidel
  Cc: linux kernel, linux-crypto, akpm, Herbert Xu, David S. Miller,
	nhorman, lho, kaber, darrenrjenkins, Frank Seidel

From: Frank Seidel <frank@f-seidel.de>

Applying kernel janitors todos (printk calls need KERN_*
constants on linebeginnings, reduce stack footprint where
possible) to tcrypts test_hash_speed (where stacks
memory footprint was very high (on i386 1184 bytes to
160 now).

Signed-off-by: Frank Seidel <frank@f-seidel.de>
---
 crypto/tcrypt.c |   16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

--- a/crypto/tcrypt.c
+++ b/crypto/tcrypt.c
@@ -396,16 +396,16 @@ static void test_hash_speed(const char *
 	struct scatterlist sg[TVMEMSIZE];
 	struct crypto_hash *tfm;
 	struct hash_desc desc;
-	char output[1024];
+	static char output[1024];
 	int i;
 	int ret;
 
-	printk("\ntesting speed of %s\n", algo);
+	printk(KERN_INFO "\ntesting speed of %s\n", algo);
 
 	tfm = crypto_alloc_hash(algo, 0, CRYPTO_ALG_ASYNC);
 
 	if (IS_ERR(tfm)) {
-		printk("failed to load transform for %s: %ld\n", algo,
+		printk(KERN_ERR "failed to load transform for %s: %ld\n", algo,
 		       PTR_ERR(tfm));
 		return;
 	}
@@ -414,7 +414,7 @@ static void test_hash_speed(const char *
 	desc.flags = 0;
 
 	if (crypto_hash_digestsize(tfm) > sizeof(output)) {
-		printk("digestsize(%u) > outputbuffer(%zu)\n",
+		printk(KERN_ERR "digestsize(%u) > outputbuffer(%zu)\n",
 		       crypto_hash_digestsize(tfm), sizeof(output));
 		goto out;
 	}
@@ -427,12 +427,14 @@ static void test_hash_speed(const char *
 
 	for (i = 0; speed[i].blen != 0; i++) {
 		if (speed[i].blen > TVMEMSIZE * PAGE_SIZE) {
-			printk("template (%u) too big for tvmem (%lu)\n",
+			printk(KERN_ERR
+			       "template (%u) too big for tvmem (%lu)\n",
 			       speed[i].blen, TVMEMSIZE * PAGE_SIZE);
 			goto out;
 		}
 
-		printk("test%3u (%5u byte blocks,%5u bytes per update,%4u updates): ",
+		printk(KERN_INFO "test%3u "
+		       "(%5u byte blocks,%5u bytes per update,%4u updates): ",
 		       i, speed[i].blen, speed[i].plen, speed[i].blen / speed[i].plen);
 
 		if (sec)
@@ -443,7 +445,7 @@ static void test_hash_speed(const char *
 					       speed[i].plen, output);
 
 		if (ret) {
-			printk("hashing failed ret=%d\n", ret);
+			printk(KERN_ERR "hashing failed ret=%d\n", ret);
 			break;
 		}
 	}

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH][trivial] crypto: tcrypt - reduce stack size
  2009-02-25 14:44           ` Geert Uytterhoeven
@ 2009-02-25 14:54             ` Frank Seidel
  0 siblings, 0 replies; 22+ messages in thread
From: Frank Seidel @ 2009-02-25 14:54 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Frank Seidel, Herbert Xu, linux kernel, linux-crypto, akpm,
	David S. Miller, nhorman, lho, kaber, darrenrjenkins, Greg KH,
	Frank Seidel

Geert Uytterhoeven wrote:
> On Wed, 25 Feb 2009, Frank Seidel wrote:
> Wel...
> 
> Using kmalloc() increases code size, makes the code more complex, and increases
> the risk of introducing a memory leak now or later.

Ok, admitted.

>> I just stumbled over tcrypt on the make checkstack output and as also
>> the kerneljanitors todo advises to reduce this footprint where possible
>> i just wanted to help out here.
> 
> Reducing stack usage is fine. However, for a loadable test module without
> concurrency issues it's far easier to do that by just making the data static.

Is PATCHv3 then ok for you?

Thanks,
Frank

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCHv3][trivial] crypto: tcrypt - reduce stack size
  2009-02-25 14:53   ` [PATCHv3][trivial] " Frank Seidel
@ 2009-02-25 15:32     ` Neil Horman
  2009-03-29  7:19       ` Herbert Xu
  2009-02-25 15:54     ` Geert Uytterhoeven
  1 sibling, 1 reply; 22+ messages in thread
From: Neil Horman @ 2009-02-25 15:32 UTC (permalink / raw)
  To: Frank Seidel
  Cc: linux kernel, linux-crypto, akpm, Herbert Xu, David S. Miller,
	lho, kaber, darrenrjenkins, Frank Seidel

On Wed, Feb 25, 2009 at 03:53:14PM +0100, Frank Seidel wrote:
> From: Frank Seidel <frank@f-seidel.de>
> 
> Applying kernel janitors todos (printk calls need KERN_*
> constants on linebeginnings, reduce stack footprint where
> possible) to tcrypts test_hash_speed (where stacks
> memory footprint was very high (on i386 1184 bytes to
> 160 now).
> 
> Signed-off-by: Frank Seidel <frank@f-seidel.de>
Looks good, thanks Frank
Acked-by: Neil Horman <nhorman@tuxdriver.com>


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCHv3][trivial] crypto: tcrypt - reduce stack size
  2009-02-25 14:53   ` [PATCHv3][trivial] " Frank Seidel
  2009-02-25 15:32     ` Neil Horman
@ 2009-02-25 15:54     ` Geert Uytterhoeven
  2009-02-25 15:56       ` Frank Seidel
  1 sibling, 1 reply; 22+ messages in thread
From: Geert Uytterhoeven @ 2009-02-25 15:54 UTC (permalink / raw)
  To: Frank Seidel
  Cc: linux kernel, linux-crypto, akpm, Herbert Xu, David S. Miller,
	nhorman, lho, kaber, darrenrjenkins, Frank Seidel

On Wed, 25 Feb 2009, Frank Seidel wrote:
> -	printk("\ntesting speed of %s\n", algo);
> +	printk(KERN_INFO "\ntesting speed of %s\n", algo);
                          ^^
			  woops

BTW, why are you using printk(KERN_INFO "..."), and not pr_info("...")?

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCHv3][trivial] crypto: tcrypt - reduce stack size
  2009-02-25 15:54     ` Geert Uytterhoeven
@ 2009-02-25 15:56       ` Frank Seidel
  2009-02-25 15:58         ` Geert Uytterhoeven
  0 siblings, 1 reply; 22+ messages in thread
From: Frank Seidel @ 2009-02-25 15:56 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Frank Seidel, linux kernel, linux-crypto, akpm, Herbert Xu,
	David S. Miller, nhorman, lho, kaber, darrenrjenkins

Geert Uytterhoeven wrote:
> BTW, why are you using printk(KERN_INFO "..."), and not pr_info("...")?

Without special reason. Is pr_info preferred?

Thanks,
Frank

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCHv3][trivial] crypto: tcrypt - reduce stack size
  2009-02-25 15:56       ` Frank Seidel
@ 2009-02-25 15:58         ` Geert Uytterhoeven
  2009-02-25 16:01           ` Frank Seidel
  2009-02-25 16:08           ` Geert Uytterhoeven
  0 siblings, 2 replies; 22+ messages in thread
From: Geert Uytterhoeven @ 2009-02-25 15:58 UTC (permalink / raw)
  To: Frank Seidel
  Cc: Frank Seidel, linux kernel, linux-crypto, akpm, Herbert Xu,
	David S. Miller, nhorman, lho, kaber, darrenrjenkins

On Wed, 25 Feb 2009, Frank Seidel wrote:
> Geert Uytterhoeven wrote:
> > BTW, why are you using printk(KERN_INFO "..."), and not pr_info("...")?
> 
> Without special reason. Is pr_info preferred?

It's shorter.

Especially `pr_err' fits in the space of `printk', so you don't have to split
lines that become to long due to adding the KERN_*.

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCHv3][trivial] crypto: tcrypt - reduce stack size
  2009-02-25 15:58         ` Geert Uytterhoeven
@ 2009-02-25 16:01           ` Frank Seidel
  2009-02-25 16:08           ` Geert Uytterhoeven
  1 sibling, 0 replies; 22+ messages in thread
From: Frank Seidel @ 2009-02-25 16:01 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Frank Seidel, linux kernel, linux-crypto, akpm, Herbert Xu,
	David S. Miller, nhorman, lho, kaber, darrenrjenkins

Geert Uytterhoeven wrote:
> It's shorter.
> 
> Especially `pr_err' fits in the space of `printk', so you don't have to split
> lines that become to long due to adding the KERN_*.

Ah, i see. Thanks for the explanation :-)

Frank


^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCHv3][trivial] crypto: tcrypt - reduce stack size
  2009-02-25 15:58         ` Geert Uytterhoeven
  2009-02-25 16:01           ` Frank Seidel
@ 2009-02-25 16:08           ` Geert Uytterhoeven
  1 sibling, 0 replies; 22+ messages in thread
From: Geert Uytterhoeven @ 2009-02-25 16:08 UTC (permalink / raw)
  To: Frank Seidel
  Cc: Frank Seidel, linux kernel, linux-crypto, akpm, Herbert Xu,
	David S. Miller, nhorman, lho, kaber, darrenrjenkins

On Wed, 25 Feb 2009, Geert Uytterhoeven wrote:
> On Wed, 25 Feb 2009, Frank Seidel wrote:
> > Geert Uytterhoeven wrote:
> > > BTW, why are you using printk(KERN_INFO "..."), and not pr_info("...")?
> > 
> > Without special reason. Is pr_info preferred?
> 
> It's shorter.
> 
> Especially `pr_err' fits in the space of `printk', so you don't have to split
> lines that become to long due to adding the KERN_*.

I sent it out too soon...

Other advantages:
  - The pr_*() calls use pr_fmt(), so you can set default prefixes (like
    __FILE__, __function, __LINE) at one place at the top of your driver source
    file.
  - pr_debug() is automatically left out if DEBUG is not defined (unless
    CONFIG_DYNAMIC_PRINTK_DEBUG is set).

With kind regards,

Geert Uytterhoeven
Software Architect

Sony Techsoft Centre Europe
The Corporate Village · Da Vincilaan 7-D1 · B-1935 Zaventem · Belgium

Phone:    +32 (0)2 700 8453
Fax:      +32 (0)2 700 8622
E-mail:   Geert.Uytterhoeven@sonycom.com
Internet: http://www.sony-europe.com/

A division of Sony Europe (Belgium) N.V.
VAT BE 0413.825.160 · RPR Brussels
Fortis · BIC GEBABEBB · IBAN BE41293037680010

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCH][trivial] crypto: tcrypt - reduce stack size
  2009-02-25 14:34       ` Frank Seidel
@ 2009-02-25 19:40         ` Greg KH
  0 siblings, 0 replies; 22+ messages in thread
From: Greg KH @ 2009-02-25 19:40 UTC (permalink / raw)
  To: Frank Seidel
  Cc: Thiago Galesi, Herbert Xu, linux kernel, linux-crypto,
	Frank Seidel, akpm, David S. Miller, nhorman, lho, kaber,
	darrenrjenkins

On Wed, Feb 25, 2009 at 03:34:11PM +0100, Frank Seidel wrote:
> Thiago Galesi wrote:
> > If you write static char output[1024]; (even inside a function) it's
> > not allocated on the stack.
> 
> Oh, yes i misunderstood Herbert, sorry. But anyway isn't
> it preferred to kmalloc such arrays?
> Greg, i thought it was you who told me so, is that right?

Generally, yes, it is preferred.  But if it can be static, that's fine
too.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 22+ messages in thread

* Re: [PATCHv3][trivial] crypto: tcrypt - reduce stack size
  2009-02-25 15:32     ` Neil Horman
@ 2009-03-29  7:19       ` Herbert Xu
  0 siblings, 0 replies; 22+ messages in thread
From: Herbert Xu @ 2009-03-29  7:19 UTC (permalink / raw)
  To: Neil Horman
  Cc: Frank Seidel, linux kernel, linux-crypto, akpm, David S. Miller,
	lho, kaber, darrenrjenkins, Frank Seidel

On Wed, Feb 25, 2009 at 10:32:31AM -0500, Neil Horman wrote:
> On Wed, Feb 25, 2009 at 03:53:14PM +0100, Frank Seidel wrote:
> > From: Frank Seidel <frank@f-seidel.de>
> > 
> > Applying kernel janitors todos (printk calls need KERN_*
> > constants on linebeginnings, reduce stack footprint where
> > possible) to tcrypts test_hash_speed (where stacks
> > memory footprint was very high (on i386 1184 bytes to
> > 160 now).
> > 
> > Signed-off-by: Frank Seidel <frank@f-seidel.de>
> Looks good, thanks Frank
> Acked-by: Neil Horman <nhorman@tuxdriver.com>

Patch applied to cryptodev.  Thanks!
-- 
Visit Openswan at http://www.openswan.org/
Email: Herbert Xu ~{PmV>HI~} <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 22+ messages in thread

end of thread, other threads:[~2009-03-29  7:20 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-02-25 13:48 [PATCH][trivial] crypto: tcrypt - reduce stack size Frank Seidel
2009-02-25 14:06 ` Thiago Galesi
2009-02-25 14:06 ` Neil Horman
2009-02-25 14:20 ` Herbert Xu
2009-02-25 14:24   ` Frank Seidel
2009-02-25 14:27     ` Herbert Xu
2009-02-25 14:31       ` Geert Uytterhoeven
2009-02-25 14:36         ` Frank Seidel
2009-02-25 14:44           ` Geert Uytterhoeven
2009-02-25 14:54             ` Frank Seidel
2009-02-25 14:29     ` Thiago Galesi
2009-02-25 14:34       ` Frank Seidel
2009-02-25 19:40         ` Greg KH
2009-02-25 14:24 ` [PATCHv2][trivial] " Frank Seidel
2009-02-25 14:53   ` [PATCHv3][trivial] " Frank Seidel
2009-02-25 15:32     ` Neil Horman
2009-03-29  7:19       ` Herbert Xu
2009-02-25 15:54     ` Geert Uytterhoeven
2009-02-25 15:56       ` Frank Seidel
2009-02-25 15:58         ` Geert Uytterhoeven
2009-02-25 16:01           ` Frank Seidel
2009-02-25 16:08           ` Geert Uytterhoeven

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).