Hi David,
On Tue, 2023-04-18 at 14:53 +0800, David Gow wrote:
> On Mon, 17 Apr 2023 at 18:43, Benjamin Berg <benjamin@xxxxxxxxxxxxxxxx> wrote:
> >
> > Hi,
> >
> > On Sat, 2023-04-15 at 17:14 +0800, David Gow wrote:
> > > KUnit tests run in a kthread, with the current->kunit_test pointer set
> > > to the test's context. This allows the kunit_get_current_test() and
> > > kunit_fail_current_test() macros to work. Normally, this pointer is
> > > still valid during test shutdown (i.e., the suite->exit function, and
> > > any resource cleanup). However, if the test has exited early (e.g., due
> > > to a failed assertion), the cleanup is done in the parent KUnit thread,
> > > which does not have an active context.
> >
> > My only question here is whether assertions (not expectations) are OK
> > within the exit/cleanup handler. That said, it wouldn't be clear
> > whether we should try to continue cleaning up after every failure, so
> > probably it is reasonable to not do that.
>
> Excellent point.
> In general:
> - It's okay to use expectations within exit and cleanup functions.
> - It's not okay for assertions to fail within an exit / cleanup handler.
> - It's not okay to access anything which was allocated on the stack
> from within a test in exit / cleanup handlers.
> - It's okay to access and free resources from within cleanup / exit
> handlers, though it's not permitted to create new resources from
> cleanup handlers (exit is okay).
The list makes sense to me.
> I do think we need to document this better, at the very least.
>
> What I think we'll end up doing is implementing a different system:
> - The test (and, if successful, cleanup) runs in a kthread.
> - If it aborts (e.g., due to an assertion), cleanup runs in another kthread.
> - If this second kthread aborts early, no further cleanup is run.
>
> This would protect the KUnit executor thread from misbehaving cleanup
> functions, and if an assertion happens in a cleanup function, we'll
> leak things (which is bad), but not dereference a bunch of invalid
> pointers (which is worse).
Sounds good.
> I've got this mostly working here, and will send it out as a
> replacement to this patch (that second kthread will have
> current->kunit_test set, rendering this change redundant).
Cool!
> > But, I did see that at least the clock tests currently use assertions
> > inside the init function. And, in those tests, if the context
> > allocation fails the exit handler will dereference the NULL pointer.
>
> Hmm... which clock test is that? Regardless, it sounds like a bug in the test.
>
> I think that ultimately, the right solution for dealing with the
> context pointer issue is to use resources (or, when available,
> kunit_add_action()), which nicely enforces the ordering as well. In
> the meantime, though, I guess it just needs wrapping in a lot of "if
> (ctx)" or similar...
I am talking about drivers/clk/clk-gate_test.c. The _init functions
(and clk_gate_test_alloc_ctx) use assertions heavily. The _exit handles
does not deal with the ctx being NULL though. But, the fix is trivial
though:
diff --git a/drivers/clk/clk-gate_test.c b/drivers/clk/clk-gate_test.c
index e136aaad48bf..f1adafe725e4 100644
--- a/drivers/clk/clk-gate_test.c
+++ b/drivers/clk/clk-gate_test.c
@@ -225,6 +225,9 @@ static void clk_gate_test_exit(struct kunit *test)
{
struct clk_gate_test_context *ctx = test->priv;
+ if (!ctx)
+ return;
+
clk_hw_unregister_gate(ctx->hw);
clk_hw_unregister_fixed_rate(ctx->parent);
}
> > So, nothing for this patch, but maybe it would make sense to mark tests
> > as failed (or print a warning) if they use assertions inside init, exit
> > or cleanup handlers?
> >
>
> I think we'll still want to support assertions in init functions
> (albeit possibly with some documentation pointing out any pitfalls).
> If actions or resources are used, it's not a problem.
Yeah, a short sentence in the "struct kunit_suite" documentation should
be enough. Something along the lines of: "This call to @exit will
always happen, even if @init returned an error or aborted due to an
assertion.".
> Also, a lot of these issues also apply to kunit_skip(), which is used
> _heavily_ in init functions.
>
> Warnings for assertions in exit or cleanup make sense. I _could_ see a
> reason to allow assertions if we wanted to have, e.g., helpers which
> asserted that their inputs were valid -- it'd be a bit rough to deny
> their use if the inputs are known to be okay -- but I'm not aware of
> any such case in practice yet, so I suspect it's still worth failing
> tests (and seeing if anyone complains).
I am not going to insist on disallowing or warning about assertions. It
is OK from my point of view, as long as the damage is contained to a
kunit kthread and "only" affects cleanup.
Benjamin
