diff mbox series

[2/2] dm integrity: use init_completion instead of COMPLETION_INITIALIZER_ONSTACK

Message ID 20170815151242.2637861-2-arnd@arndb.de
State Accepted
Commit b5e8ad92c3ac0b073bbf08ffd1a6a31d3449caae
Headers show
Series [1/2] mmc: test: reduce stack usage in mmc_test_nonblock_transfer | expand

Commit Message

Arnd Bergmann Aug. 15, 2017, 3:11 p.m. UTC
The new lockdep support for completions causeed the stack usage
in dm-integrity to explode, in case of write_journal from 504 bytes
to 1120 (using arm gcc-7.1.1):

drivers/md/dm-integrity.c: In function 'write_journal':
drivers/md/dm-integrity.c:827:1: error: the frame size of 1120 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]

The problem is that not only the size of 'struct completion' grows
significantly, but we end up having multiple copies of it on the stack
when we assign it from a local variable after the initial declaration.

COMPLETION_INITIALIZER_ONSTACK() is the right thing to use when we
want to declare and initialize a completion on the stack. However,
this driver doesn't do that and instead initializes the completion
just before it is used.

In this case, init_completion() does the same thing more efficiently,
and drops the stack usage for the function above down to 496 bytes.
While the other functions in this file are not bad enough to cause
a warning, they benefit equally from the change, so I do the change
across the entire file. In the one place where we reuse a completion,
I picked the cheaper reinit_completion() over init_completion().

Fixes: cd8084f91c02 ("locking/lockdep: Apply crossrelease to completions")
Signed-off-by: Arnd Bergmann <arnd@arndb.de>

---
The patch causing this is currently part of linux-next, scheduled for
4.14, so it would be good to have this in the same release.
---
 drivers/md/dm-integrity.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

-- 
2.9.0

Comments

Mikulas Patocka Aug. 16, 2017, 4:12 p.m. UTC | #1
This patch is OK.

Acked-by: Mikulas Patocka <mpatocka@redhat.com>


On Tue, 15 Aug 2017, Arnd Bergmann wrote:

> The new lockdep support for completions causeed the stack usage

> in dm-integrity to explode, in case of write_journal from 504 bytes

> to 1120 (using arm gcc-7.1.1):

> 

> drivers/md/dm-integrity.c: In function 'write_journal':

> drivers/md/dm-integrity.c:827:1: error: the frame size of 1120 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]

> 

> The problem is that not only the size of 'struct completion' grows

> significantly, but we end up having multiple copies of it on the stack

> when we assign it from a local variable after the initial declaration.

> 

> COMPLETION_INITIALIZER_ONSTACK() is the right thing to use when we

> want to declare and initialize a completion on the stack. However,

> this driver doesn't do that and instead initializes the completion

> just before it is used.

> 

> In this case, init_completion() does the same thing more efficiently,

> and drops the stack usage for the function above down to 496 bytes.

> While the other functions in this file are not bad enough to cause

> a warning, they benefit equally from the change, so I do the change

> across the entire file. In the one place where we reuse a completion,

> I picked the cheaper reinit_completion() over init_completion().

> 

> Fixes: cd8084f91c02 ("locking/lockdep: Apply crossrelease to completions")

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

> ---

> The patch causing this is currently part of linux-next, scheduled for

> 4.14, so it would be good to have this in the same release.

> ---

>  drivers/md/dm-integrity.c | 20 ++++++++++----------

>  1 file changed, 10 insertions(+), 10 deletions(-)

> 

> diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c

> index 293a19652d55..b16010bcbd17 100644

> --- a/drivers/md/dm-integrity.c

> +++ b/drivers/md/dm-integrity.c

