From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on archive.lwn.net X-Spam-Level: X-Spam-Status: No, score=-6.2 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,MAILING_LIST_MULTI, RCVD_IN_DNSWL_HI,SPF_HELO_NONE,SPF_NONE autolearn=unavailable autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by archive.lwn.net (Postfix) with ESMTP id C5FA87D90D for ; Tue, 13 Aug 2019 05:56:23 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726265AbfHMF4S (ORCPT ); Tue, 13 Aug 2019 01:56:18 -0400 Received: from mail.kernel.org ([198.145.29.99]:42652 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726000AbfHMF4R (ORCPT ); Tue, 13 Aug 2019 01:56:17 -0400 Received: from kernel.org (unknown [104.132.0.74]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id CA787206C2; Tue, 13 Aug 2019 05:56:15 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1565675775; bh=+1m8X1VBJfcfXMxrxr6nTIQlUnVg1cZSUB+Teb53ec4=; h=In-Reply-To:References:Subject:From:Cc:To:Date:From; b=EqLfngCp9rJG/7bhoEPl5NUB9MdmhGlTmUmGoBpVFyaOg/GPk1mA0rcZzq60Wfure Kws2pOKryAzgeVKRCPbKt04zRJdCsk27haqddkgygF0TBApKlaPreQfDt9QdDMWRIv cjQntDvtzPxrcztEj0KRg1dIjU+K5aGLXe3rSAyg= Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable In-Reply-To: References: <20190812182421.141150-1-brendanhiggins@google.com> <20190812182421.141150-10-brendanhiggins@google.com> <20190813042159.46814206C2@mail.kernel.org> Subject: Re: [PATCH v12 09/18] kunit: test: add support for test abort From: Stephen Boyd Cc: Frank Rowand , Greg KH , Josh Poimboeuf , Kees Cook , Kieran Bingham , Luis Chamberlain , Peter Zijlstra , Rob Herring , shuah , Theodore Ts'o , Masahiro Yamada , devicetree , dri-devel , kunit-dev@googlegroups.com, "open list:DOCUMENTATION" , linux-fsdevel@vger.kernel.org, linux-kbuild , Linux Kernel Mailing List , "open list:KERNEL SELFTEST FRAMEWORK" , linux-nvdimm , linux-um@lists.infradead.org, Sasha Levin , "Bird, Timothy" , Amir Goldstein , Dan Carpenter , Daniel Vetter , Jeff Dike , Joel Stanley , Julia Lawall , Kevin Hilman , Knut Omang , Logan Gunthorpe , Michael Ellerman , Petr Mladek , Randy Dunlap , Richard Weinberger , David Rientjes , Steven Rostedt , wfg@linux.intel.com To: Brendan Higgins User-Agent: alot/0.8.1 Date: Mon, 12 Aug 2019 22:56:14 -0700 Message-Id: <20190813055615.CA787206C2@mail.kernel.org> Sender: linux-doc-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-doc@vger.kernel.org Quoting Brendan Higgins (2019-08-12 21:57:55) > On Mon, Aug 12, 2019 at 9:22 PM Stephen Boyd 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 t= imes (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 dea= th_test) > > > +{ > > > + spin_lock(&test->lock); > > > + test->death_test =3D 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. >=20 > 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. >=20 > > > + > > > 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 ba= il out by > > > + * calling kunit_try_catch_throw(). If kunit_try_catch_throw() is ca= lled, @try > > > + * is stopped at the site of invocation and @catch is catch is calle= d. > > > + * > > > + * struct kunit_try_catch provides a generic interface for the funct= ionality > > > + * needed to implement kunit->abort() which in turn is needed for im= plementing > > > + * assertions. Assertions allow stating a precondition for a test si= mplifying > > > + * 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 c= ontinue. > > > + */ > > > +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'? >=20 > Sure, I can document them. >=20 > 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. >=20 > > > + */ > > > +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, struc= t 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_t= hrow is > > > + * marked __noreturn. > > > + */ > > > + WARN_ONCE(true, "Throw could not abort from test!\n"); > > > > Should this just be a BUG_ON? It's supposedly impossible. >=20 > 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. >=20 > 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. >=20 > > > + 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 encounte= red an > > > + * unanticipated fatal error. We have no idea what th= e state of > > > + * the test case is in. > > > + */ > > > + kunit_handle_test_crash(test, suite, test_case); > > > + kunit_set_failure(test); > > > > Like was done here. >=20 > Sorry, like what? Just saying this has braces for the if-else. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stephen Boyd Subject: Re: [PATCH v12 09/18] kunit: test: add support for test abort Date: Mon, 12 Aug 2019 22:56:14 -0700 Message-ID: <20190813055615.CA787206C2@mail.kernel.org> References: <20190812182421.141150-1-brendanhiggins@google.com> <20190812182421.141150-10-brendanhiggins@google.com> <20190813042159.46814206C2@mail.kernel.org> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Brendan Higgins Cc: Petr Mladek , "open list:DOCUMENTATION" , Peter Zijlstra , Amir Goldstein , dri-devel , Sasha Levin , Masahiro Yamada , Michael Ellerman , "open list:KERNEL SELFTEST FRAMEWORK" , shuah , linux-nvdimm , Frank Rowand , Knut Omang , Kieran Bingham , wfg@linux.intel.com, Joel Stanley , David Rientjes , Jeff Dike , Dan Carpenter , devicetree , linux-kbuild , "Bird, Timothy" List-Id: linux-nvdimm@lists.01.org UXVvdGluZyBCcmVuZGFuIEhpZ2dpbnMgKDIwMTktMDgtMTIgMjE6NTc6NTUpCj4gT24gTW9uLCBB dWcgMTIsIDIwMTkgYXQgOToyMiBQTSBTdGVwaGVuIEJveWQgPHNib3lkQGtlcm5lbC5vcmc+IHdy b3RlOgo+ID4KPiA+IFF1b3RpbmcgQnJlbmRhbiBIaWdnaW5zICgyMDE5LTA4LTEyIDExOjI0OjEy KQo+ID4gPiBkaWZmIC0tZ2l0IGEvaW5jbHVkZS9rdW5pdC90ZXN0LmggYi9pbmNsdWRlL2t1bml0 L3Rlc3QuaAo+ID4gPiBpbmRleCAyNjI1YmNmZWIxOWFjLi45MzM4MWY4NDFlMDlmIDEwMDY0NAo+ ID4gPiAtLS0gYS9pbmNsdWRlL2t1bml0L3Rlc3QuaAo+ID4gPiArKysgYi9pbmNsdWRlL2t1bml0 L3Rlc3QuaAo+ID4gPiBAQCAtMTc2LDYgKzE3OCwxMSBAQCBzdHJ1Y3Qga3VuaXQgewo+ID4gPiAg ICAgICAgICAqLwo+ID4gPiAgICAgICAgIGJvb2wgc3VjY2VzczsgLyogUmVhZCBvbmx5IGFmdGVy IHRlc3RfY2FzZSBmaW5pc2hlcyEgKi8KPiA+ID4gICAgICAgICBzcGlubG9ja190IGxvY2s7IC8q IEdhdXJkcyBhbGwgbXV0YWJsZSB0ZXN0IHN0YXRlLiAqLwo+ID4gPiArICAgICAgIC8qCj4gPiA+ ICsgICAgICAgICogZGVhdGhfdGVzdCBtYXkgYmUgYm90aCBzZXQgYW5kIHVuc2V0IGZyb20gbXVs dGlwbGUgdGhyZWFkcyBpbiBhIHRlc3QKPiA+ID4gKyAgICAgICAgKiBjYXNlLgo+ID4gPiArICAg ICAgICAqLwo+ID4gPiArICAgICAgIGJvb2wgZGVhdGhfdGVzdDsgLyogUHJvdGVjdGVkIGJ5IGxv Y2suICovCj4gPiA+ICAgICAgICAgLyoKPiA+ID4gICAgICAgICAgKiBCZWNhdXNlIHJlc291cmNl cyBpcyBhIGxpc3QgdGhhdCBtYXkgYmUgdXBkYXRlZCBtdWx0aXBsZSB0aW1lcyAod2l0aAo+ID4g PiAgICAgICAgICAqIG5ldyByZXNvdXJjZXMpIGZyb20gYW55IHRocmVhZCBhc3NvY2lhdGVkIHdp dGggYSB0ZXN0IGNhc2UsIHdlIG11c3QKPiA+ID4gQEAgLTE4NCw2ICsxOTEsMTMgQEAgc3RydWN0 IGt1bml0IHsKPiA+ID4gICAgICAgICBzdHJ1Y3QgbGlzdF9oZWFkIHJlc291cmNlczsgLyogUHJv dGVjdGVkIGJ5IGxvY2suICovCj4gPiA+ICB9Owo+ID4gPgo+ID4gPiArc3RhdGljIGlubGluZSB2 b2lkIGt1bml0X3NldF9kZWF0aF90ZXN0KHN0cnVjdCBrdW5pdCAqdGVzdCwgYm9vbCBkZWF0aF90 ZXN0KQo+ID4gPiArewo+ID4gPiArICAgICAgIHNwaW5fbG9jaygmdGVzdC0+bG9jayk7Cj4gPiA+ ICsgICAgICAgdGVzdC0+ZGVhdGhfdGVzdCA9IGRlYXRoX3Rlc3Q7Cj4gPiA+ICsgICAgICAgc3Bp bl91bmxvY2soJnRlc3QtPmxvY2spOwo+ID4gPiArfQo+ID4KPiA+IFRoZXNlIGdldHRlcnMgYW5k IHNldHRlcnMgYXJlIHVzaW5nIHNwaW5sb2NrcyBhZ2Fpbi4gSXQgZG9lc24ndCBtYWtlIGFueQo+ ID4gc2Vuc2UuIEl0IHByb2JhYmx5IG5lZWRzIGEgcmV3b3JrIGxpa2Ugd2FzIGRvbmUgZm9yIHRo ZSBvdGhlciBib29sCj4gPiBtZW1iZXIsIHN1Y2Nlc3MuCj4gCj4gTm8sIHRoaXMgaXMgaW50ZW50 aW9uYWwuIGRlYXRoX3Rlc3QgY2FuIHRyYW5zaXRpb24gZnJvbSBmYWxzZSB0byB0cnVlCj4gYW5k IHRoZW4gYmFjayB0byBmYWxzZSB3aXRoaW4gdGhlIHNhbWUgdGVzdC4gTWF5YmUgdGhhdCBkZXNl cnZlcyBhCj4gY29tbWVudD8KClllcy4gSG93IGRvZXMgaXQgdHJhbnNpdGlvbiBmcm9tIHRydWUg dG8gZmFsc2UgYWdhaW4/CgpFaXRoZXIgd2F5LCBoYXZpbmcgYSBzcGlubG9jayBhcm91bmQgYSBy ZWFkL3dyaXRlIEFQSSBkb2Vzbid0IG1ha2Ugc2Vuc2UKYmVjYXVzZSBpdCBqdXN0IG1ha2VzIHN1 cmUgdGhhdCB0d28gd3JpdGVzIGRvbid0IG92ZXJsYXAsIGJ1dCBvdGhlcndpc2UKZG9lcyBub3Ro aW5nIHRvIGtlZXAgdGhpbmdzIHN5bmNocm9uaXplZC4gRm9yIGV4YW1wbGUgYSBzZXQgdG8gdHJ1 ZQphZnRlciBhIHNldCB0byBmYWxzZSB3aGVuIHRoZSB0d28gY2FsbHMgdG8gc2V0IHRydWUgb3Ig ZmFsc2UgYXJlbid0CnN5bmNocm9uaXplZCBtZWFucyB0aGV5IGNhbiBoYXBwZW4gaW4gYW55IG9y ZGVyLiBTbyBJIGRvbid0IHNlZSBob3cgaXQKbmVlZHMgYSBzcGlubG9jay4gVGhlIGxvY2sgbmVl ZHMgdG8gYmUgb25lIGxldmVsIGhpZ2hlci4KCj4gCj4gPiA+ICsKPiA+ID4gIHZvaWQga3VuaXRf aW5pdF90ZXN0KHN0cnVjdCBrdW5pdCAqdGVzdCwgY29uc3QgY2hhciAqbmFtZSk7Cj4gPiA+Cj4g PiA+ICBpbnQga3VuaXRfcnVuX3Rlc3RzKHN0cnVjdCBrdW5pdF9zdWl0ZSAqc3VpdGUpOwo+ID4g PiBkaWZmIC0tZ2l0IGEvaW5jbHVkZS9rdW5pdC90cnktY2F0Y2guaCBiL2luY2x1ZGUva3VuaXQv dHJ5LWNhdGNoLmgKPiA+ID4gbmV3IGZpbGUgbW9kZSAxMDA2NDQKPiA+ID4gaW5kZXggMDAwMDAw MDAwMDAwMC4uOGE0MTRhOWFmMGI2NAo+ID4gPiAtLS0gL2Rldi9udWxsCj4gPiA+ICsrKyBiL2lu Y2x1ZGUva3VuaXQvdHJ5LWNhdGNoLmgKWy4uLl0KPiA+ID4gKwo+ID4gPiArLyoKPiA+ID4gKyAq IHN0cnVjdCBrdW5pdF90cnlfY2F0Y2ggLSBwcm92aWRlcyBhIGdlbmVyaWMgd2F5IHRvIHJ1biBj b2RlIHdoaWNoIG1pZ2h0IGZhaWwuCj4gPiA+ICsgKiBAY29udGV4dDogdXNlZCB0byBwYXNzIHVz ZXIgZGF0YSB0byB0aGUgdHJ5IGFuZCBjYXRjaCBmdW5jdGlvbnMuCj4gPiA+ICsgKgo+ID4gPiAr ICoga3VuaXRfdHJ5X2NhdGNoIHByb3ZpZGVzIGEgZ2VuZXJpYywgYXJjaGl0ZWN0dXJlIGluZGVw ZW5kZW50IHdheSB0byBleGVjdXRlCj4gPiA+ICsgKiBhbiBhcmJpdHJhcnkgZnVuY3Rpb24gb2Yg dHlwZSBrdW5pdF90cnlfY2F0Y2hfZnVuY190IHdoaWNoIG1heSBiYWlsIG91dCBieQo+ID4gPiAr ICogY2FsbGluZyBrdW5pdF90cnlfY2F0Y2hfdGhyb3coKS4gSWYga3VuaXRfdHJ5X2NhdGNoX3Ro cm93KCkgaXMgY2FsbGVkLCBAdHJ5Cj4gPiA+ICsgKiBpcyBzdG9wcGVkIGF0IHRoZSBzaXRlIG9m IGludm9jYXRpb24gYW5kIEBjYXRjaCBpcyBjYXRjaCBpcyBjYWxsZWQuCj4gPiA+ICsgKgo+ID4g PiArICogc3RydWN0IGt1bml0X3RyeV9jYXRjaCBwcm92aWRlcyBhIGdlbmVyaWMgaW50ZXJmYWNl IGZvciB0aGUgZnVuY3Rpb25hbGl0eQo+ID4gPiArICogbmVlZGVkIHRvIGltcGxlbWVudCBrdW5p dC0+YWJvcnQoKSB3aGljaCBpbiB0dXJuIGlzIG5lZWRlZCBmb3IgaW1wbGVtZW50aW5nCj4gPiA+ ICsgKiBhc3NlcnRpb25zLiBBc3NlcnRpb25zIGFsbG93IHN0YXRpbmcgYSBwcmVjb25kaXRpb24g Zm9yIGEgdGVzdCBzaW1wbGlmeWluZwo+ID4gPiArICogaG93IHRlc3QgY2FzZXMgYXJlIHdyaXR0 ZW4gYW5kIHByZXNlbnRlZC4KPiA+ID4gKyAqCj4gPiA+ICsgKiBBc3NlcnRpb25zIGFyZSBsaWtl IGV4cGVjdGF0aW9ucywgZXhjZXB0IHRoZXkgYWJvcnQgKGNhbGwKPiA+ID4gKyAqIGt1bml0X3Ry eV9jYXRjaF90aHJvdygpKSB3aGVuIHRoZSBzcGVjaWZpZWQgY29uZGl0aW9uIGlzIG5vdCBtZXQu IFRoaXMgaXMKPiA+ID4gKyAqIHVzZWZ1bCB3aGVuIHlvdSBsb29rIGF0IGEgdGVzdCBjYXNlIGFz IGEgbG9naWNhbCBzdGF0ZW1lbnQgYWJvdXQgc29tZSBwaWVjZQo+ID4gPiArICogb2YgY29kZSwg d2hlcmUgYXNzZXJ0aW9ucyBhcmUgdGhlIHByZW1pc2VzIGZvciB0aGUgdGVzdCBjYXNlLCBhbmQg dGhlCj4gPiA+ICsgKiBjb25jbHVzaW9uIGlzIGEgc2V0IG9mIHByZWRpY2F0ZXMsIHJhdGhlciBl eHBlY3RhdGlvbnMsIHRoYXQgbXVzdCBhbGwgYmUKPiA+ID4gKyAqIHRydWUuIElmIHlvdXIgcHJl bWlzZXMgYXJlIHZpb2xhdGVkLCBpdCBkb2VzIG5vdCBtYWtlcyBzZW5zZSB0byBjb250aW51ZS4K PiA+ID4gKyAqLwo+ID4gPiArc3RydWN0IGt1bml0X3RyeV9jYXRjaCB7Cj4gPiA+ICsgICAgICAg LyogcHJpdmF0ZTogaW50ZXJuYWwgdXNlIG9ubHkuICovCj4gPiA+ICsgICAgICAgc3RydWN0IGt1 bml0ICp0ZXN0Owo+ID4gPiArICAgICAgIHN0cnVjdCBjb21wbGV0aW9uICp0cnlfY29tcGxldGlv bjsKPiA+ID4gKyAgICAgICBpbnQgdHJ5X3Jlc3VsdDsKPiA+ID4gKyAgICAgICBrdW5pdF90cnlf Y2F0Y2hfZnVuY190IHRyeTsKPiA+ID4gKyAgICAgICBrdW5pdF90cnlfY2F0Y2hfZnVuY190IGNh dGNoOwo+ID4KPiA+IENhbiB0aGVzZSBvdGhlciB2YXJpYWJsZXMgYmUgZG9jdW1lbnRlZCBpbiB0 aGUga2VybmVsIGRvYz8gQW5kIHNob3VsZAo+ID4gY29udGV4dCBiZSBtYXJrZWQgYXMgJ3B1Ymxp Yyc/Cj4gCj4gU3VyZSwgSSBjYW4gZG9jdW1lbnQgdGhlbS4KPiAKPiBCdXQgSSBkb24ndCB0aGlu ayBjb250ZXh0IHNob3VsZCBiZSBwdWJsaWM7IGl0IHNob3VsZCBvbmx5IGJlIGFjY2Vzc2VkCj4g Ynkga3VuaXRfdHJ5X2NhdGNoXyogZnVuY3Rpb25zLiBjb250ZXh0IHNob3VsZCBvbmx5IGJlIHBv cHVsYXRlZCBieQo+ICpfaW5pdCwgYW5kIHdpbGwgYmUgcGFzc2VkIGludG8gKnRyeSBhbmQgKmNh dGNoIHdoZW4gdGhleSBhcmUgY2FsbGVkCj4gaW50ZXJuYWxseS4KCk9rLiBUaGVuIEkgZ3Vlc3Mg anVzdCBkb2N1bWVudCB0aGVtIGFsbCBidXQga2VlcCB0aGVtIGFsbCBtYXJrZWQgYXMKcHJpdmF0 ZS4KCj4gCj4gPiA+ICsgKi8KPiA+ID4gK3ZvaWQga3VuaXRfZ2VuZXJpY190cnlfY2F0Y2hfaW5p dChzdHJ1Y3Qga3VuaXRfdHJ5X2NhdGNoICp0cnlfY2F0Y2gpOwo+ID4gPiArCj4gPiA+ICsjZW5k aWYgLyogX0tVTklUX1RSWV9DQVRDSF9IICovCj4gPiA+IGRpZmYgLS1naXQgYS9rdW5pdC90ZXN0 LmMgYi9rdW5pdC90ZXN0LmMKPiA+ID4gaW5kZXggZTUwODBhMmM2YjI5Yy4uOTk1Y2I1M2ZlNGVl OSAxMDA2NDQKPiA+ID4gLS0tIGEva3VuaXQvdGVzdC5jCj4gPiA+ICsrKyBiL2t1bml0L3Rlc3Qu Ywo+ID4gPiBAQCAtMTU4LDYgKzE3MSwyMSBAQCBzdGF0aWMgdm9pZCBrdW5pdF9mYWlsKHN0cnVj dCBrdW5pdCAqdGVzdCwgc3RydWN0IGt1bml0X2Fzc2VydCAqYXNzZXJ0KQo+ID4gPiAgICAgICAg IGt1bml0X3ByaW50X3N0cmluZ19zdHJlYW0odGVzdCwgc3RyZWFtKTsKPiA+ID4gIH0KPiA+ID4K PiA+ID4gK3ZvaWQgX19ub3JldHVybiBrdW5pdF9hYm9ydChzdHJ1Y3Qga3VuaXQgKnRlc3QpCj4g PiA+ICt7Cj4gPiA+ICsgICAgICAga3VuaXRfc2V0X2RlYXRoX3Rlc3QodGVzdCwgdHJ1ZSk7Cj4g PiA+ICsKPiA+ID4gKyAgICAgICBrdW5pdF90cnlfY2F0Y2hfdGhyb3coJnRlc3QtPnRyeV9jYXRj aCk7Cj4gPiA+ICsKPiA+ID4gKyAgICAgICAvKgo+ID4gPiArICAgICAgICAqIFRocm93IGNvdWxk IG5vdCBhYm9ydCBmcm9tIHRlc3QuCj4gPiA+ICsgICAgICAgICoKPiA+ID4gKyAgICAgICAgKiBY WFg6IHdlIHNob3VsZCBuZXZlciByZWFjaCB0aGlzIGxpbmUhIEFzIGt1bml0X3RyeV9jYXRjaF90 aHJvdyBpcwo+ID4gPiArICAgICAgICAqIG1hcmtlZCBfX25vcmV0dXJuLgo+ID4gPiArICAgICAg ICAqLwo+ID4gPiArICAgICAgIFdBUk5fT05DRSh0cnVlLCAiVGhyb3cgY291bGQgbm90IGFib3J0 IGZyb20gdGVzdCFcbiIpOwo+ID4KPiA+IFNob3VsZCB0aGlzIGp1c3QgYmUgYSBCVUdfT04/IEl0 J3Mgc3VwcG9zZWRseSBpbXBvc3NpYmxlLgo+IAo+IEl0IHNob3VsZCBiZSBpbXBvc3NpYmxlOyBp dCB3aWxsIG9ubHkgcmVhY2ggdGhpcyBsaW5lIGlmIHRoZXJlIGlzIGEKPiBidWcgaW4ga3VuaXRf dHJ5X2NhdGNoX3Rocm93LiBUaGUgcmVhc29uIEkgZGlkbid0IHVzZSBCVUdfT04gd2FzCj4gYmVj YXVzZSBJIHByZXZpb3VzbHkgZ290IHllbGxlZCBhdCBmb3IgaGF2aW5nIEJVR19PTiBpbiB0aGlz IGNvZGUKPiBwYXRoLgo+IAo+IE5ldmVydGhlbGVzcywgSSB0aGluayBCVUdfT04gaXMgbW9yZSBj b3JyZWN0LCBzbyBpZiB5b3Ugd2lsbCBzdGFuZCBieQo+IGl0LCB0aGVuIHRoYXQncyB3aGF0IEkg d2lsbCBkby4KClllYWggQlVHX09OIGlzIGFwcHJvcHJpYXRlIGhlcmUgYW5kIHNlbGYgZG9jdW1l bnRpbmcgc28gcGxlYXNlIHVzZSBpdC4KCj4gCj4gPiA+ICsgICAgICAgICAgICAgICByZXR1cm47 Cj4gPiA+ICsgICAgICAgfQo+ID4gPiArCj4gPiA+ICsgICAgICAgaWYgKGt1bml0X2dldF9kZWF0 aF90ZXN0KHRlc3QpKSB7Cj4gPiA+ICsgICAgICAgICAgICAgICAvKgo+ID4gPiArICAgICAgICAg ICAgICAgICogRVhQRUNURUQgREVBVEg6IGt1bml0X3J1bl9jYXNlX2ludGVybmFsIGVuY291bnRl cmVkCj4gPiA+ICsgICAgICAgICAgICAgICAgKiBhbnRpY2lwYXRlZCBmYXRhbCBlcnJvci4gRXZl cnl0aGluZyBzaG91bGQgYmUgaW4gYSBzYWZlCj4gPiA+ICsgICAgICAgICAgICAgICAgKiBzdGF0 ZS4KPiA+ID4gKyAgICAgICAgICAgICAgICAqLwo+ID4gPiArICAgICAgICAgICAgICAga3VuaXRf cnVuX2Nhc2VfY2xlYW51cCh0ZXN0LCBzdWl0ZSk7Cj4gPiA+ICsgICAgICAgfSBlbHNlIHsKPiA+ ID4gKyAgICAgICAgICAgICAgIC8qCj4gPiA+ICsgICAgICAgICAgICAgICAgKiBVTkVYUEVDVEVE IERFQVRIOiBrdW5pdF9ydW5fY2FzZV9pbnRlcm5hbCBlbmNvdW50ZXJlZCBhbgo+ID4gPiArICAg ICAgICAgICAgICAgICogdW5hbnRpY2lwYXRlZCBmYXRhbCBlcnJvci4gV2UgaGF2ZSBubyBpZGVh IHdoYXQgdGhlIHN0YXRlIG9mCj4gPiA+ICsgICAgICAgICAgICAgICAgKiB0aGUgdGVzdCBjYXNl IGlzIGluLgo+ID4gPiArICAgICAgICAgICAgICAgICovCj4gPiA+ICsgICAgICAgICAgICAgICBr dW5pdF9oYW5kbGVfdGVzdF9jcmFzaCh0ZXN0LCBzdWl0ZSwgdGVzdF9jYXNlKTsKPiA+ID4gKyAg ICAgICAgICAgICAgIGt1bml0X3NldF9mYWlsdXJlKHRlc3QpOwo+ID4KPiA+IExpa2Ugd2FzIGRv bmUgaGVyZS4KPiAKPiBTb3JyeSwgbGlrZSB3aGF0PwoKSnVzdCBzYXlpbmcgdGhpcyBoYXMgYnJh Y2VzIGZvciB0aGUgaWYtZWxzZS4KCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVk ZXNrdG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZv L2RyaS1kZXZlbA== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.kernel.org ([198.145.29.99]) by bombadil.infradead.org with esmtps (Exim 4.92 #3 (Red Hat Linux)) id 1hxPn6-0005S1-NA for linux-um@lists.infradead.org; Tue, 13 Aug 2019 05:56:18 +0000 MIME-Version: 1.0 In-Reply-To: References: <20190812182421.141150-1-brendanhiggins@google.com> <20190812182421.141150-10-brendanhiggins@google.com> <20190813042159.46814206C2@mail.kernel.org> Subject: Re: [PATCH v12 09/18] kunit: test: add support for test abort From: Stephen Boyd Date: Mon, 12 Aug 2019 22:56:14 -0700 Message-Id: <20190813055615.CA787206C2@mail.kernel.org> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-um" Errors-To: linux-um-bounces+geert=linux-m68k.org@lists.infradead.org To: Brendan Higgins Cc: Petr Mladek , "open list:DOCUMENTATION" , Peter Zijlstra , Amir Goldstein , dri-devel , Sasha Levin , Masahiro Yamada , Michael Ellerman , "open list:KERNEL SELFTEST FRAMEWORK" , shuah , Rob Herring , linux-nvdimm , Frank Rowand , Knut Omang , Kieran Bingham , wfg@linux.intel.com, Joel Stanley , David Rientjes , Jeff Dike , Dan Carpenter , devicetree , linux-kbuild , "Bird, Timothy , linux-um@lists.infradead.org, Steven Rostedt" , Julia Lawall , Josh Poimboeuf , kunit-dev@googlegroups.com, Theodore Ts'o , Richard Weinberger , Greg KH , Randy Dunlap , Linux Kernel Mailing List , Luis Chamberlain , Daniel Vetter , Kees Cook , linux-fsdevel@vger.kernel.org, Logan Gunthorpe , Kevin Hilman Quoting Brendan Higgins (2019-08-12 21:57:55) > On Mon, Aug 12, 2019 at 9:22 PM Stephen Boyd 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. _______________________________________________ linux-um mailing list linux-um@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-um