[PATCH 0/2] completion: Reduce stack usage caused by COMPLETION_INITIALIZER_ONSTACK()

[PATCH 0/2] completion: Reduce stack usage caused by COMPLETION_INITIALIZER_ONSTACK()

[Boqun Feng]

With LOCKDEP_CROSSRELEASE and LOCKDEP_COMPLETIONS introduced, the growth
in kernel stack usage of several functions were reported:

	https://marc.info/?l=linux-kernel&m=150270063231284&w=2

The root cause of this is in COMPLETION_INITIALIZER_ONSTACK(), we use

	({init_completion(&work); work})

, which will create a temporary object when returned. However this
temporary object is unnecessary. And this patch fixes it by making the
statement expression in COMPLETION_INITIALIZER_ONSTACK() return a
pointer rather than a whole structure. This will reduce the stack usage
even if !LOCKDEP.

However, such a change does make one COMPLETION_INITIALIZER_ONSTACK()
callsite invalid, so we fix this first via converting to
init_completion().

Regards,
Boqun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 1/2] nfit: Use init_completion() in acpi_nfit_flush_probe()

[Boqun Feng]

There is no need to use COMPLETION_INITIALIZER_ONSTACK() in
acpi_nfit_flush_probe(), replace it with init_completion().

Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
 drivers/acpi/nfit/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 19182d091587..1893e416e7c0 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -2884,7 +2884,7 @@ static int acpi_nfit_flush_probe(struct nvdimm_bus_descriptor *nd_desc)
 	 * need to be interruptible while waiting.
 	 */
 	INIT_WORK_ONSTACK(&flush.work, flush_probe);
-	COMPLETION_INITIALIZER_ONSTACK(flush.cmp);
+	init_completion(&flush.cmp);
 	queue_work(nfit_wq, &flush.work);
 	mutex_unlock(&acpi_desc->init_mutex);
 
-- 
2.14.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH 2/2] completion: Avoid unnecessary stack allocation for COMPLETION_INITIALIZER_ONSTACK()

[Boqun Feng]

