Re: [PATCH v8 09/14] lockdep: Apply crossrelease to completions

[Boqun Feng]

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