linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/2] mmc: test: reduce stack usage in mmc_test_nonblock_transfer
@ 2017-08-15 15:11 Arnd Bergmann
  2017-08-15 15:11 ` [PATCH 2/2] dm integrity: use init_completion instead of COMPLETION_INITIALIZER_ONSTACK Arnd Bergmann
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Arnd Bergmann @ 2017-08-15 15:11 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Ingo Molnar, Byungchul Park, Peter Zijlstra, Arnd Bergmann,
	Linus Walleij, Shawn Lin, Adrian Hunter, linux-mmc, linux-kernel

The new lockdep annotations for completions cause a warning in the
mmc test module, in a function that now has four 150 byte structures
on the stack:

drivers/mmc/core/mmc_test.c: In function 'mmc_test_nonblock_transfer.constprop':
drivers/mmc/core/mmc_test.c:892:1: error: the frame size of 1360 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]

The mmc_test_ongoing_transfer function evidently had a similar problem,
and worked around it by using dynamic allocation.

This generalizes the approach used by mmc_test_ongoing_transfer() and
applies it to mmc_test_nonblock_transfer() as well.

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.

Since the change is not entirely trivial, please test this before applying.
---
 drivers/mmc/core/mmc_test.c | 97 +++++++++++++++++++--------------------------
 1 file changed, 41 insertions(+), 56 deletions(-)

diff --git a/drivers/mmc/core/mmc_test.c b/drivers/mmc/core/mmc_test.c
index 7a304a6e5bf1..478869805b96 100644
--- a/drivers/mmc/core/mmc_test.c
+++ b/drivers/mmc/core/mmc_test.c
@@ -800,38 +800,44 @@ static int mmc_test_check_broken_result(struct mmc_test_card *test,
 	return ret;
 }
 
+struct mmc_test_req {
+	struct mmc_request mrq;
+	struct mmc_command sbc;
+	struct mmc_command cmd;
+	struct mmc_command stop;
+	struct mmc_command status;
+	struct mmc_data data;
+};
+
 /*
  * Tests nonblock transfer with certain parameters
  */
-static void mmc_test_nonblock_reset(struct mmc_request *mrq,
-				    struct mmc_command *cmd,
-				    struct mmc_command *stop,
-				    struct mmc_data *data)
+static void mmc_test_req_reset(struct mmc_test_req *rq)
+{
+	memset(rq, 0, sizeof(struct mmc_test_req));
+
+	rq->mrq.cmd = &rq->cmd;
+	rq->mrq.data = &rq->data;
+	rq->mrq.stop = &rq->stop;
+}
+
+static struct mmc_test_req *mmc_test_req_alloc(void)
 {
-	memset(mrq, 0, sizeof(struct mmc_request));
-	memset(cmd, 0, sizeof(struct mmc_command));
-	memset(data, 0, sizeof(struct mmc_data));
-	memset(stop, 0, sizeof(struct mmc_command));
+	struct mmc_test_req *rq = kmalloc(sizeof(*rq), GFP_KERNEL);
 
-	mrq->cmd = cmd;
-	mrq->data = data;
-	mrq->stop = stop;
+	if (rq)
+		mmc_test_req_reset(rq);
+
+	return rq;
 }
+
+
 static int mmc_test_nonblock_transfer(struct mmc_test_card *test,
 				      struct scatterlist *sg, unsigned sg_len,
 				      unsigned dev_addr, unsigned blocks,
 				      unsigned blksz, int write, int count)
 {
-	struct mmc_request mrq1;
-	struct mmc_command cmd1;
-	struct mmc_command stop1;
-	struct mmc_data data1;
-
-	struct mmc_request mrq2;
-	struct mmc_command cmd2;
-	struct mmc_command stop2;
-	struct mmc_data data2;
-
+	struct mmc_test_req *rq1, *rq2;
 	struct mmc_test_async_req test_areq[2];
 	struct mmc_async_req *done_areq;
 	struct mmc_async_req *cur_areq = &test_areq[0].areq;
@@ -843,12 +849,16 @@ static int mmc_test_nonblock_transfer(struct mmc_test_card *test,
 	test_areq[0].test = test;
 	test_areq[1].test = test;
 
-	mmc_test_nonblock_reset(&mrq1, &cmd1, &stop1, &data1);
-	mmc_test_nonblock_reset(&mrq2, &cmd2, &stop2, &data2);
+	rq1 = mmc_test_req_alloc();
+	rq2 = mmc_test_req_alloc();
+	if (!rq1 || !rq2) {
+		ret = RESULT_FAIL;
+		goto err;
+	}
 
-	cur_areq->mrq = &mrq1;
+	cur_areq->mrq = &rq1->mrq;
 	cur_areq->err_check = mmc_test_check_result_async;
-	other_areq->mrq = &mrq2;
+	other_areq->mrq = &rq2->mrq;
 	other_areq->err_check = mmc_test_check_result_async;
 
 	for (i = 0; i < count; i++) {
@@ -861,14 +871,10 @@ static int mmc_test_nonblock_transfer(struct mmc_test_card *test,
 			goto err;
 		}
 
-		if (done_areq) {
-			if (done_areq->mrq == &mrq2)
-				mmc_test_nonblock_reset(&mrq2, &cmd2,
-							&stop2, &data2);
-			else
-				mmc_test_nonblock_reset(&mrq1, &cmd1,
-							&stop1, &data1);
-		}
+		if (done_areq)
+			mmc_test_req_reset(container_of(done_areq->mrq,
+						struct mmc_test_req, mrq));
+
 		swap(cur_areq, other_areq);
 		dev_addr += blocks;
 	}
@@ -877,8 +883,9 @@ static int mmc_test_nonblock_transfer(struct mmc_test_card *test,
 	if (status != MMC_BLK_SUCCESS)
 		ret = RESULT_FAIL;
 
-	return ret;
 err:
+	kfree(rq1);
+	kfree(rq2);
 	return ret;
 }
 
@@ -2329,28 +2336,6 @@ static int mmc_test_reset(struct mmc_test_card *test)
 	return RESULT_FAIL;
 }
 
-struct mmc_test_req {
-	struct mmc_request mrq;
-	struct mmc_command sbc;
-	struct mmc_command cmd;
-	struct mmc_command stop;
-	struct mmc_command status;
-	struct mmc_data data;
-};
-
-static struct mmc_test_req *mmc_test_req_alloc(void)
-{
-	struct mmc_test_req *rq = kzalloc(sizeof(*rq), GFP_KERNEL);
-
-	if (rq) {
-		rq->mrq.cmd = &rq->cmd;
-		rq->mrq.data = &rq->data;
-		rq->mrq.stop = &rq->stop;
-	}
-
-	return rq;
-}
-
 static int mmc_test_send_status(struct mmc_test_card *test,
 				struct mmc_command *cmd)
 {
-- 
2.9.0

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

* [PATCH 2/2] dm integrity: use init_completion instead of COMPLETION_INITIALIZER_ONSTACK
  2017-08-15 15:11 [PATCH 1/2] mmc: test: reduce stack usage in mmc_test_nonblock_transfer Arnd Bergmann
@ 2017-08-15 15:11 ` Arnd Bergmann
  2017-08-16 16:12   ` [dm-devel] " Mikulas Patocka
  2017-08-21 16:13   ` Mike Snitzer
  2017-08-16 10:14 ` [PATCH 1/2] mmc: test: reduce stack usage in mmc_test_nonblock_transfer Adrian Hunter
  2017-08-22 11:14 ` Ulf Hansson
  2 siblings, 2 replies; 6+ messages in thread