In theory, COMPLETION_INITIALIZER_ONSTACK() should never affect the
stack allocation of the caller. However, on some compilers, a temporary
structure was allocated for the return value of
COMPLETION_INITIALIZER_ONSTACK(), for example in write_journal() with
LOCKDEP_COMPLETIONS=y(gcc is 7.1.1):

	io_comp.comp = COMPLETION_INITIALIZER_ONSTACK(io_comp.comp);
	    2462:       e8 00 00 00 00          callq  2467 <write_journal+0x47>
	    2467:       48 8d 85 80 fd ff ff    lea    -0x280(%rbp),%rax
	    246e:       48 c7 c6 00 00 00 00    mov    $0x0,%rsi
	    2475:       48 c7 c2 00 00 00 00    mov    $0x0,%rdx
		x->done = 0;
	    247c:       c7 85 90 fd ff ff 00    movl   $0x0,-0x270(%rbp)
	    2483:       00 00 00
		init_waitqueue_head(&x->wait);
	    2486:       48 8d 78 18             lea    0x18(%rax),%rdi
	    248a:       e8 00 00 00 00          callq  248f <write_journal+0x6f>
		if (commit_start + commit_sections <= ic->journal_sections) {
	    248f:       41 8b 87 a8 00 00 00    mov    0xa8(%r15),%eax
		io_comp.comp = COMPLETION_INITIALIZER_ONSTACK(io_comp.comp);
	    2496:       48 8d bd e8 f9 ff ff    lea    -0x618(%rbp),%rdi
	    249d:       48 8d b5 90 fd ff ff    lea    -0x270(%rbp),%rsi
	    24a4:       b9 17 00 00 00          mov    $0x17,%ecx
	    24a9:       f3 48 a5                rep movsq %ds:(%rsi),%es:(%rdi)
		if (commit_start + commit_sections <= ic->journal_sections) {
	    24ac:       41 39 c6                cmp    %eax,%r14d
		io_comp.comp = COMPLETION_INITIALIZER_ONSTACK(io_comp.comp);
	    24af:       48 8d bd 90 fd ff ff    lea    -0x270(%rbp),%rdi
	    24b6:       48 8d b5 e8 f9 ff ff    lea    -0x618(%rbp),%rsi
	    24bd:       b9 17 00 00 00          mov    $0x17,%ecx
	    24c2:       f3 48 a5                rep movsq %ds:(%rsi),%es:(%rdi)

We can obviously see the temporary structure allocated, and the compiler
also does two meaningless memcpy with "rep movsq".

And according to:

	https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html#Statement-Exprs

The return value of a statement expression is returned by value, so the
temporary variable is created in COMPLETION_INITIALIZER_ONSTACK(), and
that's why the temporary structures are allocted.

To fix this, make the brace block in COMPLETION_INITIALIZER_ONSTACK()
return a pointer and dereference it outside the block rather than return
the whole structure, in this way, we are able to teach the compiler not
to do the unnecessary stack allocation.

This could also reduce the stack size even if !LOCKDEP, for example in
write_journal(), compiled with gcc 7.1.1, the result of command:

	 objdump -d drivers/md/dm-integrity.o | ./scripts/checkstack.pl x86

before:

	0x0000246a write_journal [dm-integrity.o]:              696

after:

	0x00002b7a write_journal [dm-integrity.o]:              296

Reported-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
 include/linux/completion.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/completion.h b/include/linux/completion.h
index 791f053f28b7..cae5400022a3 100644
--- a/include/linux/completion.h
+++ b/include/linux/completion.h
@@ -74,7 +74,7 @@ static inline void complete_release_commit(struct completion *x) {}
 #endif
 
 #define COMPLETION_INITIALIZER_ONSTACK(work) \
-	({ init_completion(&work); work; })
+	(*({ init_completion(&work); &work; }))
 
 /**
  * DECLARE_COMPLETION - declare and initialize a completion structure
-- 
2.14.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 0/2] completion: Reduce stack usage caused by COMPLETION_INITIALIZER_ONSTACK()

[Arnd Bergmann]

On Wed, Aug 23, 2017 at 5:25 PM, Boqun Feng <boqun.feng@gmail.com> wrote:
> With LOCKDEP_CROSSRELEASE and LOCKDEP_COMPLETIONS introduced, the growth
> in kernel stack usage of several functions were reported:
>
>         https://marc.info/?l=linux-kernel&m=150270063231284&w=2
>
> The root cause of this is in COMPLETION_INITIALIZER_ONSTACK(), we use
>
>         ({init_completion(&work); work})
>
> , which will create a temporary object when returned. However this
> temporary object is unnecessary. And this patch fixes it by making the
> statement expression in COMPLETION_INITIALIZER_ONSTACK() return a
> pointer rather than a whole structure. This will reduce the stack usage
> even if !LOCKDEP.
>
> However, such a change does make one COMPLETION_INITIALIZER_ONSTACK()
> callsite invalid, so we fix this first via converting to
> init_completion().

Both patches

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

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] nfit: Use init_completion() in acpi_nfit_flush_probe()

[Dan Williams]

On Wed, Aug 23, 2017 at 8:25 AM, Boqun Feng <boqun.feng@gmail.com> wrote:
> There is no need to use COMPLETION_INITIALIZER_ONSTACK() in
> acpi_nfit_flush_probe(), replace it with init_completion().

Now that I see the better version of this patch with the improved
changelog in the -mm tree...

Acked-by: Dan Williams <dan.j.williams@intel.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] nfit: Use init_completion() in acpi_nfit_flush_probe()

[Thomas Gleixner]

On Wed, 23 Aug 2017, Boqun Feng wrote:

> There is no need to use COMPLETION_INITIALIZER_ONSTACK() in
> acpi_nfit_flush_probe(), replace it with init_completion().

You completely fail to explain WHY.

Thanks,

	tglx

 
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> ---
>  drivers/acpi/nfit/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 19182d091587..1893e416e7c0 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -2884,7 +2884,7 @@ static int acpi_nfit_flush_probe(struct nvdimm_bus_descriptor *nd_desc)
>  	 * need to be interruptible while waiting.
>  	 */
>  	INIT_WORK_ONSTACK(&flush.work, flush_probe);
> -	COMPLETION_INITIALIZER_ONSTACK(flush.cmp);
> +	init_completion(&flush.cmp);
>  	queue_work(nfit_wq, &flush.work);
>  	mutex_unlock(&acpi_desc->init_mutex);
>  
> -- 
> 2.14.1
> 
> 

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH 1/2] nfit: Use init_completion() in acpi_nfit_flush_probe()

[Boqun Feng]

[-- Attachment #1: Type: text/plain, Size: 1566 bytes --]

On Thu, Aug 24, 2017 at 03:07:42PM +0200, Thomas Gleixner wrote:
> On Wed, 23 Aug 2017, Boqun Feng wrote:
> 
> > There is no need to use COMPLETION_INITIALIZER_ONSTACK() in
> > acpi_nfit_flush_probe(), replace it with init_completion().
> 
> You completely fail to explain WHY.
> 

I thought COMPLETION_INITIALIZER_ONSTACK() should only use in assigment
or compound literals, so the usage here is obviously wrong, but seems
I was wrong?

Ingo,

Is the usage of COMPLETION_INITIALIZER_ONSTACK() correct? If not,
I could rephrase my commit log saying this is a fix for wrong usage of
COMPLETION_INITIALIZER_ONSTACK(), otherwise, I will rewrite the commit
indicating this patch is a necessary dependency for patch #2. Thanks!

Regards,
Boqun

> Thanks,
> 
> 	tglx
> 
>  
> > Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> > ---
> >  drivers/acpi/nfit/core.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> > index 19182d091587..1893e416e7c0 100644
> > --- a/drivers/acpi/nfit/core.c
> > +++ b/drivers/acpi/nfit/core.c
> > @@ -2884,7 +2884,7 @@ static int acpi_nfit_flush_probe(struct nvdimm_bus_descriptor *nd_desc)
> >  	 * need to be interruptible while waiting.
> >  	 */
> >  	INIT_WORK_ONSTACK(&flush.work, flush_probe);
> > -	COMPLETION_INITIALIZER_ONSTACK(flush.cmp);
> > +	init_completion(&flush.cmp);
> >  	queue_work(nfit_wq, &flush.work);
> >  	mutex_unlock(&acpi_desc->init_mutex);
> >  
> > -- 
> > 2.14.1
> > 
> > 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/2] nfit: Use init_completion() in acpi_nfit_flush_probe()

[Arnd Bergmann]

On Thu, Aug 24, 2017 at 3:28 PM, Boqun Feng <boqun.feng@gmail.com> wrote:
> On Thu, Aug 24, 2017 at 03:07:42PM +0200, Thomas Gleixner wrote:
>> On Wed, 23 Aug 2017, Boqun Feng wrote:
>>
>> > There is no need to use COMPLETION_INITIALIZER_ONSTACK() in
>> > acpi_nfit_flush_probe(), replace it with init_completion().
>>
>> You completely fail to explain WHY.
>>
>
> I thought COMPLETION_INITIALIZER_ONSTACK() should only use in assigment
> or compound literals, so the usage here is obviously wrong, but seems
> I was wrong?
>
> Ingo,
>
> Is the usage of COMPLETION_INITIALIZER_ONSTACK() correct? If not,
> I could rephrase my commit log saying this is a fix for wrong usage of
> COMPLETION_INITIALIZER_ONSTACK(), otherwise, I will rewrite the commit
> indicating this patch is a necessary dependency for patch #2. Thanks!

I think your patch is correct, but your changelog text is useless, as
Thomas mentioned: you should instead explain that it breaks with the
other fix in the series, and what the difference between init_completion()
and COMPLETION_INITIALIZER_ONSTACK() is.

      Arnd

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

[PATCH v2 1/2] nfit: Fix the abuse of COMPLETION_INITIALIZER_ONSTACK()

[Boqun Feng]

COMPLETION_INITIALIZER_ONSTACK() is supposed to used as an initializer,
in other words, it should only be used in assignment expressions or
compound literals. So the usage in drivers/acpi/nfit/core.c:

	COMPLETION_INITIALIZER_ONSTACK(flush.cmp);

, is inappropriate.

Besides, this usage could also break compilations for another fix to
reduce stack sizes caused by COMPLETION_INITIALIZER_ONSTACK(), because
that fix changes COMPLETION_INITIALIZER_ONSTACK() from rvalue to lvalue,
and usage as above will report error:

	drivers/acpi/nfit/core.c: In function 'acpi_nfit_flush_probe':
	include/linux/completion.h:77:3: error: value computed is not used [-Werror=unused-value]
	  (*({ init_completion(&work); &work; }))

This patch fixes this by replacing COMPLETION_INITIALIZER_ONSTACK() with
init_completion() in acpi_nfit_flush_probe(), which does the same
initialization without any other problem.

Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
---
 drivers/acpi/nfit/core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 19182d091587..1893e416e7c0 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -2884,7 +2884,7 @@ static int acpi_nfit_flush_probe(struct nvdimm_bus_descriptor *nd_desc)
 	 * need to be interruptible while waiting.
 	 */
 	INIT_WORK_ONSTACK(&flush.work, flush_probe);
-	COMPLETION_INITIALIZER_ONSTACK(flush.cmp);
+	init_completion(&flush.cmp);
 	queue_work(nfit_wq, &flush.work);
 	mutex_unlock(&acpi_desc->init_mutex);
 
-- 
2.14.1

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH v2 1/2] nfit: Fix the abuse of COMPLETION_INITIALIZER_ONSTACK()

