From: Stephen Boyd <sboyd@kernel.org>
To: Brendan Higgins <brendanhiggins@google.com>
Cc: Frank Rowand <frowand.list@gmail.com>,
Greg KH <gregkh@linuxfoundation.org>,
Josh Poimboeuf <jpoimboe@redhat.com>,
Kees Cook <keescook@google.com>,
Kieran Bingham <kieran.bingham@ideasonboard.com>,
Luis Chamberlain <mcgrof@kernel.org>,
Peter Zijlstra <peterz@infradead.org>,
Rob Herring <robh@kernel.org>, shuah <shuah@kernel.org>,
Theodore Ts'o <tytso@mit.edu>,
Masahiro Yamada <yamada.masahiro@socionext.com>,
devicetree <devicetree@vger.kernel.org>,
dri-devel <dri-devel@lists.freedesktop.org>,
kunit-dev@googlegroups.com,
"open list:DOCUMENTATION" <linux-doc@vger.kernel.org>,
linux-fsdevel@vger.kernel.org,
linux-kbuild <linux-kbuild@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
"open list:KERNEL SELFTEST FRAMEWORK"
<linux-kselftest@vger.kernel.org>,
linux-nvdimm <linux-nvdimm@lists.01.org>,
linux-um@lists.infradead.org,
Sasha Levin <Alexander.Levin@microsoft.com>,
"Bird, Timothy" <Tim.Bird@sony.com>,
Amir Goldstein <amir73il@gmail.com>,
Dan Carpenter <dan.carpenter@oracle.com>,
Daniel Vetter <daniel@ffwll.ch>, Jeff Dike <jdike@addtoit.com>,
Joel Stanley <joel@jms.id.au>,
Julia Lawall <julia.lawall@lip6.fr>,
Kevin Hilman <khilman@baylibre.com>,
Knut Omang <knut.omang@oracle.com>,
Logan Gunthorpe <logang@deltatee.com>,
Michael Ellerman <mpe@ellerman.id.au>,
Petr Mladek <pmladek@suse.com>,
Randy Dunlap <rdunlap@infradead.org>,
Richard Weinberger <richard@nod.at>,
David Rientjes <rientjes@google.com>,
Steven Rostedt <rostedt@goodmis.org>,
wfg@linux.intel.com
Subject: Re: [PATCH v12 09/18] kunit: test: add support for test abort
Date: Mon, 12 Aug 2019 22:56:14 -0700 [thread overview]
Message-ID: <20190813055615.CA787206C2@mail.kernel.org> (raw)
In-Reply-To: <CAFd5g44XyQi-oprPcdgx-EPboQYaHY6Ocz8Te6NX2SxV=mVhQA@mail.gmail.com>
Quoting Brendan Higgins (2019-08-12 21:57:55)
> On Mon, Aug 12, 2019 at 9:22 PM Stephen Boyd <sboyd@kernel.org> wrote:
> >
> > Quoting Brendan Higgins (2019-08-12 11:24:12)
> > > diff --git a/include/kunit/test.h b/include/kunit/test.h
> > > index 2625bcfeb19ac..93381f841e09f 100644
> > > --- a/include/kunit/test.h
> > > +++ b/include/kunit/test.h
> > > @@ -176,6 +178,11 @@ struct kunit {
> > > */
> > > bool success; /* Read only after test_case finishes! */
> > > spinlock_t lock; /* Gaurds all mutable test state. */
> > > + /*
> > > + * death_test may be both set and unset from multiple threads in a test
> > > + * case.
> > > + */
> > > + bool death_test; /* Protected by lock. */
> > > /*
> > > * Because resources is a list that may be updated multiple times (with
> > > * new resources) from any thread associated with a test case, we must
> > > @@ -184,6 +191,13 @@ struct kunit {
> > > struct list_head resources; /* Protected by lock. */
> > > };
> > >
> > > +static inline void kunit_set_death_test(struct kunit *test, bool death_test)
> > > +{
> > > + spin_lock(&test->lock);
> > > + test->death_test = death_test;
> > > + spin_unlock(&test->lock);
> > > +}
> >
> > These getters and setters are using spinlocks again. It doesn't make any
> > sense. It probably needs a rework like was done for the other bool
> > member, success.
>
> No, this is intentional. death_test can transition from false to true
> and then back to false within the same test. Maybe that deserves a
> comment?
Yes. How does it transition from true to false again?
Either way, having a spinlock around a read/write API doesn't make sense
because it just makes sure that two writes don't overlap, but otherwise
does nothing to keep things synchronized. For example a set to true
after a set to false when the two calls to set true or false aren't
synchronized means they can happen in any order. So I don't see how it
needs a spinlock. The lock needs to be one level higher.
>
> > > +
> > > void kunit_init_test(struct kunit *test, const char *name);
> > >
> > > int kunit_run_tests(struct kunit_suite *suite);
> > > diff --git a/include/kunit/try-catch.h b/include/kunit/try-catch.h
> > > new file mode 100644
> > > index 0000000000000..8a414a9af0b64
> > > --- /dev/null
> > > +++ b/include/kunit/try-catch.h
[...]
> > > +
> > > +/*
> > > + * struct kunit_try_catch - provides a generic way to run code which might fail.
> > > + * @context: used to pass user data to the try and catch functions.
> > > + *
> > > + * kunit_try_catch provides a generic, architecture independent way to execute
> > > + * an arbitrary function of type kunit_try_catch_func_t which may bail out by
> > > + * calling kunit_try_catch_throw(). If kunit_try_catch_throw() is called, @try
> > > + * is stopped at the site of invocation and @catch is catch is called.
> > > + *
> > > + * struct kunit_try_catch provides a generic interface for the functionality
> > > + * needed to implement kunit->abort() which in turn is needed for implementing
> > > + * assertions. Assertions allow stating a precondition for a test simplifying
> > > + * how test cases are written and presented.
> > > + *
> > > + * Assertions are like expectations, except they abort (call
> > > + * kunit_try_catch_throw()) when the specified condition is not met. This is
> > > + * useful when you look at a test case as a logical statement about some piece
> > > + * of code, where assertions are the premises for the test case, and the
> > > + * conclusion is a set of predicates, rather expectations, that must all be
> > > + * true. If your premises are violated, it does not makes sense to continue.
> > > + */
> > > +struct kunit_try_catch {
> > > + /* private: internal use only. */
> > > + struct kunit *test;
> > > + struct completion *try_completion;
> > > + int try_result;
> > > + kunit_try_catch_func_t try;
> > > + kunit_try_catch_func_t catch;
> >
> > Can these other variables be documented in the kernel doc? And should
> > context be marked as 'public'?
>
> Sure, I can document them.
>
> But I don't think context should be public; it should only be accessed
> by kunit_try_catch_* functions. context should only be populated by
> *_init, and will be passed into *try and *catch when they are called
> internally.
Ok. Then I guess just document them all but keep them all marked as
private.
>
> > > + */
> > > +void kunit_generic_try_catch_init(struct kunit_try_catch *try_catch);
> > > +
> > > +#endif /* _KUNIT_TRY_CATCH_H */
> > > diff --git a/kunit/test.c b/kunit/test.c
> > > index e5080a2c6b29c..995cb53fe4ee9 100644
> > > --- a/kunit/test.c
> > > +++ b/kunit/test.c
> > > @@ -158,6 +171,21 @@ static void kunit_fail(struct kunit *test, struct kunit_assert *assert)
> > > kunit_print_string_stream(test, stream);
> > > }
> > >
> > > +void __noreturn kunit_abort(struct kunit *test)
> > > +{
> > > + kunit_set_death_test(test, true);
> > > +
> > > + kunit_try_catch_throw(&test->try_catch);
> > > +
> > > + /*
> > > + * Throw could not abort from test.
> > > + *
> > > + * XXX: we should never reach this line! As kunit_try_catch_throw is
> > > + * marked __noreturn.
> > > + */
> > > + WARN_ONCE(true, "Throw could not abort from test!\n");
> >
> > Should this just be a BUG_ON? It's supposedly impossible.
>
> It should be impossible; it will only reach this line if there is a
> bug in kunit_try_catch_throw. The reason I didn't use BUG_ON was
> because I previously got yelled at for having BUG_ON in this code
> path.
>
> Nevertheless, I think BUG_ON is more correct, so if you will stand by
> it, then that's what I will do.
Yeah BUG_ON is appropriate here and self documenting so please use it.
>
> > > + return;
> > > + }
> > > +
> > > + if (kunit_get_death_test(test)) {
> > > + /*
> > > + * EXPECTED DEATH: kunit_run_case_internal encountered
> > > + * anticipated fatal error. Everything should be in a safe
> > > + * state.
> > > + */
> > > + kunit_run_case_cleanup(test, suite);
> > > + } else {
> > > + /*
> > > + * UNEXPECTED DEATH: kunit_run_case_internal encountered an
> > > + * unanticipated fatal error. We have no idea what the state of
> > > + * the test case is in.
> > > + */
> > > + kunit_handle_test_crash(test, suite, test_case);
> > > + kunit_set_failure(test);
> >
> > Like was done here.
>
> Sorry, like what?
Just saying this has braces for the if-else.
next prev parent reply other threads:[~2019-08-13 5:56 UTC|newest]
Thread overview: 60+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-08-12 18:24 [PATCH v12 00/18] kunit: introduce KUnit, the Linux kernel unit testing framework Brendan Higgins
2019-08-12 18:24 ` [PATCH v12 01/18] kunit: test: add KUnit test runner core Brendan Higgins
2019-08-12 18:24 ` [PATCH v12 02/18] kunit: test: add test resource management API Brendan Higgins
2019-08-12 22:10 ` Stephen Boyd
2019-08-12 18:24 ` [PATCH v12 03/18] kunit: test: add string_stream a std::stream like string builder Brendan Higgins
2019-08-12 22:55 ` Stephen Boyd
2019-08-12 23:33 ` Brendan Higgins
2019-08-12 23:59 ` Stephen Boyd
2019-08-13 0:41 ` Brendan Higgins
2019-08-13 4:56 ` Stephen Boyd
2019-08-13 5:02 ` Brendan Higgins
2019-08-13 5:30 ` Stephen Boyd
2019-08-13 9:04 ` Brendan Higgins
2019-08-13 9:12 ` Brendan Higgins
2019-08-13 16:48 ` Stephen Boyd
2019-08-12 18:24 ` [PATCH v12 04/18] kunit: test: add assertion printing library Brendan Higgins
2019-08-12 23:46 ` Stephen Boyd
2019-08-12 23:56 ` Brendan Higgins
2019-08-13 4:27 ` Brendan Higgins
2019-08-13 4:57 ` Stephen Boyd
2019-08-13 5:03 ` Brendan Higgins
2019-08-12 18:24 ` [PATCH v12 05/18] kunit: test: add the concept of expectations Brendan Higgins
2019-08-12 23:57 ` Stephen Boyd
2019-08-13 0:33 ` Brendan Higgins
2019-08-13 5:02 ` Stephen Boyd
2019-08-13 5:04 ` Brendan Higgins
2019-08-12 18:24 ` [PATCH v12 06/18] kbuild: enable building KUnit Brendan Higgins
2019-08-12 18:24 ` [PATCH v12 07/18] kunit: test: add initial tests Brendan Higgins
2019-08-12 23:59 ` Stephen Boyd
2019-08-12 18:24 ` [PATCH v12 08/18] objtool: add kunit_try_catch_throw to the noreturn list Brendan Higgins
2019-08-12 18:24 ` [PATCH v12 09/18] kunit: test: add support for test abort Brendan Higgins
2019-08-13 4:21 ` Stephen Boyd
2019-08-13 4:57 ` Brendan Higgins
2019-08-13 5:56 ` Stephen Boyd [this message]
2019-08-13 7:52 ` Brendan Higgins
2019-08-13 17:06 ` Stephen Boyd
2019-08-12 18:24 ` [PATCH v12 10/18] kunit: test: add tests for kunit " Brendan Higgins
2019-08-13 4:24 ` Stephen Boyd
2019-08-13 5:06 ` Brendan Higgins
2019-08-13 5:57 ` Stephen Boyd
2019-08-13 7:53 ` Brendan Higgins
2019-08-12 18:24 ` [PATCH v12 11/18] kunit: test: add the concept of assertions Brendan Higgins
2019-08-13 4:55 ` Stephen Boyd
2019-08-13 5:09 ` Brendan Higgins
2019-08-12 18:24 ` [PATCH v12 12/18] kunit: test: add tests for KUnit managed resources Brendan Higgins
2019-08-13 4:31 ` Stephen Boyd
2019-08-13 7:57 ` Brendan Higgins
2019-08-13 17:07 ` Stephen Boyd
2019-08-12 18:24 ` [PATCH v12 13/18] kunit: tool: add Python wrappers for running KUnit tests Brendan Higgins
2019-08-13 6:02 ` Stephen Boyd
2019-08-12 18:24 ` [PATCH v12 14/18] kunit: defconfig: add defconfigs for building " Brendan Higgins
2019-08-13 4:38 ` Stephen Boyd
2019-08-13 7:59 ` Brendan Higgins
2019-08-12 18:24 ` [PATCH v12 15/18] Documentation: kunit: add documentation for KUnit Brendan Higgins
2019-08-13 4:46 ` Stephen Boyd
2019-08-12 18:24 ` [PATCH v12 16/18] MAINTAINERS: add entry for KUnit the unit testing framework Brendan Higgins
2019-08-13 5:26 ` Stephen Boyd
2019-08-12 18:24 ` [PATCH v12 17/18] kernel/sysctl-test: Add null pointer test for sysctl.c:proc_dointvec() Brendan Higgins
2019-08-13 4:48 ` Stephen Boyd
2019-08-12 18:24 ` [PATCH v12 18/18] MAINTAINERS: add proc sysctl KUnit test to PROC SYSCTL section Brendan Higgins
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190813055615.CA787206C2@mail.kernel.org \
--to=sboyd@kernel.org \
--cc=Alexander.Levin@microsoft.com \
--cc=Tim.Bird@sony.com \
--cc=amir73il@gmail.com \
--cc=brendanhiggins@google.com \
--cc=dan.carpenter@oracle.com \
--cc=daniel@ffwll.ch \
--cc=devicetree@vger.kernel.org \
--cc=dri-devel@lists.freedesktop.org \
--cc=frowand.list@gmail.com \
--cc=gregkh@linuxfoundation.org \
--cc=jdike@addtoit.com \
--cc=joel@jms.id.au \
--cc=jpoimboe@redhat.com \
--cc=julia.lawall@lip6.fr \
--cc=keescook@google.com \
--cc=khilman@baylibre.com \
--cc=kieran.bingham@ideasonboard.com \
--cc=knut.omang@oracle.com \
--cc=kunit-dev@googlegroups.com \
--cc=linux-doc@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kbuild@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=linux-nvdimm@lists.01.org \
--cc=linux-um@lists.infradead.org \
--cc=logang@deltatee.com \
--cc=mcgrof@kernel.org \
--cc=mpe@ellerman.id.au \
--cc=peterz@infradead.org \
--cc=pmladek@suse.com \
--cc=rdunlap@infradead.org \
--cc=richard@nod.at \
--cc=rientjes@google.com \
--cc=robh@kernel.org \
--cc=rostedt@goodmis.org \
--cc=shuah@kernel.org \
--cc=tytso@mit.edu \
--cc=wfg@linux.intel.com \
--cc=yamada.masahiro@socionext.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).