From: Arnd Bergmann @ 2017-08-15 15:11 UTC (permalink / raw)
  To: Alasdair Kergon, Mike Snitzer, dm-devel, Shaohua Li
  Cc: Ingo Molnar, Byungchul Park, Peter Zijlstra, Arnd Bergmann,
	Mikulas Patocka, Jens Axboe, linux-raid, linux-kernel

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

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

* Re: [PATCH 1/2] mmc: test: reduce stack usage in mmc_test_nonblock_transfer
  2017-08-15 15:11 [PATCH 1/2] mmc: test: reduce stack usage in mmc_test_nonblock_transfer Arnd Bergmann
  2017-08-15 15:11 ` [PATCH 2/2] dm integrity: use init_completion instead of COMPLETION_INITIALIZER_ONSTACK Arnd Bergmann
@ 2017-08-16 10:14 ` Adrian Hunter
  2017-08-22 11:14 ` Ulf Hansson
  2 siblings, 0 replies; 6+ messages in thread
From: Adrian Hunter @ 2017-08-16 10:14 UTC (permalink / raw)
  To: Arnd Bergmann, Ulf Hansson
  Cc: Ingo Molnar, Byungchul Park, Peter Zijlstra, Linus Walleij,
	Shawn Lin, linux-mmc, linux-kernel

On 15/08/17 18:11, Arnd Bergmann wrote:
> The new lockdep annotations for completions cause a warning in the
> mmc test module, in a function that now has four 150 byte structures
> on the stack:
> 
> drivers/mmc/core/mmc_test.c: In function 'mmc_test_nonblock_transfer.constprop':
> drivers/mmc/core/mmc_test.c:892:1: error: the frame size of 1360 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
> 
> The mmc_test_ongoing_transfer function evidently had a similar problem,
> and worked around it by using dynamic allocation.
> 
> This generalizes the approach used by mmc_test_ongoing_transfer() and
> applies it to mmc_test_nonblock_transfer() as well.
> 
> Fixes: cd8084f91c02 ("locking/lockdep: Apply crossrelease to completions")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Apart from duplicate blank line pointed out below:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>
Tested-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
> 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.
> 
> Since the change is not entirely trivial, please test this before applying.
> ---
>  drivers/mmc/core/mmc_test.c | 97 +++++++++++++++++++--------------------------
>  1 file changed, 41 insertions(+), 56 deletions(-)
> 
> diff --git a/drivers/mmc/core/mmc_test.c b/drivers/mmc/core/mmc_test.c
> index 7a304a6e5bf1..478869805b96 100644
> --- a/drivers/mmc/core/mmc_test.c
> +++ b/drivers/mmc/core/mmc_test.c
> @@ -800,38 +800,44 @@ static int mmc_test_check_broken_result(struct mmc_test_card *test,
>  	return ret;
>  }
>  
> +struct mmc_test_req {
> +	struct mmc_request mrq;
> +	struct mmc_command sbc;
> +	struct mmc_command cmd;
> +	struct mmc_command stop;
> +	struct mmc_command status;
> +	struct mmc_data data;
> +};
> +
>  /*
>   * Tests nonblock transfer with certain parameters
>   */
> -static void mmc_test_nonblock_reset(struct mmc_request *mrq,
> -				    struct mmc_command *cmd,
> -				    struct mmc_command *stop,
> -				    struct mmc_data *data)
> +static void mmc_test_req_reset(struct mmc_test_req *rq)
> +{
> +	memset(rq, 0, sizeof(struct mmc_test_req));
> +
> +	rq->mrq.cmd = &rq->cmd;
> +	rq->mrq.data = &rq->data;
> +	rq->mrq.stop = &rq->stop;
> +}
> +
> +static struct mmc_test_req *mmc_test_req_alloc(void)
>  {
> -	memset(mrq, 0, sizeof(struct mmc_request));
> -	memset(cmd, 0, sizeof(struct mmc_command));
> -	memset(data, 0, sizeof(struct mmc_data));
> -	memset(stop, 0, sizeof(struct mmc_command));
> +	struct mmc_test_req *rq = kmalloc(sizeof(*rq), GFP_KERNEL);
>  
> -	mrq->cmd = cmd;
> -	mrq->data = data;
> -	mrq->stop = stop;
> +	if (rq)
> +		mmc_test_req_reset(rq);
> +
> +	return rq;
>  }
> +
> +

Duplicate blank line

>  static int mmc_test_nonblock_transfer(struct mmc_test_card *test,
>  				      struct scatterlist *sg, unsigned sg_len,
>  				      unsigned dev_addr, unsigned blocks,
>  				      unsigned blksz, int write, int count)
>  {
> -	struct mmc_request mrq1;
> -	struct mmc_command cmd1;
> -	struct mmc_command stop1;
> -	struct mmc_data data1;
> -
> -	struct mmc_request mrq2;
> -	struct mmc_command cmd2;
> -	struct mmc_command stop2;
> -	struct mmc_data data2;
> -
> +	struct mmc_test_req *rq1, *rq2;
>  	struct mmc_test_async_req test_areq[2];
>  	struct mmc_async_req *done_areq;
>  	struct mmc_async_req *cur_areq = &test_areq[0].areq;
> @@ -843,12 +849,16 @@ static int mmc_test_nonblock_transfer(struct mmc_test_card *test,
>  	test_areq[0].test = test;
>  	test_areq[1].test = test;
>  
> -	mmc_test_nonblock_reset(&mrq1, &cmd1, &stop1, &data1);
> -	mmc_test_nonblock_reset(&mrq2, &cmd2, &stop2, &data2);
> +	rq1 = mmc_test_req_alloc();
> +	rq2 = mmc_test_req_alloc();
> +	if (!rq1 || !rq2) {
> +		ret = RESULT_FAIL;
> +		goto err;
> +	}
>  
> -	cur_areq->mrq = &mrq1;
> +	cur_areq->mrq = &rq1->mrq;
>  	cur_areq->err_check = mmc_test_check_result_async;
> -	other_areq->mrq = &mrq2;
> +	other_areq->mrq = &rq2->mrq;
>  	other_areq->err_check = mmc_test_check_result_async;
>  
>  	for (i = 0; i < count; i++) {
> @@ -861,14 +871,10 @@ static int mmc_test_nonblock_transfer(struct mmc_test_card *test,
>  			goto err;
>  		}
>  
> -		if (done_areq) {
> -			if (done_areq->mrq == &mrq2)
> -				mmc_test_nonblock_reset(&mrq2, &cmd2,
> -							&stop2, &data2);
> -			else
> -				mmc_test_nonblock_reset(&mrq1, &cmd1,
> -							&stop1, &data1);
> -		}
> +		if (done_areq)
> +			mmc_test_req_reset(container_of(done_areq->mrq,
> +						struct mmc_test_req, mrq));
> +
>  		swap(cur_areq, other_areq);
>  		dev_addr += blocks;
>  	}
> @@ -877,8 +883,9 @@ static int mmc_test_nonblock_transfer(struct mmc_test_card *test,
>  	if (status != MMC_BLK_SUCCESS)
>  		ret = RESULT_FAIL;
>  
> -	return ret;
>  err:
> +	kfree(rq1);
> +	kfree(rq2);
>  	return ret;
>  }
>  
> @@ -2329,28 +2336,6 @@ static int mmc_test_reset(struct mmc_test_card *test)
>  	return RESULT_FAIL;
>  }
>  
> -struct mmc_test_req {
> -	struct mmc_request mrq;
> -	struct mmc_command sbc;
> -	struct mmc_command cmd;
> -	struct mmc_command stop;
> -	struct mmc_command status;
> -	struct mmc_data data;
> -};
> -
> -static struct mmc_test_req *mmc_test_req_alloc(void)
> -{
> -	struct mmc_test_req *rq = kzalloc(sizeof(*rq), GFP_KERNEL);
> -
> -	if (rq) {
> -		rq->mrq.cmd = &rq->cmd;
> -		rq->mrq.data = &rq->data;
> -		rq->mrq.stop = &rq->stop;
> -	}
> -
> -	return rq;
> -}
> -
>  static int mmc_test_send_status(struct mmc_test_card *test,
>  				struct mmc_command *cmd)
>  {
> 

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

* Re: [dm-devel] [PATCH 2/2] dm integrity: use init_completion instead of COMPLETION_INITIALIZER_ONSTACK
  2017-08-15 15:11 ` [PATCH 2/2] dm integrity: use init_completion instead of COMPLETION_INITIALIZER_ONSTACK Arnd Bergmann
