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

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

[Arnd Bergmann]

On Mon, Dec 30, 2019 at 6:16 PM PMWG CI <pmwg-ci@linaro.org> wrote:
>
>
> The error/warning: 1 drivers/base/test/property-entry-test.c:214:1: warning: the frame size of 3128 bytes is larger than 2048 bytes [-Wframe-larger-than=]
> ... was introduced by commit:
>
> commit c032ace71c29d513bf9df64ace1885fe5ff24981
> Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> Date:   Wed Dec 4 10:53:15 2019 -0800
>
>     software node: add basic tests for property entries

This problem is a result of the KUNIT_ASSERTION() definition that puts
a local struct on the stack interacting badly with the structleak_plugin
when CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL is set in
allmodconfig:

pe_test_uint_arrays() contains a couple of larger variables, plus 41
instances of KUNIT_EXPECT_*() or KUNIT_ASSERT_*(), each one
of these adds its own copy of the structure, eventually exceeding
the warning limit.

We can work around this locally by splitting up the largest four
functions in this file (pe_test_uints, pe_test_uint_arrays, pe_test_strings,
and pe_test_reference) into smaller functions that stay below the
warning limit, but it would be nice to find a way for kunit to not
use as much stack space. Any suggestions?

        Arnd

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

[Brendan Higgins]

+Stephen Boyd

On Tue, Jan 7, 2020 at 4:37 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Mon, Dec 30, 2019 at 6:16 PM PMWG CI <pmwg-ci@linaro.org> wrote:
> >
> >
> > The error/warning: 1 drivers/base/test/property-entry-test.c:214:1: warning: the frame size of 3128 bytes is larger than 2048 bytes [-Wframe-larger-than=]
> > ... was introduced by commit:
> >
> > commit c032ace71c29d513bf9df64ace1885fe5ff24981
> > Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Date:   Wed Dec 4 10:53:15 2019 -0800
> >
> >     software node: add basic tests for property entries
>
> This problem is a result of the KUNIT_ASSERTION() definition that puts
> a local struct on the stack interacting badly with the structleak_plugin
> when CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL is set in
> allmodconfig:
>
> pe_test_uint_arrays() contains a couple of larger variables, plus 41
> instances of KUNIT_EXPECT_*() or KUNIT_ASSERT_*(), each one
> of these adds its own copy of the structure, eventually exceeding
> the warning limit.

Uh oh, sorry about that.

> We can work around this locally by splitting up the largest four
> functions in this file (pe_test_uints, pe_test_uint_arrays, pe_test_strings,
> and pe_test_reference) into smaller functions that stay below the
> warning limit, but it would be nice to find a way for kunit to not
> use as much stack space. Any suggestions?

Agreed. I have a couple ideas. The grossest idea, which I am guessing
most people won't like is to go back to the stream builder method,
basically build the message as we go on the heap; however, the current
method was created specifically to not do that which much help from
Stephen (CC'ed), so probably not want we want to do.

Another idea, the struct just exists to pack up data and ship it off
to a function which handles the expectation/assertion. Consequently,
the struct is only used inside the expectation/assertion macro; it is
not needed before the macro block and is not needed after it. So could
we maybe make some kind of expectation/assertion union so that we know
they are all the same size, and then somehow tag the stack allocation
so that we only ever have one copy in a stack frame? I am not sure if
that kind of compiler magic exists. I guess one way to accomplish this
is to make a dummy function in KUnit whose job it is to call the unit
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?

Also, sorry in advance for delayed responses: I am on vacation until
the 13th and then I will be at LCA until the 18th.

Cheers!

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

[Arnd Bergmann]

On Wed, Jan 8, 2020 at 3:41 PM Brendan Higgins
<brendanhiggins@google.com> wrote:
> On Tue, Jan 7, 2020 at 4:37 AM Arnd Bergmann <arnd@arndb.de> wrote:
> > On Mon, Dec 30, 2019 at 6:16 PM PMWG CI <pmwg-ci@linaro.org> wrote:
> > pe_test_uint_arrays() contains a couple of larger variables, plus 41
> > instances of KUNIT_EXPECT_*() or KUNIT_ASSERT_*(), each one
> > of these adds its own copy of the structure, eventually exceeding
> > the warning limit.
>
> Uh oh, sorry about that.

No worries, I don't think anyone could have foreseen that bug.

> Another idea, the struct just exists to pack up data and ship it off
> to a function which handles the expectation/assertion. Consequently,
> the struct is only used inside the expectation/assertion macro; it is
> not needed before the macro block and is not needed after it. So could
> we maybe make some kind of expectation/assertion union so that we know
> they are all the same size, and then somehow tag the stack allocation
> so that we only ever have one copy in a stack frame? I am not sure if
> that kind of compiler magic exists. I guess one way to accomplish this
> is to make a dummy function in KUnit whose job it is to call the unit
> 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?

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).

      Arnd

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

