* [PATCH 0/2] completion: Reduce stack usage caused by COMPLETION_INITIALIZER_ONSTACK() @ 2017-08-23 15:25 ` Boqun Feng 0 siblings, 0 replies; 26+ messages in thread From: Boqun Feng @ 2017-08-23 15:25 UTC (permalink / raw) To: linux-kernel, linux-mm Cc: Peter Zijlstra, Ingo Molnar, Thomas Gleixner, walken, Byungchul Park, Arnd Bergmann, Andrew Morton, willy, Nicholas Piggin, kernel-team 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 0/2] completion: Reduce stack usage caused by COMPLETION_INITIALIZER_ONSTACK() @ 2017-08-23 15:25 ` Boqun Feng 0 siblings, 0 replies; 26+ messages in thread From: Boqun Feng @ 2017-08-23 15:25 UTC (permalink / raw) To: linux-kernel, linux-mm Cc: Peter Zijlstra, Ingo Molnar, Thomas Gleixner, walken, Byungchul Park, Arnd Bergmann, Andrew Morton, willy, Nicholas Piggin, kernel-team 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> ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH 1/2] nfit: Use init_completion() in acpi_nfit_flush_probe() 2017-08-23 15:25 ` Boqun Feng @ 2017-08-23 15:25 ` Boqun Feng -1 siblings, 0 replies; 26+ messages in thread From: Boqun Feng @ 2017-08-23 15:25 UTC (permalink / raw) To: linux-kernel, linux-mm Cc: Peter Zijlstra, Ingo Molnar, Thomas Gleixner, walken, Byungchul Park, Arnd Bergmann, Andrew Morton, willy, Nicholas Piggin, kernel-team, Boqun Feng, Dan Williams, Rafael J. Wysocki, Len Brown, linux-nvdimm, linux-acpi 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> ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 1/2] nfit: Use init_completion() in acpi_nfit_flush_probe() @ 2017-08-23 15:25 ` Boqun Feng 0 siblings, 0 replies; 26+ messages in thread From: Boqun Feng @ 2017-08-23 15:25 UTC (permalink / raw) To: linux-kernel, linux-mm Cc: Peter Zijlstra, Ingo Molnar, Thomas Gleixner, walken, Byungchul Park, Arnd Bergmann, Andrew Morton, willy, Nicholas Piggin, kernel-team, Boqun Feng, Dan Williams, Rafael J. Wysocki, Len Brown, linux-nvdimm, linux-acpi 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] nfit: Use init_completion() in acpi_nfit_flush_probe() 2017-08-23 15:25 ` Boqun Feng (?) @ 2017-08-23 22:08 ` Dan Williams -1 siblings, 0 replies; 26+ messages in thread From: Dan Williams @ 2017-08-23 22:08 UTC (permalink / raw) To: Boqun Feng Cc: Andrew Morton, Arnd Bergmann, kernel-team, Peter Zijlstra, linux-nvdimm, Rafael J. Wysocki, linux-kernel, Matthew Wilcox, Byungchul Park, Linux MM, Nicholas Piggin, Thomas Gleixner, walken, Linux ACPI, Ingo Molnar, Len Brown 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> _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] nfit: Use init_completion() in acpi_nfit_flush_probe() @ 2017-08-23 22:08 ` Dan Williams 0 siblings, 0 replies; 26+ messages in thread From: Dan Williams @ 2017-08-23 22:08 UTC (permalink / raw) To: Boqun Feng Cc: linux-kernel, Linux MM, Peter Zijlstra, Ingo Molnar, Thomas Gleixner, walken, Byungchul Park, Arnd Bergmann, Andrew Morton, Matthew Wilcox, Nicholas Piggin, kernel-team, Rafael J. Wysocki, Len Brown, linux-nvdimm, Linux ACPI 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> ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] nfit: Use init_completion() in acpi_nfit_flush_probe() @ 2017-08-23 22:08 ` Dan Williams 0 siblings, 0 replies; 26+ messages in thread From: Dan Williams @ 2017-08-23 22:08 UTC (permalink / raw) To: Boqun Feng Cc: linux-kernel, Linux MM, Peter Zijlstra, Ingo Molnar, Thomas Gleixner, walken, Byungchul Park, Arnd Bergmann, Andrew Morton, Matthew Wilcox, Nicholas Piggin, kernel-team, Rafael J. Wysocki, Len Brown, linux-nvdimm, Linux ACPI 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> ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] nfit: Use init_completion() in acpi_nfit_flush_probe() 2017-08-23 15:25 ` Boqun Feng @ 2017-08-24 13:07 ` Thomas Gleixner -1 siblings, 0 replies; 26+ messages in thread From: Thomas Gleixner @ 2017-08-24 13:07 UTC (permalink / raw) To: Boqun Feng Cc: linux-kernel, linux-mm, Peter Zijlstra, Ingo Molnar, walken, Byungchul Park, Arnd Bergmann, Andrew Morton, willy, Nicholas Piggin, kernel-team, Dan Williams, Rafael J. Wysocki, Len Brown, linux-nvdimm, linux-acpi 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> ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] nfit: Use init_completion() in acpi_nfit_flush_probe() @ 2017-08-24 13:07 ` Thomas Gleixner 0 siblings, 0 replies; 26+ messages in thread From: Thomas Gleixner @ 2017-08-24 13:07 UTC (permalink / raw) To: Boqun Feng Cc: linux-kernel, linux-mm, Peter Zijlstra, Ingo Molnar, walken, Byungchul Park, Arnd Bergmann, Andrew Morton, willy, Nicholas Piggin, kernel-team, Dan Williams, Rafael J. Wysocki, Len Brown, linux-nvdimm, linux-acpi 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 > > ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] nfit: Use init_completion() in acpi_nfit_flush_probe() 2017-08-24 13:07 ` Thomas Gleixner (?) @ 2017-08-24 13:28 ` Boqun Feng 2017-08-24 13:46 ` Arnd Bergmann -1 siblings, 1 reply; 26+ messages in thread From: Boqun Feng @ 2017-08-24 13:28 UTC (permalink / raw) To: Thomas Gleixner Cc: linux-kernel, linux-mm, Peter Zijlstra, Ingo Molnar, walken, Byungchul Park, Arnd Bergmann, Andrew Morton, willy, Nicholas Piggin, kernel-team, Dan Williams, Rafael J. Wysocki, Len Brown, linux-nvdimm, linux-acpi [-- 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 --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] nfit: Use init_completion() in acpi_nfit_flush_probe() 2017-08-24 13:28 ` Boqun Feng 2017-08-24 13:46 ` Arnd Bergmann @ 2017-08-24 13:46 ` Arnd Bergmann 0 siblings, 0 replies; 26+ messages in thread From: Arnd Bergmann @ 2017-08-24 13:46 UTC (permalink / raw) To: Boqun Feng Cc: Andrew Morton, kernel-team, Peter Zijlstra, Rafael J. Wysocki, linux-nvdimm, Linux Kernel Mailing List, willy, Byungchul Park, Linux-MM, Nicholas Piggin, Thomas Gleixner, Michel Lespinasse, ACPI Devel Maling List, Ingo Molnar, Len Brown 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 _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] nfit: Use init_completion() in acpi_nfit_flush_probe() @ 2017-08-24 13:46 ` Arnd Bergmann 0 siblings, 0 replies; 26+ messages in thread From: Arnd Bergmann @ 2017-08-24 13:46 UTC (permalink / raw) To: Boqun Feng Cc: Thomas Gleixner, Linux Kernel Mailing List, Linux-MM, Peter Zijlstra, Ingo Molnar, Michel Lespinasse, Byungchul Park, Andrew Morton, willy, Nicholas Piggin, kernel-team, Dan Williams, Rafael J. Wysocki, Len Brown, linux-nvdimm, ACPI Devel Maling List 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> ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 1/2] nfit: Use init_completion() in acpi_nfit_flush_probe() @ 2017-08-24 13:46 ` Arnd Bergmann 0 siblings, 0 replies; 26+ messages in thread From: Arnd Bergmann @ 2017-08-24 13:46 UTC (permalink / raw) To: Boqun Feng Cc: Thomas Gleixner, Linux Kernel Mailing List, Linux-MM, Peter Zijlstra, Ingo Molnar, Michel Lespinasse, Byungchul Park, Andrew Morton, willy, Nicholas Piggin, kernel-team, Dan Williams, Rafael J. Wysocki, Len Brown, linux-nvdimm, ACPI Devel Maling List 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 ^ permalink raw reply [flat|nested] 26+ messages in thread
* [PATCH v2 1/2] nfit: Fix the abuse of COMPLETION_INITIALIZER_ONSTACK() @ 2017-08-24 14:22 ` Boqun Feng 0 siblings, 0 replies; 26+ messages in thread From: Boqun Feng @ 2017-08-24 14:22 UTC (permalink / raw) To: linux-kernel, linux-mm Cc: Peter Zijlstra, Ingo Molnar, Thomas Gleixner, walken, Byungchul Park, Arnd Bergmann, Andrew Morton, willy, Nicholas Piggin, kernel-team, Boqun Feng, Dan Williams, Rafael J. Wysocki, Len Brown, linux-nvdimm, linux-acpi 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> ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 1/2] nfit: Fix the abuse of COMPLETION_INITIALIZER_ONSTACK() @ 2017-08-24 14:22 ` Boqun Feng 0 siblings, 0 replies; 26+ messages in thread From: Boqun Feng @ 2017-08-24 14:22 UTC (permalink / raw) To: linux-kernel, linux-mm Cc: Peter Zijlstra, Ingo Molnar, Thomas Gleixner, walken, Byungchul Park, Arnd Bergmann, Andrew Morton, willy, Nicholas Piggin, kernel-team, Boqun Feng, Dan Williams, Rafael J. Wysocki, Len Brown, linux-nvdimm, linux-acpi 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH v2 1/2] nfit: Fix the abuse of COMPLETION_INITIALIZER_ONSTACK() @ 2017-08-24 14:22 ` Boqun Feng 0 siblings, 0 replies; 26+ messages in thread From: Boqun Feng @ 2017-08-24 14:22 UTC (permalink / raw) To: linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-mm-Bw31MaZKKs3YtjvyW6yDsg Cc: Andrew Morton, Arnd Bergmann, linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, Peter Zijlstra, Rafael J. Wysocki, Boqun Feng, linux-acpi-u79uwXL29TY76Z2rM5mHXA, willy-wEGCiKHe2LqWVfeAwA7xHQ, Byungchul Park, kernel-team-Hm3cg6mZ9cc, Nicholas Piggin, Thomas Gleixner, walken-hpIqsD4AKlfQT0dZR+AlfA, Ingo Molnar, Len Brown 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-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> --- 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/2] nfit: Fix the abuse of COMPLETION_INITIALIZER_ONSTACK() 2017-08-24 14:22 ` Boqun Feng (?) (?) @ 2017-08-25 0:18 ` Boqun Feng -1 siblings, 0 replies; 26+ messages in thread From: Boqun Feng @ 2017-08-25 0:18 UTC (permalink / raw) To: linux-kernel, linux-mm Cc: Peter Zijlstra, Ingo Molnar, Thomas Gleixner, walken, Byungchul Park, Arnd Bergmann, Andrew Morton, willy, Nicholas Piggin, kernel-team, Dan Williams, Rafael J. Wysocki, Len Brown, linux-nvdimm, linux-acpi [-- 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 --] ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/2] nfit: Fix the abuse of COMPLETION_INITIALIZER_ONSTACK() 2017-08-24 14:22 ` Boqun Feng (?) @ 2017-08-25 0:36 ` Dan Williams -1 siblings, 0 replies; 26+ messages in thread From: Dan Williams @ 2017-08-25 0:36 UTC (permalink / raw) To: Boqun Feng Cc: Andrew Morton, Arnd Bergmann, kernel-team, Peter Zijlstra, linux-nvdimm, Rafael J. Wysocki, linux-kernel, Matthew Wilcox, Byungchul Park, Linux MM, Nicholas Piggin, Thomas Gleixner, Michel Lespinasse, Linux ACPI, Ingo Molnar, Len Brown 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> _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/2] nfit: Fix the abuse of COMPLETION_INITIALIZER_ONSTACK() @ 2017-08-25 0:36 ` Dan Williams 0 siblings, 0 replies; 26+ messages in thread From: Dan Williams @ 2017-08-25 0:36 UTC (permalink / raw) To: Boqun Feng Cc: linux-kernel, Linux MM, Peter Zijlstra, Ingo Molnar, Thomas Gleixner, Michel Lespinasse, Byungchul Park, Arnd Bergmann, Andrew Morton, Matthew Wilcox, Nicholas Piggin, kernel-team, Rafael J. Wysocki, Len Brown, linux-nvdimm, Linux ACPI 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> ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH v2 1/2] nfit: Fix the abuse of COMPLETION_INITIALIZER_ONSTACK() @ 2017-08-25 0:36 ` Dan Williams 0 siblings, 0 replies; 26+ messages in thread From: Dan Williams @ 2017-08-25 0:36 UTC (permalink / raw) To: Boqun Feng Cc: linux-kernel, Linux MM, Peter Zijlstra, Ingo Molnar, Thomas Gleixner, Michel Lespinasse, Byungchul Park, Arnd Bergmann, Andrew Morton, Matthew Wilcox, Nicholas Piggin, kernel-team, Rafael J. Wysocki, Len Brown, linux-nvdimm, Linux ACPI 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> ^ permalink raw reply [flat|nested] 26+ messages in thread
* [tip:locking/core] acpi/nfit: Fix COMPLETION_INITIALIZER_ONSTACK() abuse 2017-08-24 14:22 ` Boqun Feng ` (3 preceding siblings ...) (?) @ 2017-08-29 14:24 ` tip-bot for Boqun Feng -1 siblings, 0 replies; 26+ messages in thread From: tip-bot for Boqun Feng @ 2017-08-29 14:24 UTC (permalink / raw) To: linux-tip-commits Cc: boqun.feng, torvalds, arnd, dan.j.williams, linux-kernel, akpm, hpa, rjw, lenb, peterz, mingo, npiggin, byungchul.park, tglx Commit-ID: 1c322ac06d9af7ea259098ae5dc977855207d335 Gitweb: http://git.kernel.org/tip/1c322ac06d9af7ea259098ae5dc977855207d335 Author: Boqun Feng <boqun.feng@gmail.com> AuthorDate: Thu, 24 Aug 2017 22:22:36 +0800 Committer: Ingo Molnar <mingo@kernel.org> CommitDate: Tue, 29 Aug 2017 15:14:38 +0200 acpi/nfit: Fix COMPLETION_INITIALIZER_ONSTACK() abuse COMPLETION_INITIALIZER_ONSTACK() is supposed to be 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 the build for another fix that reduces stack sizes caused by COMPLETION_INITIALIZER_ONSTACK(), because that fix changes COMPLETION_INITIALIZER_ONSTACK() from rvalue to lvalue, and usage as above will report the following 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 problems. Signed-off-by: Boqun Feng <boqun.feng@gmail.com> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Acked-by: Dan Williams <dan.j.williams@intel.com> Acked-by: Arnd Bergmann <arnd@arndb.de> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Byungchul Park <byungchul.park@lge.com> Cc: Len Brown <lenb@kernel.org> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Nicholas Piggin <npiggin@gmail.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: walken@google.com Cc: willy@infradead.org Link: http://lkml.kernel.org/r/20170824142239.15178-1-boqun.feng@gmail.com Signed-off-by: Ingo Molnar <mingo@kernel.org> --- 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 19182d0..1893e41 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); ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 2/2] completion: Avoid unnecessary stack allocation for COMPLETION_INITIALIZER_ONSTACK() 2017-08-23 15:25 ` Boqun Feng @ 2017-08-23 15:25 ` Boqun Feng -1 siblings, 0 replies; 26+ messages in thread From: Boqun Feng @ 2017-08-23 15:25 UTC (permalink / raw) To: linux-kernel, linux-mm Cc: Peter Zijlstra, Ingo Molnar, Thomas Gleixner, walken, Byungchul Park, Arnd Bergmann, Andrew Morton, willy, Nicholas Piggin, kernel-team, 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [PATCH 2/2] completion: Avoid unnecessary stack allocation for COMPLETION_INITIALIZER_ONSTACK() @ 2017-08-23 15:25 ` Boqun Feng 0 siblings, 0 replies; 26+ messages in thread From: Boqun Feng @ 2017-08-23 15:25 UTC (permalink / raw) To: linux-kernel, linux-mm Cc: Peter Zijlstra, Ingo Molnar, Thomas Gleixner, walken, Byungchul Park, Arnd Bergmann, Andrew Morton, willy, Nicholas Piggin, kernel-team, 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> ^ permalink raw reply related [flat|nested] 26+ messages in thread
* [tip:locking/core] sched/completion: Avoid unnecessary stack allocation for COMPLETION_INITIALIZER_ONSTACK() 2017-08-23 15:25 ` Boqun Feng (?) @ 2017-08-29 14:24 ` tip-bot for Boqun Feng -1 siblings, 0 replies; 26+ messages in thread From: tip-bot for Boqun Feng @ 2017-08-29 14:24 UTC (permalink / raw) To: linux-tip-commits Cc: boqun.feng, torvalds, arnd, peterz, byungchul.park, akpm, npiggin, linux-kernel, hpa, mingo, tglx Commit-ID: ec81048cc340bb03334e6ca62661ecc0a684897a Gitweb: http://git.kernel.org/tip/ec81048cc340bb03334e6ca62661ecc0a684897a Author: Boqun Feng <boqun.feng@gmail.com> AuthorDate: Wed, 23 Aug 2017 23:25:38 +0800 Committer: Ingo Molnar <mingo@kernel.org> CommitDate: Tue, 29 Aug 2017 15:14:38 +0200 sched/completion: Avoid unnecessary stack allocation for COMPLETION_INITIALIZER_ONSTACK() 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> Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org> Acked-by: Arnd Bergmann <arnd@arndb.de> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Byungchul Park <byungchul.park@lge.com> Cc: Linus Torvalds <torvalds@linux-foundation.org> Cc: Nicholas Piggin <npiggin@gmail.com> Cc: Peter Zijlstra <peterz@infradead.org> Cc: Thomas Gleixner <tglx@linutronix.de> Cc: walken@google.com Cc: willy@infradead.org Link: http://lkml.kernel.org/r/20170823152542.5150-3-boqun.feng@gmail.com Signed-off-by: Ingo Molnar <mingo@kernel.org> --- 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 791f053..cae5400 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 ^ permalink raw reply related [flat|nested] 26+ messages in thread
* Re: [PATCH 0/2] completion: Reduce stack usage caused by COMPLETION_INITIALIZER_ONSTACK() 2017-08-23 15:25 ` Boqun Feng @ 2017-08-23 15:33 ` Arnd Bergmann -1 siblings, 0 replies; 26+ messages in thread From: Arnd Bergmann @ 2017-08-23 15:33 UTC (permalink / raw) To: Boqun Feng Cc: Linux Kernel Mailing List, Linux-MM, Peter Zijlstra, Ingo Molnar, Thomas Gleixner, Michel Lespinasse, Byungchul Park, Andrew Morton, willy, Nicholas Piggin, kernel-team 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> ^ permalink raw reply [flat|nested] 26+ messages in thread
* Re: [PATCH 0/2] completion: Reduce stack usage caused by COMPLETION_INITIALIZER_ONSTACK() @ 2017-08-23 15:33 ` Arnd Bergmann 0 siblings, 0 replies; 26+ messages in thread From: Arnd Bergmann @ 2017-08-23 15:33 UTC (permalink / raw) To: Boqun Feng Cc: Linux Kernel Mailing List, Linux-MM, Peter Zijlstra, Ingo Molnar, Thomas Gleixner, Michel Lespinasse, Byungchul Park, Andrew Morton, willy, Nicholas Piggin, kernel-team 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> ^ permalink raw reply [flat|nested] 26+ messages in thread
end of thread, other threads:[~2017-08-29 14:30 UTC | newest] Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-08-23 15:25 [PATCH 0/2] completion: Reduce stack usage caused by COMPLETION_INITIALIZER_ONSTACK() Boqun Feng 2017-08-23 15:25 ` Boqun Feng 2017-08-23 15:25 ` [PATCH 1/2] nfit: Use init_completion() in acpi_nfit_flush_probe() Boqun Feng 2017-08-23 15:25 ` Boqun Feng 2017-08-23 22:08 ` Dan Williams 2017-08-23 22:08 ` Dan Williams 2017-08-23 22:08 ` Dan Williams 2017-08-24 13:07 ` Thomas Gleixner 2017-08-24 13:07 ` Thomas Gleixner 2017-08-24 13:28 ` Boqun Feng 2017-08-24 13:46 ` Arnd Bergmann 2017-08-24 13:46 ` Arnd Bergmann 2017-08-24 13:46 ` Arnd Bergmann 2017-08-24 14:22 ` [PATCH v2 1/2] nfit: Fix the abuse of COMPLETION_INITIALIZER_ONSTACK() Boqun Feng 2017-08-24 14:22 ` Boqun Feng 2017-08-24 14:22 ` Boqun Feng 2017-08-25 0:18 ` Boqun Feng 2017-08-25 0:36 ` Dan Williams 2017-08-25 0:36 ` Dan Williams 2017-08-25 0:36 ` Dan Williams 2017-08-29 14:24 ` [tip:locking/core] acpi/nfit: Fix COMPLETION_INITIALIZER_ONSTACK() abuse tip-bot for Boqun Feng 2017-08-23 15:25 ` [PATCH 2/2] completion: Avoid unnecessary stack allocation for COMPLETION_INITIALIZER_ONSTACK() Boqun Feng 2017-08-23 15:25 ` Boqun Feng 2017-08-29 14:24 ` [tip:locking/core] sched/completion: " tip-bot for Boqun Feng 2017-08-23 15:33 ` [PATCH 0/2] completion: Reduce stack usage caused by COMPLETION_INITIALIZER_ONSTACK() Arnd Bergmann 2017-08-23 15:33 ` Arnd Bergmann
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.