@ 2017-08-16 16:12   ` Mikulas Patocka
  2017-08-21 16:13   ` Mike Snitzer
  1 sibling, 0 replies; 6+ messages in thread
From: Mikulas Patocka @ 2017-08-16 16:12 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Alasdair Kergon, Mike Snitzer, dm-devel, Shaohua Li, Jens Axboe,
	linux-raid, Peter Zijlstra, linux-kernel, Byungchul Park,
	Ingo Molnar

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
> 

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

* Re: [PATCH 2/2] dm integrity: use init_completion instead of COMPLETION_INITIALIZER_ONSTACK
  2017-08-15 15:11 ` [PATCH 2/2] dm integrity: use init_completion instead of COMPLETION_INITIALIZER_ONSTACK Arnd Bergmann
  2017-08-16 16:12   ` [dm-devel] " Mikulas Patocka
@ 2017-08-21 16:13   ` Mike Snitzer
  1 sibling, 0 replies; 6+ messages in thread
From: Mike Snitzer @ 2017-08-21 16:13 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Alasdair Kergon, dm-devel, Shaohua Li, Ingo Molnar,
	Byungchul Park, Peter Zijlstra, Mikulas Patocka, Jens Axboe,
	linux-raid, linux-kernel

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>

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

* Re: [PATCH 1/2] mmc: test: reduce stack usage in mmc_test_nonblock_transfer
  2017-08-15 15:11 [PATCH 1/2] mmc: test: reduce stack usage in mmc_test_nonblock_transfer Arnd Bergmann
  2017-08-15 15:11 ` [PATCH 2/2] dm integrity: use init_completion instead of COMPLETION_INITIALIZER_ONSTACK Arnd Bergmann
  2017-08-16 10:14 ` [PATCH 1/2] mmc: test: reduce stack usage in mmc_test_nonblock_transfer Adrian Hunter
@ 2017-08-22 11:14 ` Ulf Hansson
  2 siblings, 0 replies; 6+ messages in thread