[Boqun Feng]

[-- Attachment #1: Type: text/plain, Size: 1921 bytes --]

On Thu, Aug 24, 2017 at 10:22:36PM +0800, Boqun Feng wrote:
> COMPLETION_INITIALIZER_ONSTACK() is supposed to used as an initializer,
> in other words, it should only be used in assignment expressions or
> compound literals. So the usage in drivers/acpi/nfit/core.c:
> 
> 	COMPLETION_INITIALIZER_ONSTACK(flush.cmp);
> 
> , is inappropriate.
> 
> Besides, this usage could also break compilations for another fix to
> reduce stack sizes caused by COMPLETION_INITIALIZER_ONSTACK(), because
> that fix changes COMPLETION_INITIALIZER_ONSTACK() from rvalue to lvalue,
> and usage as above will report error:
> 
> 	drivers/acpi/nfit/core.c: In function 'acpi_nfit_flush_probe':
> 	include/linux/completion.h:77:3: error: value computed is not used [-Werror=unused-value]
> 	  (*({ init_completion(&work); &work; }))
> 
> This patch fixes this by replacing COMPLETION_INITIALIZER_ONSTACK() with
> init_completion() in acpi_nfit_flush_probe(), which does the same
> initialization without any other problem.
> 
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>
> ---

Sorry, forget to metion:

v1 --> v2:
	Improve the commit log, based on Dan, Thomas and Arnd's
	comments.

Only V2 of this patch #1 is updated. 

Regards,
Boqun

>  drivers/acpi/nfit/core.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 19182d091587..1893e416e7c0 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -2884,7 +2884,7 @@ static int acpi_nfit_flush_probe(struct nvdimm_bus_descriptor *nd_desc)
>  	 * need to be interruptible while waiting.
>  	 */
>  	INIT_WORK_ONSTACK(&flush.work, flush_probe);
> -	COMPLETION_INITIALIZER_ONSTACK(flush.cmp);
> +	init_completion(&flush.cmp);
>  	queue_work(nfit_wq, &flush.work);
>  	mutex_unlock(&acpi_desc->init_mutex);
>  
> -- 
> 2.14.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/2] nfit: Fix the abuse of COMPLETION_INITIALIZER_ONSTACK()

