Messages in this thread |  | | From | Brendan Higgins <> | Date | Mon, 13 Jan 2020 17:50:25 -0800 | Subject | Re: kunit stack usage, was: pmwg-ci report v5.5-rc4-147-gc62d43442481 |
| |
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.)
|  |