Re: kunit stack usage, was: pmwg-ci report v5.5-rc4-147-gc62d43442481

[Brendan Higgins]

On Fri, Jan 10, 2020 at 12:05 AM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
> On Fri, Jan 10, 2020 at 7:27 AM Stephen Boyd <sboyd@xxxxxxxxxx> wrote:
> > Quoting Arnd Bergmann (2020-01-08 07:13:46)
> > > On Wed, Jan 8, 2020 at 3:41 PM Brendan Higgins <brendanhiggins@xxxxxxxxxx> wrote:
> > > > On Tue, Jan 7, 2020 at 4:37 AM Arnd Bergmann <arnd@xxxxxxxx> wrote:
> > > > test function, which allocates the object, and then calls the unit
> > > > test function with a reference to the object allocation; then we could
> > > > just reuse that allocation and we can avoid making a bunch of
> > > > piecemeal heap allocations.
> > > >
> > > > What do people think? Any other ideas?
> >
> > How about forcing inlining of kunit_do_assertion()? That may allow the
> > compiler to remove all the assertion structs and inline the arguments
> > from the struct to whatever functions the assertion functions call? It
> > may bloat the text size.
>
> I haven't tried it, but I'm fairly sure that would not reliably fix it. The
> problem is that the local 'struct kunit_assert' structure escapes to
> an extern function call it is passed to by reference. If we inline
> kunit_do_assertion(), nothing really changes in that regard as
> the compiler still has to construct and initialize that structure on
> the stack.
>
> However, the reverse would be possible. Turning
> KUNIT_BASE_BINARY_ASSERTION() into an extern
> function that takes all the arguments without passing a
> structure would solve it. I've prototyped this by changing
> KUNIT_BINARY_EQ_ASSERTION() and
> KUNIT_BINARY_NE_ASSERTION() like
>
> @@ -651,13 +649,19 @@ do {
>                                 \
>                                     fmt,                                       \
>                                     ##__VA_ARGS__)
>
> -#define KUNIT_BINARY_NE_ASSERTION(test, assert_type, left, right)             \
> +#define __KUNIT_BINARY_NE_ASSERTION(test, assert_type, left, right)           \
>         KUNIT_BINARY_NE_MSG_ASSERTION(test,                                    \
>                                       assert_type,                             \
>                                       left,                                    \
>                                       right,                                   \
>                                       NULL)
>
> +static __maybe_unused noinline void KUNIT_BINARY_NE_ASSERTION(struct
> kunit *test, int assert_type,
> +                                       long long left, long long right)
> +{
> +       __KUNIT_BINARY_NE_ASSERTION(test, assert_type, left, right);
> +}
> +
>  #define KUNIT_BINARY_PTR_NE_MSG_ASSERTION(test,
>                 \
>                                           assert_type,                         \
>                                           left,                                \
>
>
> A little more work is needed to make the varargs and
> code location passing all work correctly.
>
> > > The idea of annotating it got me thinking about what could be
> > > done to improve the structleak plugin, and that in turn got me on
> > > the right track to a silly but trivial fix for the issue: The only thing
> > > that structleak does here is to initialize the implied padding in
> > > the kunit_binary_assert structure. If there is no padding, it all
> > > works out find and the structures don't get pinned to the stack
> > > because the plugin can simply ignore them.
> > >
> > > I tried out this patch and it works:
> > >
> > > diff --git a/include/kunit/assert.h b/include/kunit/assert.h
> > > index db6a0fca09b4..5b09439fa8ae 100644
> > > --- a/include/kunit/assert.h
> > > +++ b/include/kunit/assert.h
> > > @@ -200,8 +200,9 @@ struct kunit_binary_assert {
> > >         struct kunit_assert assert;
> > >         const char *operation;
> > >         const char *left_text;
> > > -       long long left_value;
> > >         const char *right_text;
> > > +       long __pad;
> > > +       long long left_value;
> > >         long long right_value;
> > >  };
> > >
> > > There may also be a problem in 'struct kunit_assert' depending on the
> > > architecture, if there are any on which the 'enum kunit_assert_type'
> > > type is 64 bit wide (which I think is allowed in C, but may not happen
> > > on any toolchain that builds kernels).
> > >
> >
> > What does the padding do? This is all magical!
>
> It turned out to not work after all, The change above fixed some of the
> cases I saw, but not others.
>
> I'm still struggling to fully understand why the structleak gcc plugin
> sometimes forces the structures on the stack and sometimes doesn't.
> The idea for the patch above was to avoid implicit padding by making
> the padding explicit. What happens with the implicit padding is that
> the expanded macro containing code like
>
> struct { const char *left_text; long long left_value; } assert =
>     { .left_text  = # _left, .left_value =  _left };
> func(&assert);
>
> produces a partially initialized object on a 32-bit architecture, with the
> padding between left_text and left_value being old stack data. The
> structleak plugin forces this to be initialized to zero, which in turn
> forces the structure to be allocated on the stack during the execution
> of the function, not just within the surrounding basic block (this
> is a known deficiency in structleak).
>
> The theory so far made sense to me, except that as I said above the
> padding alone did not fix the problem. :(

The padding idea makes sense to me; however, it isn't going to address
the problem with using too much stack space, right? I think the
union/single copy idea would address that, no? (Not that I am excited
by the prospect of making these macros any more magical than they
already are.)