> @@ -773,13 +773,13 @@ static void write_journal(struct dm_integrity_c *ic, unsigned commit_start, unsi

>  	unsigned i;

>  

>  	io_comp.ic = ic;

> -	io_comp.comp = COMPLETION_INITIALIZER_ONSTACK(io_comp.comp);

> +	init_completion(&io_comp.comp);

>  

>  	if (commit_start + commit_sections <= ic->journal_sections) {

>  		io_comp.in_flight = (atomic_t)ATOMIC_INIT(1);

>  		if (ic->journal_io) {

>  			crypt_comp_1.ic = ic;

> -			crypt_comp_1.comp = COMPLETION_INITIALIZER_ONSTACK(crypt_comp_1.comp);

> +			init_completion(&crypt_comp_1.comp);

>  			crypt_comp_1.in_flight = (atomic_t)ATOMIC_INIT(0);

>  			encrypt_journal(ic, true, commit_start, commit_sections, &crypt_comp_1);

>  			wait_for_completion_io(&crypt_comp_1.comp);

> @@ -795,18 +795,18 @@ static void write_journal(struct dm_integrity_c *ic, unsigned commit_start, unsi

>  		to_end = ic->journal_sections - commit_start;

>  		if (ic->journal_io) {

>  			crypt_comp_1.ic = ic;

> -			crypt_comp_1.comp = COMPLETION_INITIALIZER_ONSTACK(crypt_comp_1.comp);

> +			init_completion(&crypt_comp_1.comp);

>  			crypt_comp_1.in_flight = (atomic_t)ATOMIC_INIT(0);

>  			encrypt_journal(ic, true, commit_start, to_end, &crypt_comp_1);

>  			if (try_wait_for_completion(&crypt_comp_1.comp)) {

>  				rw_journal(ic, REQ_OP_WRITE, REQ_FUA, commit_start, to_end, &io_comp);

> -				crypt_comp_1.comp = COMPLETION_INITIALIZER_ONSTACK(crypt_comp_1.comp);

> +				reinit_completion(&crypt_comp_1.comp);

>  				crypt_comp_1.in_flight = (atomic_t)ATOMIC_INIT(0);

>  				encrypt_journal(ic, true, 0, commit_sections - to_end, &crypt_comp_1);

>  				wait_for_completion_io(&crypt_comp_1.comp);

>  			} else {

>  				crypt_comp_2.ic = ic;

> -				crypt_comp_2.comp = COMPLETION_INITIALIZER_ONSTACK(crypt_comp_2.comp);

> +				init_completion(&crypt_comp_2.comp);

>  				crypt_comp_2.in_flight = (atomic_t)ATOMIC_INIT(0);

>  				encrypt_journal(ic, true, 0, commit_sections - to_end, &crypt_comp_2);

>  				wait_for_completion_io(&crypt_comp_1.comp);

> @@ -1679,7 +1679,7 @@ static void dm_integrity_map_continue(struct dm_integrity_io *dio, bool from_map

>  	dio->in_flight = (atomic_t)ATOMIC_INIT(2);

>  

>  	if (need_sync_io) {

> -		read_comp = COMPLETION_INITIALIZER_ONSTACK(read_comp);

> +		init_completion(&read_comp);

>  		dio->completion = &read_comp;

>  	} else

>  		dio->completion = NULL;

> @@ -1836,7 +1836,7 @@ static void do_journal_write(struct dm_integrity_c *ic, unsigned write_start,

>  

>  	comp.ic = ic;

>  	comp.in_flight = (atomic_t)ATOMIC_INIT(1);

> -	comp.comp = COMPLETION_INITIALIZER_ONSTACK(comp.comp);

> +	init_completion(&comp.comp);

>  

>  	i = write_start;

>  	for (n = 0; n < write_sections; n++, i++, wraparound_section(ic, &i)) {

> @@ -2063,7 +2063,7 @@ static void replay_journal(struct dm_integrity_c *ic)

>  		if (ic->journal_io) {

>  			struct journal_completion crypt_comp;

>  			crypt_comp.ic = ic;

> -			crypt_comp.comp = COMPLETION_INITIALIZER_ONSTACK(crypt_comp.comp);

> +			init_completion(&crypt_comp.comp);

>  			crypt_comp.in_flight = (atomic_t)ATOMIC_INIT(0);

>  			encrypt_journal(ic, false, 0, ic->journal_sections, &crypt_comp);

>  			wait_for_completion(&crypt_comp.comp);

> @@ -2636,7 +2636,7 @@ static int create_journal(struct dm_integrity_c *ic, char **error)

>  			memset(iv, 0x00, ivsize);

>  

>  			skcipher_request_set_crypt(req, sg, sg, PAGE_SIZE * ic->journal_pages + sizeof ic->commit_ids, iv);

> -			comp.comp = COMPLETION_INITIALIZER_ONSTACK(comp.comp);

> +			init_completion(&comp.comp);

>  			comp.in_flight = (atomic_t)ATOMIC_INIT(1);

>  			if (do_crypt(true, req, &comp))

>  				wait_for_completion(&comp.comp);

> @@ -2693,7 +2693,7 @@ static int create_journal(struct dm_integrity_c *ic, char **error)

>  

>  				sg_init_one(&sg, crypt_data, crypt_len);

>  				skcipher_request_set_crypt(req, &sg, &sg, crypt_len, iv);

> -				comp.comp = COMPLETION_INITIALIZER_ONSTACK(comp.comp);

> +				init_completion(&comp.comp);

>  				comp.in_flight = (atomic_t)ATOMIC_INIT(1);

>  				if (do_crypt(true, req, &comp))

>  					wait_for_completion(&comp.comp);

> -- 

> 2.9.0

> 

> --

> dm-devel mailing list

> dm-devel@redhat.com

> https://www.redhat.com/mailman/listinfo/dm-devel

>
Mike Snitzer Aug. 21, 2017, 4:13 p.m. UTC | #2
On Tue, Aug 15 2017 at 11:11am -0400,
Arnd Bergmann <arnd@arndb.de> wrote:

> The new lockdep support for completions causeed the stack usage

> in dm-integrity to explode, in case of write_journal from 504 bytes

> to 1120 (using arm gcc-7.1.1):

> 

> drivers/md/dm-integrity.c: In function 'write_journal':

> drivers/md/dm-integrity.c:827:1: error: the frame size of 1120 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]

> 

> The problem is that not only the size of 'struct completion' grows

> significantly, but we end up having multiple copies of it on the stack

> when we assign it from a local variable after the initial declaration.

> 

> COMPLETION_INITIALIZER_ONSTACK() is the right thing to use when we

> want to declare and initialize a completion on the stack. However,

> this driver doesn't do that and instead initializes the completion

> just before it is used.

> 

> In this case, init_completion() does the same thing more efficiently,

> and drops the stack usage for the function above down to 496 bytes.

> While the other functions in this file are not bad enough to cause

> a warning, they benefit equally from the change, so I do the change

> across the entire file. In the one place where we reuse a completion,

> I picked the cheaper reinit_completion() over init_completion().

> 

> Fixes: cd8084f91c02 ("locking/lockdep: Apply crossrelease to completions")

> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

> ---

> The patch causing this is currently part of linux-next, scheduled for

> 4.14, so it would be good to have this in the same release.


This looks good.  Please just stage it for 4.14 along with its
dependencies via whatever tree they are going in.

Acked-by: Mike Snitzer <snitzer@redhat.com>
diff mbox series

Patch

diff --git a/drivers/md/dm-integrity.c b/drivers/md/dm-integrity.c
index 293a19652d55..b16010bcbd17 100644
--- a/drivers/md/dm-integrity.c
+++ b/drivers/md/dm-integrity.c
@@ -773,13 +773,13 @@  static void write_journal(struct dm_integrity_c *ic, unsigned commit_start, unsi
 	unsigned i;
 
 	io_comp.ic = ic;
-	io_comp.comp = COMPLETION_INITIALIZER_ONSTACK(io_comp.comp);
+	init_completion(&io_comp.comp);
 
 	if (commit_start + commit_sections <= ic->journal_sections) {
 		io_comp.in_flight = (atomic_t)ATOMIC_INIT(1);
 		if (ic->journal_io) {
 			crypt_comp_1.ic = ic;
-			crypt_comp_1.comp = COMPLETION_INITIALIZER_ONSTACK(crypt_comp_1.comp);
+			init_completion(&crypt_comp_1.comp);
 			crypt_comp_1.in_flight = (atomic_t)ATOMIC_INIT(0);
 			encrypt_journal(ic, true, commit_start, commit_sections, &crypt_comp_1);
 			wait_for_completion_io(&crypt_comp_1.comp);
@@ -795,18 +795,18 @@  static void write_journal(struct dm_integrity_c *ic, unsigned commit_start, unsi
 		to_end = ic->journal_sections - commit_start;
 		if (ic->journal_io) {
 			crypt_comp_1.ic = ic;
-			crypt_comp_1.comp = COMPLETION_INITIALIZER_ONSTACK(crypt_comp_1.comp);
+			init_completion(&crypt_comp_1.comp);
 			crypt_comp_1.in_flight = (atomic_t)ATOMIC_INIT(0);
 			encrypt_journal(ic, true, commit_start, to_end, &crypt_comp_1);
 			if (try_wait_for_completion(&crypt_comp_1.comp)) {
 				rw_journal(ic, REQ_OP_WRITE, REQ_FUA, commit_start, to_end, &io_comp);
-				crypt_comp_1.comp = COMPLETION_INITIALIZER_ONSTACK(crypt_comp_1.comp);
+				reinit_completion(&crypt_comp_1.comp);
 				crypt_comp_1.in_flight = (atomic_t)ATOMIC_INIT(0);
 				encrypt_journal(ic, true, 0, commit_sections - to_end, &crypt_comp_1);
 				wait_for_completion_io(&crypt_comp_1.comp);
 			} else {
 				crypt_comp_2.ic = ic;
-				crypt_comp_2.comp = COMPLETION_INITIALIZER_ONSTACK(crypt_comp_2.comp);
+				init_completion(&crypt_comp_2.comp);
 				crypt_comp_2.in_flight = (atomic_t)ATOMIC_INIT(0);
 				encrypt_journal(ic, true, 0, commit_sections - to_end, &crypt_comp_2);
 				wait_for_completion_io(&crypt_comp_1.comp);
@@ -1679,7 +1679,7 @@  static void dm_integrity_map_continue(struct dm_integrity_io *dio, bool from_map
 	dio->in_flight = (atomic_t)ATOMIC_INIT(2);
 
 	if (need_sync_io) {
-		read_comp = COMPLETION_INITIALIZER_ONSTACK(read_comp);
+		init_completion(&read_comp);
 		dio->completion = &read_comp;
 	} else
 		dio->completion = NULL;
@@ -1836,7 +1836,7 @@  static void do_journal_write(struct dm_integrity_c *ic, unsigned write_start,
 
 	comp.ic = ic;
 	comp.in_flight = (atomic_t)ATOMIC_INIT(1);
-	comp.comp = COMPLETION_INITIALIZER_ONSTACK(comp.comp);
+	init_completion(&comp.comp);
 
 	i = write_start;
 	for (n = 0; n < write_sections; n++, i++, wraparound_section(ic, &i)) {
@@ -2063,7 +2063,7 @@  static void replay_journal(struct dm_integrity_c *ic)
 		if (ic->journal_io) {
 			struct journal_completion crypt_comp;
 			crypt_comp.ic = ic;
-			crypt_comp.comp = COMPLETION_INITIALIZER_ONSTACK(crypt_comp.comp);
+			init_completion(&crypt_comp.comp);
 			crypt_comp.in_flight = (atomic_t)ATOMIC_INIT(0);
 			encrypt_journal(ic, false, 0, ic->journal_sections, &crypt_comp);
 			wait_for_completion(&crypt_comp.comp);
@@ -2636,7 +2636,7 @@  static int create_journal(struct dm_integrity_c *ic, char **error)
 			memset(iv, 0x00, ivsize);
 
 			skcipher_request_set_crypt(req, sg, sg, PAGE_SIZE * ic->journal_pages + sizeof ic->commit_ids, iv);
-			comp.comp = COMPLETION_INITIALIZER_ONSTACK(comp.comp);
+			init_completion(&comp.comp);
 			comp.in_flight = (atomic_t)ATOMIC_INIT(1);
 			if (do_crypt(true, req, &comp))
 				wait_for_completion(&comp.comp);
@@ -2693,7 +2693,7 @@  static int create_journal(struct dm_integrity_c *ic, char **error)
 
 				sg_init_one(&sg, crypt_data, crypt_len);
 				skcipher_request_set_crypt(req, &sg, &sg, crypt_len, iv);
-				comp.comp = COMPLETION_INITIALIZER_ONSTACK(comp.comp);
+				init_completion(&comp.comp);
 				comp.in_flight = (atomic_t)ATOMIC_INIT(1);
 				if (do_crypt(true, req, &comp))
 					wait_for_completion(&comp.comp);