Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1710734 > unrolled thread
| Started by | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| First post | 2017-08-14 11:00 +0200 |
| Last post | 2017-08-20 05:20 +0200 |
| Articles | 6 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v8 09/14] lockdep: Apply crossrelease to completions Arnd Bergmann <arnd@arndb.de> - 2017-08-14 11:00 +0200
Re: [PATCH v8 09/14] lockdep: Apply crossrelease to completions Boqun Feng <boqun.feng@gmail.com> - 2017-08-19 01:50 +0200
Re: [PATCH v8 09/14] lockdep: Apply crossrelease to completions Arnd Bergmann <arnd@arndb.de> - 2017-08-19 15:00 +0200
Re: [PATCH v8 09/14] lockdep: Apply crossrelease to completions Arnd Bergmann <arnd@arndb.de> - 2017-08-19 15:40 +0200
Re: [PATCH v8 09/14] lockdep: Apply crossrelease to completions Boqun Feng <boqun.feng@gmail.com> - 2017-08-23 16:50 +0200
Re: [PATCH v8 09/14] lockdep: Apply crossrelease to completions Boqun Feng <boqun.feng@gmail.com> - 2017-08-20 05:20 +0200
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2017-08-14 11:00 +0200 |
| Subject | Re: [PATCH v8 09/14] lockdep: Apply crossrelease to completions |
| Message-ID | <uejo5-1Xt-11@gated-at.bofh.it> |
On Mon, Aug 7, 2017 at 9:12 AM, Byungchul Park <byungchul.park@lge.com> wrote:
> Although wait_for_completion() and its family can cause deadlock, the
> lock correctness validator could not be applied to them until now,
> because things like complete() are usually called in a different context
> from the waiting context, which violates lockdep's assumption.
>
> Thanks to CONFIG_LOCKDEP_CROSSRELEASE, we can now apply the lockdep
> detector to those completion operations. Applied it.
>
> Signed-off-by: Byungchul Park <byungchul.park@lge.com>
This patch introduced a significant growth in kernel stack usage for a small
set of functions. I see two new warnings for functions that get tipped over the
1024 or 2048 byte frame size limit in linux-next (with a few other patches
applied):
Before:
drivers/md/dm-integrity.c: In function 'write_journal':
drivers/md/dm-integrity.c:827:1: error: the frame size of 504 bytes is
larger than xxx bytes [-Werror=frame-larger-than=]
drivers/mmc/core/mmc_test.c: In function 'mmc_test_area_io_seq':
drivers/mmc/core/mmc_test.c:1491:1: error: the frame size of 680 bytes
is larger than 104 bytes [-Werror=frame-larger-than=]
After:
drivers/md/dm-integrity.c: In function 'write_journal':
drivers/md/dm-integrity.c:827:1: error: the frame size of 1280 bytes
is larger than 1024 bytes [-Werror=frame-larger-than=]
drivers/mmc/core/mmc_test.c: In function 'mmc_test_area_io_seq':
drivers/mmc/core/mmc_test.c:1491:1: error: the frame size of 1072
bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
I have not checked in detail why this happens, but I'm guessing that
there is an overall increase in stack usage with
CONFIG_LOCKDEP_COMPLETE in functions using completions,
and I think it would be good to try to come up with a version that doesn't
add as much.
Arnd
[toc] | [next] | [standalone]
| From | Boqun Feng <boqun.feng@gmail.com> |
|---|---|
| Date | 2017-08-19 01:50 +0200 |
| Message-ID | <ufZbz-25E-1@gated-at.bofh.it> |
| In reply to | #1710734 |
Hi Arnd,
On Mon, Aug 14, 2017 at 10:50:24AM +0200, Arnd Bergmann wrote:
> On Mon, Aug 7, 2017 at 9:12 AM, Byungchul Park <byungchul.park@lge.com> wrote:
> > Although wait_for_completion() and its family can cause deadlock, the
> > lock correctness validator could not be applied to them until now,
> > because things like complete() are usually called in a different context
> > from the waiting context, which violates lockdep's assumption.
> >
> > Thanks to CONFIG_LOCKDEP_CROSSRELEASE, we can now apply the lockdep
> > detector to those completion operations. Applied it.
> >
> > Signed-off-by: Byungchul Park <byungchul.park@lge.com>
>
> This patch introduced a significant growth in kernel stack usage for a small
> set of functions. I see two new warnings for functions that get tipped over the
> 1024 or 2048 byte frame size limit in linux-next (with a few other patches
> applied):
>
> Before:
>
> drivers/md/dm-integrity.c: In function 'write_journal':
> drivers/md/dm-integrity.c:827:1: error: the frame size of 504 bytes is
> larger than xxx bytes [-Werror=frame-larger-than=]
> drivers/mmc/core/mmc_test.c: In function 'mmc_test_area_io_seq':
> drivers/mmc/core/mmc_test.c:1491:1: error: the frame size of 680 bytes
> is larger than 104 bytes [-Werror=frame-larger-than=]
>
> After:
>
> drivers/md/dm-integrity.c: In function 'write_journal':
> drivers/md/dm-integrity.c:827:1: error: the frame size of 1280 bytes
> is larger than 1024 bytes [-Werror=frame-larger-than=]
> drivers/mmc/core/mmc_test.c: In function 'mmc_test_area_io_seq':
> drivers/mmc/core/mmc_test.c:1491:1: error: the frame size of 1072
> bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
>
> I have not checked in detail why this happens, but I'm guessing that
> there is an overall increase in stack usage with
> CONFIG_LOCKDEP_COMPLETE in functions using completions,
> and I think it would be good to try to come up with a version that doesn't
> add as much.
>
So I have been staring at this for a while, and below is what I found:
(BTW, Arnd, may I know your compiler version? Mine is 7.1.1)
In write_journal(), I can see the code generated like this on x86:
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)
Those two "rep movsq"s are very suspicious, because
COMPLETION_INITIALIZER_ONSTACK() should initialize the data in-place,
rather than move it to some temporary variable and copy it back.
I tried to reduce the size of completion struct, and the "rep movsq" did
go away, however it seemed the compiler still allocated the memory for
the temporary variables on the stack, because whenever I
increased/decreased the size of completion, the stack size of
write_journal() got increased/decreased *7* times, but there are only
3 journal_completion structures in write_journal(). So the *4* callsites
of COMPLETION_INITIALIZER_ONSTACK() looked very suspicous.
So I come up with the following patch, trying to teach the compiler not
to do the unnecessary allocation, could you give it a try?
Besides, I could also observe the stack size reduction of
write_journal() even for !LOCKDEP kernel.
-------------------------->8
Subject: [PATCH] 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".
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
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2017-08-19 15:00 +0200 |
| Message-ID | <ugbw6-1pj-9@gated-at.bofh.it> |
| In reply to | #1715510 |
On Sat, Aug 19, 2017 at 1:43 AM, Boqun Feng <boqun.feng@gmail.com> wrote:
> Hi Arnd,
>
> On Mon, Aug 14, 2017 at 10:50:24AM +0200, Arnd Bergmann wrote:
>> On Mon, Aug 7, 2017 at 9:12 AM, Byungchul Park <byungchul.park@lge.com> wrote:
>> > Although wait_for_completion() and its family can cause deadlock, the
>> > lock correctness validator could not be applied to them until now,
>> > because things like complete() are usually called in a different context
>> > from the waiting context, which violates lockdep's assumption.
>> >
>> > Thanks to CONFIG_LOCKDEP_CROSSRELEASE, we can now apply the lockdep
>> > detector to those completion operations. Applied it.
>> >
>> > Signed-off-by: Byungchul Park <byungchul.park@lge.com>
>>
>> This patch introduced a significant growth in kernel stack usage for a small
>> set of functions. I see two new warnings for functions that get tipped over the
>> 1024 or 2048 byte frame size limit in linux-next (with a few other patches
>> applied):
>>
>> Before:
>>
>> drivers/md/dm-integrity.c: In function 'write_journal':
>> drivers/md/dm-integrity.c:827:1: error: the frame size of 504 bytes is
>> larger than xxx bytes [-Werror=frame-larger-than=]
>> drivers/mmc/core/mmc_test.c: In function 'mmc_test_area_io_seq':
>> drivers/mmc/core/mmc_test.c:1491:1: error: the frame size of 680 bytes
>> is larger than 104 bytes [-Werror=frame-larger-than=]
>>
>> After:
>>
>> drivers/md/dm-integrity.c: In function 'write_journal':
>> drivers/md/dm-integrity.c:827:1: error: the frame size of 1280 bytes
>> is larger than 1024 bytes [-Werror=frame-larger-than=]
>> drivers/mmc/core/mmc_test.c: In function 'mmc_test_area_io_seq':
>> drivers/mmc/core/mmc_test.c:1491:1: error: the frame size of 1072
>> bytes is larger than 1024 bytes [-Werror=frame-larger-than=]
>>
>> I have not checked in detail why this happens, but I'm guessing that
>> there is an overall increase in stack usage with
>> CONFIG_LOCKDEP_COMPLETE in functions using completions,
>> and I think it would be good to try to come up with a version that doesn't
>> add as much.
>>
>
> So I have been staring at this for a while, and below is what I found:
>
> (BTW, Arnd, may I know your compiler version? Mine is 7.1.1)
That is what I used as well, on x86, arm32 and arm64.
> In write_journal(), I can see the code generated like this on x86:
>
> 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)
>
> Those two "rep movsq"s are very suspicious, because
> COMPLETION_INITIALIZER_ONSTACK() should initialize the data in-place,
> rather than move it to some temporary variable and copy it back.
Right. I've seen this behavior before when using c99 compound
literals, but I was surprised to see it here.
I also submitted a patch for the one driver that turned up a new
warning because of this behavior:
https://www.spinics.net/lists/raid/msg58766.html
In case of the mmc driver, the behavior was as expected, it was
just a little too large and I sent the obvious workaround for it
https://patchwork.kernel.org/patch/9902063/
> I tried to reduce the size of completion struct, and the "rep movsq" did
> go away, however it seemed the compiler still allocated the memory for
> the temporary variables on the stack, because whenever I
> increased/decreased the size of completion, the stack size of
> write_journal() got increased/decreased *7* times, but there are only
> 3 journal_completion structures in write_journal(). So the *4* callsites
> of COMPLETION_INITIALIZER_ONSTACK() looked very suspicous.
>
> So I come up with the following patch, trying to teach the compiler not
> to do the unnecessary allocation, could you give it a try?
>
> Besides, I could also observe the stack size reduction of
> write_journal() even for !LOCKDEP kernel.
Ok.
> -------------------
> 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
Nice hack. Any idea why that's different to the compiler?
I've applied that one to my test tree now, and reverted my own patch,
will let you know if anything else shows up. I think we probably want
to merge both patches to mainline.
Arnd
[toc] | [prev] | [next] | [standalone]
| From | Arnd Bergmann <arnd@arndb.de> |
|---|---|
| Date | 2017-08-19 15:40 +0200 |
| Message-ID | <ugc8O-1SQ-5@gated-at.bofh.it> |
| In reply to | #1715664 |
On Sat, Aug 19, 2017 at 2:51 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>> --- 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
>
> Nice hack. Any idea why that's different to the compiler?
>
> I've applied that one to my test tree now, and reverted my own patch,
> will let you know if anything else shows up. I think we probably want
> to merge both patches to mainline.
There is apparently one user of COMPLETION_INITIALIZER_ONSTACK
that causes a regression with the patch above:
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; }))
It would be trivial to convert to init_completion(), which seems to be
what was intended there.
Arnd
[toc] | [prev] | [next] | [standalone]
| From | Boqun Feng <boqun.feng@gmail.com> |
|---|---|
| Date | 2017-08-23 16:50 +0200 |
| Message-ID | <uhF8K-1Mw-17@gated-at.bofh.it> |
| In reply to | #1715670 |
[Multipart message — attachments visible in raw view] — view raw
On Sat, Aug 19, 2017 at 03:34:01PM +0200, Arnd Bergmann wrote:
> On Sat, Aug 19, 2017 at 2:51 PM, Arnd Bergmann <arnd@arndb.de> wrote:
>
> >> --- 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
> >
> > Nice hack. Any idea why that's different to the compiler?
> >
So I find this link:
https://gcc.gnu.org/onlinedocs/gcc/Statement-Exprs.html
it says:
"In G++, the result value of a statement expression undergoes array and
function pointer decay, and is returned by value to the enclosing
expression. "
I think this is why the temporary variable is constructed(or at least
allocated). Lemme put this in my commit log.
> > I've applied that one to my test tree now, and reverted my own patch,
> > will let you know if anything else shows up. I think we probably want
> > to merge both patches to mainline.
>
> There is apparently one user of COMPLETION_INITIALIZER_ONSTACK
> that causes a regression with the patch above:
>
> 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; }))
>
> It would be trivial to convert to init_completion(), which seems to be
> what was intended there.
>
Thanks. Will send the conversion as a separate patch along with my
patch.
Regards,
Boqun
> Arnd
[toc] | [prev] | [next] | [standalone]
| From | Boqun Feng <boqun.feng@gmail.com> |
|---|---|
| Date | 2017-08-20 05:20 +0200 |
| Message-ID | <ugoWl-1tG-11@gated-at.bofh.it> |
| In reply to | #1715664 |
[Multipart message — attachments visible in raw view] — view raw
On Sat, Aug 19, 2017 at 02:51:17PM +0200, Arnd Bergmann wrote:
[...]
> > Those two "rep movsq"s are very suspicious, because
> > COMPLETION_INITIALIZER_ONSTACK() should initialize the data in-place,
> > rather than move it to some temporary variable and copy it back.
>
> Right. I've seen this behavior before when using c99 compound
> literals, but I was surprised to see it here.
>
> I also submitted a patch for the one driver that turned up a new
> warning because of this behavior:
>
> https://www.spinics.net/lists/raid/msg58766.html
>
This solution also came up into my mind but then I found there are
several callsites of COMPLETION_INITIALIZER_ONSTACK(), so I then tried
to find a way to fix the macro itself. But your patch looks good to me
;-)
> In case of the mmc driver, the behavior was as expected, it was
> just a little too large and I sent the obvious workaround for it
>
> https://patchwork.kernel.org/patch/9902063/
>
Yep.
> > I tried to reduce the size of completion struct, and the "rep movsq" did
> > go away, however it seemed the compiler still allocated the memory for
> > the temporary variables on the stack, because whenever I
> > increased/decreased the size of completion, the stack size of
> > write_journal() got increased/decreased *7* times, but there are only
> > 3 journal_completion structures in write_journal(). So the *4* callsites
> > of COMPLETION_INITIALIZER_ONSTACK() looked very suspicous.
> >
> > So I come up with the following patch, trying to teach the compiler not
> > to do the unnecessary allocation, could you give it a try?
> >
> > Besides, I could also observe the stack size reduction of
> > write_journal() even for !LOCKDEP kernel.
>
> Ok.
>
> > -------------------
> > 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
>
> Nice hack. Any idea why that's different to the compiler?
>
So *I think* the block {init_completion(&work); &work;} now will return
a pointer rather than a whole structure, and a pointer could fit in a
register, so the compiler won't bother to allocate the memory for it.
> I've applied that one to my test tree now, and reverted my own patch,
> will let you know if anything else shows up. I think we probably want
Thanks ;-)
> to merge both patches to mainline.
>
Agreed! Unless we want to remove COMPLETION_INITIALIZER_ONSTACK() for
some reason, then my patch is not needed.
Regards,
Boqun
> Arnd
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web