From: Ulf Hansson @ 2017-08-22 11:14 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: Ingo Molnar, Byungchul Park, Peter Zijlstra, Linus Walleij,
	Shawn Lin, Adrian Hunter, linux-mmc, linux-kernel

On 15 August 2017 at 17:11, Arnd Bergmann <arnd@arndb.de> wrote:
> The new lockdep annotations for completions cause a warning in the
> mmc test module, in a function that now has four 150 byte structures
> on the stack:
>
> drivers/mmc/core/mmc_test.c: In function 'mmc_test_nonblock_transfer.constprop':
> drivers/mmc/core/mmc_test.c:892:1: error: the frame size of 1360 bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
>
> The mmc_test_ongoing_transfer function evidently had a similar problem,
> and worked around it by using dynamic allocation.
>
> This generalizes the approach used by mmc_test_ongoing_transfer() and
> applies it to mmc_test_nonblock_transfer() as well.
>
> Fixes: cd8084f91c02 ("locking/lockdep: Apply crossrelease to completions")
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Thanks, applied for next!

Kind regards
Uffe

> ---
> 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.
>
> Since the change is not entirely trivial, please test this before applying.
> ---
>  drivers/mmc/core/mmc_test.c | 97 +++++++++++++++++++--------------------------
>  1 file changed, 41 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/mmc/core/mmc_test.c b/drivers/mmc/core/mmc_test.c
> index 7a304a6e5bf1..478869805b96 100644
> --- a/drivers/mmc/core/mmc_test.c
> +++ b/drivers/mmc/core/mmc_test.c
> @@ -800,38 +800,44 @@ static int mmc_test_check_broken_result(struct mmc_test_card *test,
>         return ret;
>  }
>
> +struct mmc_test_req {
> +       struct mmc_request mrq;
> +       struct mmc_command sbc;
> +       struct mmc_command cmd;
> +       struct mmc_command stop;
> +       struct mmc_command status;
> +       struct mmc_data data;
> +};
> +
>  /*
>   * Tests nonblock transfer with certain parameters
>   */
> -static void mmc_test_nonblock_reset(struct mmc_request *mrq,
> -                                   struct mmc_command *cmd,
> -                                   struct mmc_command *stop,
> -                                   struct mmc_data *data)
> +static void mmc_test_req_reset(struct mmc_test_req *rq)
> +{
> +       memset(rq, 0, sizeof(struct mmc_test_req));
> +
> +       rq->mrq.cmd = &rq->cmd;
> +       rq->mrq.data = &rq->data;
> +       rq->mrq.stop = &rq->stop;
> +}
> +
> +static struct mmc_test_req *mmc_test_req_alloc(void)
>  {
> -       memset(mrq, 0, sizeof(struct mmc_request));
> -       memset(cmd, 0, sizeof(struct mmc_command));
> -       memset(data, 0, sizeof(struct mmc_data));
> -       memset(stop, 0, sizeof(struct mmc_command));
> +       struct mmc_test_req *rq = kmalloc(sizeof(*rq), GFP_KERNEL);
>
> -       mrq->cmd = cmd;
> -       mrq->data = data;
> -       mrq->stop = stop;
> +       if (rq)
> +               mmc_test_req_reset(rq);
> +
> +       return rq;
>  }
> +
> +
>  static int mmc_test_nonblock_transfer(struct mmc_test_card *test,
>                                       struct scatterlist *sg, unsigned sg_len,
>                                       unsigned dev_addr, unsigned blocks,
>                                       unsigned blksz, int write, int count)
>  {
> -       struct mmc_request mrq1;
> -       struct mmc_command cmd1;
> -       struct mmc_command stop1;
> -       struct mmc_data data1;
> -
> -       struct mmc_request mrq2;
> -       struct mmc_command cmd2;
> -       struct mmc_command stop2;
> -       struct mmc_data data2;
> -
> +       struct mmc_test_req *rq1, *rq2;
>         struct mmc_test_async_req test_areq[2];
>         struct mmc_async_req *done_areq;
>         struct mmc_async_req *cur_areq = &test_areq[0].areq;
> @@ -843,12 +849,16 @@ static int mmc_test_nonblock_transfer(struct mmc_test_card *test,
>         test_areq[0].test = test;
>         test_areq[1].test = test;
>
> -       mmc_test_nonblock_reset(&mrq1, &cmd1, &stop1, &data1);
> -       mmc_test_nonblock_reset(&mrq2, &cmd2, &stop2, &data2);
> +       rq1 = mmc_test_req_alloc();
> +       rq2 = mmc_test_req_alloc();
> +       if (!rq1 || !rq2) {
> +               ret = RESULT_FAIL;
> +               goto err;
> +       }
>
> -       cur_areq->mrq = &mrq1;
> +       cur_areq->mrq = &rq1->mrq;
>         cur_areq->err_check = mmc_test_check_result_async;
> -       other_areq->mrq = &mrq2;
> +       other_areq->mrq = &rq2->mrq;
>         other_areq->err_check = mmc_test_check_result_async;
>
>         for (i = 0; i < count; i++) {
> @@ -861,14 +871,10 @@ static int mmc_test_nonblock_transfer(struct mmc_test_card *test,
>                         goto err;
>                 }
>
> -               if (done_areq) {
> -                       if (done_areq->mrq == &mrq2)
> -                               mmc_test_nonblock_reset(&mrq2, &cmd2,
> -                                                       &stop2, &data2);
> -                       else
> -                               mmc_test_nonblock_reset(&mrq1, &cmd1,
> -                                                       &stop1, &data1);
> -               }
> +               if (done_areq)
> +                       mmc_test_req_reset(container_of(done_areq->mrq,
> +                                               struct mmc_test_req, mrq));
> +
>                 swap(cur_areq, other_areq);
>                 dev_addr += blocks;
>         }
> @@ -877,8 +883,9 @@ static int mmc_test_nonblock_transfer(struct mmc_test_card *test,
>         if (status != MMC_BLK_SUCCESS)
>                 ret = RESULT_FAIL;
>
> -       return ret;
>  err:
> +       kfree(rq1);
> +       kfree(rq2);
>         return ret;
>  }
>
> @@ -2329,28 +2336,6 @@ static int mmc_test_reset(struct mmc_test_card *test)
>         return RESULT_FAIL;
>  }
>
> -struct mmc_test_req {
> -       struct mmc_request mrq;
> -       struct mmc_command sbc;
> -       struct mmc_command cmd;
> -       struct mmc_command stop;
> -       struct mmc_command status;
> -       struct mmc_data data;
> -};
> -
> -static struct mmc_test_req *mmc_test_req_alloc(void)
> -{
> -       struct mmc_test_req *rq = kzalloc(sizeof(*rq), GFP_KERNEL);
> -
> -       if (rq) {
> -               rq->mrq.cmd = &rq->cmd;
> -               rq->mrq.data = &rq->data;
> -               rq->mrq.stop = &rq->stop;
> -       }
> -
> -       return rq;
> -}
> -
>  static int mmc_test_send_status(struct mmc_test_card *test,
>                                 struct mmc_command *cmd)
>  {
> --
> 2.9.0
>

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

end of thread, other threads:[~2017-08-22 11:14 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-08-15 15:11 [PATCH 1/2] mmc: test: reduce stack usage in mmc_test_nonblock_transfer Arnd Bergmann
2017-08-15 15:11 ` [PATCH 2/2] dm integrity: use init_completion instead of COMPLETION_INITIALIZER_ONSTACK Arnd Bergmann
2017-08-16 16:12   ` [dm-devel] " Mikulas Patocka
2017-08-21 16:13   ` Mike Snitzer
2017-08-16 10:14 ` [PATCH 1/2] mmc: test: reduce stack usage in mmc_test_nonblock_transfer Adrian Hunter
2017-08-22 11:14 ` Ulf Hansson

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