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@xxxxxxxx> > > Signed-off-by: Boqun Feng <boqun.feng@xxxxxxxxx> > > --- > > 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
Attachment:
signature.asc
Description: PGP signature