[Arnd Bergmann]

On Fri, Jan 10, 2020 at 7:27 AM Stephen Boyd <sboyd@kernel.org> wrote:
> Quoting Arnd Bergmann (2020-01-08 07:13:46)
> > On Wed, Jan 8, 2020 at 3:41 PM Brendan Higgins <brendanhiggins@google.com> wrote:
> > > On Tue, Jan 7, 2020 at 4:37 AM Arnd Bergmann <arnd@arndb.de> 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. :(

        Arnd

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@arndb.de> wrote:
>
> On Fri, Jan 10, 2020 at 7:27 AM Stephen Boyd <sboyd@kernel.org> wrote:
> > Quoting Arnd Bergmann (2020-01-08 07:13:46)
> > > On Wed, Jan 8, 2020 at 3:41 PM Brendan Higgins <brendanhiggins@google.com> wrote:
> > > > On Tue, Jan 7, 2020 at 4:37 AM Arnd Bergmann <arnd@arndb.de> 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.)

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

[Kees Cook]

On Tue, Jan 07, 2020 at 01:37:07PM +0100, Arnd Bergmann wrote:
> On Mon, Dec 30, 2019 at 6:16 PM PMWG CI <pmwg-ci@linaro.org> wrote:
> >
> >
> > The error/warning: 1 drivers/base/test/property-entry-test.c:214:1: warning: the frame size of 3128 bytes is larger than 2048 bytes [-Wframe-larger-than=]
> > ... was introduced by commit:
> >
> > commit c032ace71c29d513bf9df64ace1885fe5ff24981
> > Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > Date:   Wed Dec 4 10:53:15 2019 -0800
> >
> >     software node: add basic tests for property entries
> 
> This problem is a result of the KUNIT_ASSERTION() definition that puts
> a local struct on the stack interacting badly with the structleak_plugin
> when CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL is set in
> allmodconfig:

Geh, BYREF_ALL strikes again. I'm at LCA currently, but I'd like to try
to revisit actually fixing the basic-block splitting in the plugin. This
was looked at before, but I need to dig up the thread.

If a fast fix is needed, I'm fine with disabling BYREF_ALL with KUNIT.
It's not optimal, but I feel it's on the BYREF_ALL code to solve this. :)

-- 
Kees Cook

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

[Brendan Higgins]

On Wed, Jan 15, 2020 at 5:29 PM Kees Cook <keescook@chromium.org> wrote:
>
> On Tue, Jan 07, 2020 at 01:37:07PM +0100, Arnd Bergmann wrote:
> > On Mon, Dec 30, 2019 at 6:16 PM PMWG CI <pmwg-ci@linaro.org> wrote:
> > >
> > >
> > > The error/warning: 1 drivers/base/test/property-entry-test.c:214:1: warning: the frame size of 3128 bytes is larger than 2048 bytes [-Wframe-larger-than=]
> > > ... was introduced by commit:
> > >
> > > commit c032ace71c29d513bf9df64ace1885fe5ff24981
> > > Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > > Date:   Wed Dec 4 10:53:15 2019 -0800
> > >
> > >     software node: add basic tests for property entries
> >
> > This problem is a result of the KUNIT_ASSERTION() definition that puts
> > a local struct on the stack interacting badly with the structleak_plugin
> > when CONFIG_GCC_PLUGIN_STRUCTLEAK_BYREF_ALL is set in
> > allmodconfig:
>
> Geh, BYREF_ALL strikes again. I'm at LCA currently, but I'd like to try
> to revisit actually fixing the basic-block splitting in the plugin. This
> was looked at before, but I need to dig up the thread.

Sounds ideal.

I almost got the idea I suggested with the union/single copy
implemented, but it turns out that it is much more complicated than I
originally thought. It turns out that I need more than one copy per
struct kunit instance, I need one per active thread associated with a
struct kunit instance. It still seems possible to do this with percpu,
but it also makes the macro factory more complicated as well.

I am now questioning whether the approach I suggested is really any
better than Arnd's approach.

So yeah, I would definitely prefer fixing the struct leak code.

> If a fast fix is needed, I'm fine with disabling BYREF_ALL with KUNIT.
> It's not optimal, but I feel it's on the BYREF_ALL code to solve this. :)

Sounds good to me.