[Dan Williams]

On Thu, Aug 24, 2017 at 7:22 AM, Boqun Feng <boqun.feng@gmail.com> wrote:
> COMPLETION_INITIALIZER_ONSTACK() is supposed to used as an initializer,
> in other words, it should only be used in assignment expressions or
> compound literals. So the usage in drivers/acpi/nfit/core.c:
>
>         COMPLETION_INITIALIZER_ONSTACK(flush.cmp);
>
> , is inappropriate.
>
> Besides, this usage could also break compilations for another fix to
> reduce stack sizes caused by COMPLETION_INITIALIZER_ONSTACK(), because
> that fix changes COMPLETION_INITIALIZER_ONSTACK() from rvalue to lvalue,
> and usage as above will report error:
>
>         drivers/acpi/nfit/core.c: In function 'acpi_nfit_flush_probe':
>         include/linux/completion.h:77:3: error: value computed is not used [-Werror=unused-value]
>           (*({ init_completion(&work); &work; }))
>
> This patch fixes this by replacing COMPLETION_INITIALIZER_ONSTACK() with
> init_completion() in acpi_nfit_flush_probe(), which does the same
> initialization without any other problem.
>
> Signed-off-by: Boqun Feng <boqun.feng@gmail.com>

Acked-by: Dan Williams <dan.j.williams@intel.com>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>