Messages in this thread Patch in this message |  | | From | Arnd Bergmann <> | Date | Fri, 10 Jan 2020 09:04:41 +0100 | Subject | Re: kunit stack usage, was: pmwg-ci report v5.5-rc4-147-gc62d43442481 |
| |
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
|  |