Re: [PATCH v3 linux-kselftest-test 5/6] kunit: allow kunit to be loaded as a module

[Brendan Higgins <brendanhiggins@google.com>]

On Thu, Nov 14, 2019 at 1:31 PM Brendan Higgins
<brendanhiggins@google.com> wrote:
>
> +kselftest and kunit lists to document this decision.

Sorry for the spam. I accidentally CC'ed the doc list instead of the
kselftest list in my previous email.

> On Wed, Nov 13, 2019 at 11:54 PM Alan Maguire <alan.maguire@oracle.com> wrote:
> >
> > On Wed, 13 Nov 2019, Stephen Boyd wrote:
> >
> > > Quoting Brendan Higgins (2019-11-11 13:41:38)
> > > > +Stephen Boyd - since he is more of an expert on the hung task timer than I am.
> > > >
> > > > On Fri, Nov 8, 2019 at 7:30 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> > > > >
> > > > > On Thu, 7 Nov 2019, Brendan Higgins wrote:
> > > > >
> > > > > > On Thu, Oct 17, 2019 at 11:09 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> > > > > > > +MODULE_LICENSE("GPL");
> > > > > > > diff --git a/lib/kunit/try-catch.c b/lib/kunit/try-catch.c
> > > > > > > index 1c1e9af..72fc8ed 100644
> > > > > > > --- a/lib/kunit/try-catch.c
> > > > > > > +++ b/lib/kunit/try-catch.c
> > > > > > > @@ -31,6 +31,8 @@ static int kunit_generic_run_threadfn_adapter(void *data)
> > > > > > >         complete_and_exit(try_catch->try_completion, 0);
> > > > > > >  }
> > > > > > >
> > > > > > > +KUNIT_VAR_SYMBOL(sysctl_hung_task_timeout_secs, unsigned long);
> > > > > >
> > > > > > Can you just export sysctl_hung_task_timeout_secs?
> > > > > >
> > > > > > I don't mean to make you redo all this work for one symbol twice, but
> > > > > > I thought we agreed on just exposing this symbol, but in a namespace.
> > > > > > It seemed like a good use case for that namespaced exporting thing
> > > > > > that Luis was talking about. As I understood it, you would have to
> > > > > > export it in the module that defines it, and then use the new
> > > > > > MODULE_IMPORT_NS() macro here.
> > > > > >
> > > > >
> > > > > Sure, I can certainly look into that, though I wonder if we should
> > > > > consider another possibility - should kunit have its own sysctl table for
> > > > > things like configuring timeouts? I can look at adding a patch for that
> > > >
> > > > So on the one hand, yes, I would like to have configurable test
> > > > timeouts for KUnit, but that is not what the parameter check is for
> > > > here. This is to make sure KUnit times a test case out before the hung
> > > > task timer does.
> > > >
> > > > > prior to the module patch so the issues with exporting the hung task
> > > > > timeout would go away. Now the reason I suggest this isn't as much a hack
> > > > > to solve this specific problem, rather it seems to fit better with the
> > > > > longer-term intent expressed by the comment around use of the field (at
> > > > > least as I read it, I may be wrong).
> > > >
> > > > Not really. Although I do agree that adding configurability here might
> > > > be a good idea, I believe we would need to clamp such a value by
> > > > sysctl_hung_task_timeout_secs regardless since we don't want to be
> > > > killed by the hung task timer; thus, we still need access to
> > > > sysctl_hung_task_timeout_secs either way, and so doing what you are
> > > > proposing would be off topic.
> > > >
> > > > > Exporting the symbol does allow us to piggy-back on an existing value, but
> > > > > maybe we should support out our own tunable "kunit_timeout_secs" here?
> > > > > Doing so would also lay the groundwork for supporting other kunit
> > > > > tunables in the future if needed. What do you think?
> > > >
> > > > The goal is not to piggy back on the value as I mentioned above.
> > > > Stephen, do you have any thoughts on this? Do you see any other
> > > > preferable solution to what Alan is trying to do?
> > >
> > > One idea would be to make some sort of process flag that says "this is a
> > > kunit task, ignore me with regards to the hung task timeout". Then we
> > > can hardcode the 5 minute kunit timeout. I'm not sure we have any more
> > > flags though.
> > >
> > > Or drop the whole timeout clamping logic, let the hung task timeout kick
> > > in and potentially oops the kernel, but then continue to let the test
> > > run and maybe sometimes get the kunit timeout here. This last option
> > > doesn't sound so bad to me given that this is all a corner case anyway
> > > where we don't expect to actually ever hit this problem so letting the
> > > hung task detector do its job is probably fine. This nicely avoids
> > > having to export this symbol to modules too.
> > >
> >
> > Thanks for suggestions! This latter approach seems fine to me; presumably
> > something has gone wrong if we are tripping the hung task timeout anyway,
> > so having an oops to document that seems fine. Brendan, what do you think?
>
> If Stephen thinks it is fine to drop the clamping logic, I think it is
> fine too. I think it would probably be good to replace it with a
> comment under the TODO that explains that a hung test *can* cause an
> oops if the hung task timeout is less than the kunit timeout value. It
> would probably be good to also select a timeout value that is less
> than the default hung task timeout. We might also want to link to this
> discussion. I fully expect that the timeout logic will get more
> attention at some point in the future.
>
> One more thing: Alan, can you submit the commit that drops the
> clamping logic in its own commit? I would prefer to make sure that it
> is easy to spot in the commit history.
>
> Cheers!