From mboxrd@z Thu Jan 1 00:00:00 1970 From: Brendan Higgins Subject: Re: [PATCH v5 01/18] kunit: test: add KUnit test runner core Date: Fri, 28 Jun 2019 01:09:44 -0700 Message-ID: References: <20190617082613.109131-1-brendanhiggins@google.com> <20190617082613.109131-2-brendanhiggins@google.com> <20190620001526.93426218BE@mail.kernel.org> <20190626034100.B238520883@mail.kernel.org> <20190627181636.5EA752064A@mail.kernel.org> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20190627181636.5EA752064A@mail.kernel.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Stephen Boyd 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 T24gVGh1LCBKdW4gMjcsIDIwMTkgYXQgMTE6MTYgQU0gU3RlcGhlbiBCb3lkIDxzYm95ZEBrZXJu ZWwub3JnPiB3cm90ZToKPgo+IFF1b3RpbmcgQnJlbmRhbiBIaWdnaW5zICgyMDE5LTA2LTI2IDE2 OjAwOjQwKQo+ID4gT24gVHVlLCBKdW4gMjUsIDIwMTkgYXQgODo0MSBQTSBTdGVwaGVuIEJveWQg PHNib3lkQGtlcm5lbC5vcmc+IHdyb3RlOgo+ID4KPiA+ID4gc2NlbmFyaW8gbGlrZSBiZWxvdywg YnV0IHdoZXJlIGl0IGlzIGEgcHJvYmxlbS4gVGhlcmUgY291bGQgYmUgdGhyZWUKPiA+ID4gQ1BV cywgb3IgZXZlbiBvbmUgQ1BVIGFuZCB0aHJlZSB0aHJlYWRzIGlmIHlvdSB3YW50IHRvIGRlc2Ny aWJlIHRoZQo+ID4gPiBleHRyYSB0aHJlYWQgc2NlbmFyaW8uCj4gPiA+Cj4gPiA+IEhlcmUncyBt eSBzY2VuYXJpbyB3aGVyZSBpdCBpc24ndCBuZWVkZWQ6Cj4gPiA+Cj4gPiA+ICAgICBDUFUwICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBDUFUxCj4gPiA+ICAgICAtLS0tICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAtLS0tCj4gPiA+ICAgICBrdW5pdF9y dW5fdGVzdCgmdGVzdCkKPiA+ID4gICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgIHRlc3RfY2FzZV9mdW5jKCkKPiA+ID4gICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgLi4uLgo+ID4gPiAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgW21vY2sgaGFyZGlycV0KPiA+ID4gICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAga3VuaXRfc2V0X3N1Y2Nlc3MoJnRl c3QpCj4gPiA+ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBb aGFyZGlycSBlbmRzXQo+ID4gPiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAuLi4KPiA+ID4gICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICAg ICAgICAgICAgICAgY29tcGxldGUoJnRlc3RfZG9uZSkKPiA+ID4gICAgICAgd2FpdF9mb3JfY29t cGxldGlvbigmdGVzdF9kb25lKQo+ID4gPiAgICAgICBrdW5pdF9nZXRfc3VjY2VzcygmdGVzdCkK PiA+ID4KPiA+ID4gV2UgZG9uJ3QgbmVlZCB0byBjYXJlIGFib3V0IGhhdmluZyBsb2NraW5nIGhl cmUgYmVjYXVzZSBzdWNjZXNzIG9yCj4gPiA+IGZhaWx1cmUgb25seSBoYXBwZW5zIGluIG9uZSBw bGFjZSBhbmQgaXQncyBzeW5jaHJvbml6ZWQgd2l0aCB0aGUKPiA+ID4gY29tcGxldGlvbi4KPiA+ Cj4gPiBIZXJlIGlzIHRoZSBzY2VuYXJpbyBJIGFtIGNvbmNlcm5lZCBhYm91dDoKPiA+Cj4gPiBD UFUwICAgICAgICAgICAgICAgICAgICAgIENQVTEgICAgICAgICAgICAgICAgICAgICAgIENQVTIK PiA+IC0tLS0gICAgICAgICAgICAgICAgICAgICAgLS0tLSAgICAgICAgICAgICAgICAgICAgICAg LS0tLQo+ID4ga3VuaXRfcnVuX3Rlc3QoJnRlc3QpCj4gPiAgICAgICAgICAgICAgICAgICAgICAg ICAgIHRlc3RfY2FzZV9mdW5jKCkKPiA+ICAgICAgICAgICAgICAgICAgICAgICAgICAgICAuLi4u Cj4gPiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgc2NoZWR1bGVfd29yayhmb29fZnVuYykK PiA+ICAgICAgICAgICAgICAgICAgICAgICAgICAgW21vY2sgaGFyZGlycV0gICAgICAgICAgICAg Zm9vX2Z1bmMoKQo+ID4gICAgICAgICAgICAgICAgICAgICAgICAgICAgIC4uLiAgICAgICAgICAg ICAgICAgICAgICAgIC4uLgo+ID4gICAgICAgICAgICAgICAgICAgICAgICAgICAgIGt1bml0X3Nl dF9zdWNjZXNzKGZhbHNlKSAgIGt1bml0X3NldF9zdWNjZXNzKGZhbHNlKQo+ID4gICAgICAgICAg ICAgICAgICAgICAgICAgICBbaGFyZGlycSBlbmRzXSAgICAgICAgICAgICAgIC4uLgo+ID4gICAg ICAgICAgICAgICAgICAgICAgICAgICAgIC4uLgo+ID4gICAgICAgICAgICAgICAgICAgICAgICAg ICAgIGNvbXBsZXRlKCZ0ZXN0X2RvbmUpCj4gPiAgIHdhaXRfZm9yX2NvbXBsZXRpb24oLi4uKQo+ ID4gICBrdW5pdF9nZXRfc3VjY2VzcygmdGVzdCkKPiA+Cj4gPiBJbiBteSBzY2VuYXJpbywgc2lu Y2UgYm90aCBDUFUxIGFuZCBDUFUyIHVwZGF0ZSB0aGUgc3VjY2VzcyBzdGF0dXMgb2YKPiA+IHRo ZSB0ZXN0IHNpbXVsdGFuZW91c2x5LCBldmVuIHRob3VnaCB0aGV5IGFyZSBzZXR0aW5nIGl0IHRv IHRoZSBzYW1lCj4gPiB2YWx1ZS4gSWYgbXkgdW5kZXJzdGFuZGluZyBpcyBjb3JyZWN0LCB0aGlz IGNvdWxkIHJlc3VsdCBpbiBhCj4gPiB3cml0ZS10ZWFyIG9uIHNvbWUgYXJjaGl0ZWN0dXJlcyBp biBzb21lIGNpcmN1bXN0YW5jZXMuIEkgc3VwcG9zZSB3ZQo+ID4gY291bGQganVzdCBtYWtlIGl0 IGFuIGF0b21pYyBib29sZWFuLCBidXQgSSBmaWd1cmVkIGxvY2tpbmcgaXMgYWxzbwo+ID4gZmlu ZSwgYW5kIGdlbmVyYWxseSBwcmVmZXJyZWQuCj4KPiBUaGlzIGlzIHdoYXQgd2UgaGF2ZSBXUklU RV9PTkNFKCkgYW5kIFJFQURfT05DRSgpIGZvci4gTWF5YmUgeW91IGNvdWxkCj4ganVzdCB1c2Ug dGhhdCBpbiB0aGUgZ2V0dGVyIGFuZCBzZXR0ZXJzIGFuZCByZW1vdmUgdGhlIGxvY2sgaWYgaXQg aXNuJ3QKPiB1c2VkIGZvciBhbnl0aGluZyBlbHNlLgo+Cj4gSXQgbWF5IGFsc28gYmUgYSBnb29k IGlkZWEgdG8gaGF2ZSBhIGt1bml0X2ZhaWxfdGVzdCgpIEFQSSB0aGF0IGZhaWxzCj4gdGhlIHRl c3QgcGFzc2VkIGluIHdpdGggYSBXUklURV9PTkNFKGZhbHNlKS4gT3RoZXJ3aXNlLCB0aGUgdGVz dCBpcwo+IGFzc3VtZWQgc3VjY2Vzc2Z1bCBhbmQgaXQgaXNuJ3QgZXZlbiBwb3NzaWJsZSBmb3Ig YSB0ZXN0IHRvIGNoYW5nZSB0aGUKPiBzdGF0ZSBmcm9tIGZhaWx1cmUgdG8gc3VjY2VzcyBkdWUg dG8gYSBsb2dpY2FsIGVycm9yIGJlY2F1c2UgdGhlIEFQSQo+IGlzbid0IGF2YWlsYWJsZS4gVGhl biB3ZSBkb24ndCByZWFsbHkgbmVlZCB0byBoYXZlIGEgZ2VuZXJpYwo+IGt1bml0X3NldF9zdWNj ZXNzKCkgZnVuY3Rpb24gYXQgYWxsLiBXZSBjb3VsZCBoYXZlIGEga3VuaXRfdGVzdF9mYWlsZWQo KQo+IGZ1bmN0aW9uIHRvbyB0aGF0IHJlcGxhY2VzIHRoZSBrdW5pdF9nZXRfc3VjY2VzcygpIGZ1 bmN0aW9uLiBUaGF0IHdvdWxkCj4gcmVhZCBiZXR0ZXIgaW4gYW4gaWYgY29uZGl0aW9uLgoKWW91 IGtub3cgd2hhdCwgSSB0aGluayB5b3UgYXJlIHJpZ2h0LgoKU29ycnksIGZvciBub3QgcmVhbGl6 aW5nIHRoaXMgZWFybGllciwgSSB0aGluayB5b3UgbWVudGlvbmVkIHNvbWV0aGluZwphbG9uZyB0 aGVzZSBsaW5lcyBhIGxvbmcgdGltZSBhZ28uCgpUaGFua3MgZm9yIHlvdXIgcGF0aWVuY2UhCgo+ ID4KPiA+IEFsc28sIHRvIGJlIGNsZWFyLCBJIGFtIG9uYm9hcmQgd2l0aCBkcm9wcGluZyB0aGVu IElSUSBzdHVmZiBmb3Igbm93Lgo+ID4gSSBhbSBmaW5lIG1vdmluZyB0byBhIG11dGV4IGZvciB0 aGUgdGltZSBiZWluZy4KPiA+Cj4KPiBPay4KClRoYW5rcyEKX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVsIG1haWxpbmcgbGlzdApkcmktZGV2 ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21h aWxtYW4vbGlzdGluZm8vZHJpLWRldmVs From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.4 required=3.0 tests=DKIMWL_WL_MED,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,USER_IN_DEF_DKIM_WL autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0DAA6C4321A for ; Fri, 28 Jun 2019 08:09:59 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id CC8802063F for ; Fri, 28 Jun 2019 08:09:58 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=google.com header.i=@google.com header.b="e5kLlPi6" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726842AbfF1IJ5 (ORCPT ); Fri, 28 Jun 2019 04:09:57 -0400 Received: from mail-pg1-f194.google.com ([209.85.215.194]:32921 "EHLO mail-pg1-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726562AbfF1IJ4 (ORCPT ); Fri, 28 Jun 2019 04:09:56 -0400 Received: by mail-pg1-f194.google.com with SMTP id m4so2252774pgk.0 for ; Fri, 28 Jun 2019 01:09:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=yibP/Iv6ps2Lbs4MfioQUnaQ1X0MZgCBiZaerRAAUs0=; b=e5kLlPi61UOlg3aWbm3hyHJ6zEvPrbJg4WaMTgr5J7N6beCFHbvMVMWvXm34zFIMK6 sy+TCthgNvl6QKdGAvF199JWgLSzY0riL7Qema9Z8eQSMrEVD7QmDgObmKA9VnGm6jQk Qk9Vw4tG1NS/Rsaor+MwqJJtzbW9NLCCg64qdKIwo/lqdLjqjD0HoEJZR2UkGwIQNQU7 B8LVi8olEdiEG1gQQPAGB3QH7tntl/R65/A9R5rItsLpolqg+IRJEp1Du/WH5LqoOfL3 I3l8I0kuDspWHSG/V5vI+q5UJh9nND3VlxUlk753w0GhWsuvm1nMSIm2VqKl1wq8mbGo 9uhw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=yibP/Iv6ps2Lbs4MfioQUnaQ1X0MZgCBiZaerRAAUs0=; b=k1xPfUIvIDha27hIh88QC5jhQ8eA7QxQR8qzFb3WM1BWcWnJ99/cp5BX9wv2fc9eQF fvgoGnboyuSr0ZFL7kIJPXY370TR3Z8AjrCf3s3xTphuV9tUXV1fbFhzkl12EuDFRBBO LV/nwPMFciLkTcIKuMDLvj2g+uPrUYfAfbtwqvBKlnjFxVeL5vm7vbdWoh8uh6e5pnAn tJTnwCiprArbb+0Aw/s+jkJASj5MCDHAu+RAGVShZqGWKi4xukFQtG67OfUZvxJzoujH l67Kvjurlhdbt67TDD1hOStED5HdW+z4pT5v0rUPGUtFRCf/NcDU72omYbbHb2Z5cDEl ouXA== X-Gm-Message-State: APjAAAVu//zzdcBdTOiQl0a4D7UBDeqem+sb/M8XMoTiwwnHP7ize2Dt UyROi8V3ILmxlVXuCs3xvuNZTKiq3v15Cg315/VV4Q== X-Google-Smtp-Source: APXvYqwa5Gi43WdeQYYulM3cScojWTt0/MI+kDN9NbNdt1CeW4jxKTFR3Algm98Jmh+XvwAADJqdAi9a1c1dDVA05Ew= X-Received: by 2002:a17:90a:be0d:: with SMTP id a13mr11033056pjs.84.1561709395368; Fri, 28 Jun 2019 01:09:55 -0700 (PDT) MIME-Version: 1.0 References: <20190617082613.109131-1-brendanhiggins@google.com> <20190617082613.109131-2-brendanhiggins@google.com> <20190620001526.93426218BE@mail.kernel.org> <20190626034100.B238520883@mail.kernel.org> <20190627181636.5EA752064A@mail.kernel.org> In-Reply-To: <20190627181636.5EA752064A@mail.kernel.org> From: Brendan Higgins Date: Fri, 28 Jun 2019 01:09:44 -0700 Message-ID: Subject: Re: [PATCH v5 01/18] kunit: test: add KUnit test runner core To: 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 Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jun 27, 2019 at 11:16 AM Stephen Boyd wrote: > > Quoting Brendan Higgins (2019-06-26 16:00:40) > > On Tue, Jun 25, 2019 at 8:41 PM Stephen Boyd wrote: > > > > > scenario like below, but where it is a problem. There could be three > > > CPUs, or even one CPU and three threads if you want to describe the > > > extra thread scenario. > > > > > > Here's my scenario where it isn't needed: > > > > > > CPU0 CPU1 > > > ---- ---- > > > kunit_run_test(&test) > > > test_case_func() > > > .... > > > [mock hardirq] > > > kunit_set_success(&test) > > > [hardirq ends] > > > ... > > > complete(&test_done) > > > wait_for_completion(&test_done) > > > kunit_get_success(&test) > > > > > > We don't need to care about having locking here because success or > > > failure only happens in one place and it's synchronized with the > > > completion. > > > > Here is the scenario I am concerned about: > > > > CPU0 CPU1 CPU2 > > ---- ---- ---- > > kunit_run_test(&test) > > test_case_func() > > .... > > schedule_work(foo_func) > > [mock hardirq] foo_func() > > ... ... > > kunit_set_success(false) kunit_set_success(false) > > [hardirq ends] ... > > ... > > complete(&test_done) > > wait_for_completion(...) > > kunit_get_success(&test) > > > > In my scenario, since both CPU1 and CPU2 update the success status of > > the test simultaneously, even though they are setting it to the same > > value. If my understanding is correct, this could result in a > > write-tear on some architectures in some circumstances. I suppose we > > could just make it an atomic boolean, but I figured locking is also > > fine, and generally preferred. > > This is what we have WRITE_ONCE() and READ_ONCE() for. Maybe you could > just use that in the getter and setters and remove the lock if it isn't > used for anything else. > > It may also be a good idea to have a kunit_fail_test() API that fails > the test passed in with a WRITE_ONCE(false). Otherwise, the test is > assumed successful and it isn't even possible for a test to change the > state from failure to success due to a logical error because the API > isn't available. Then we don't really need to have a generic > kunit_set_success() function at all. We could have a kunit_test_failed() > function too that replaces the kunit_get_success() function. That would > read better in an if condition. You know what, I think you are right. Sorry, for not realizing this earlier, I think you mentioned something along these lines a long time ago. Thanks for your patience! > > > > Also, to be clear, I am onboard with dropping then IRQ stuff for now. > > I am fine moving to a mutex for the time being. > > > > Ok. Thanks! From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-f195.google.com ([209.85.210.195]:41770 "EHLO mail-pf1-f195.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726408AbfF1IJ4 (ORCPT ); Fri, 28 Jun 2019 04:09:56 -0400 Received: by mail-pf1-f195.google.com with SMTP id m30so2571542pff.8 for ; Fri, 28 Jun 2019 01:09:56 -0700 (PDT) MIME-Version: 1.0 References: <20190617082613.109131-1-brendanhiggins@google.com> <20190617082613.109131-2-brendanhiggins@google.com> <20190620001526.93426218BE@mail.kernel.org> <20190626034100.B238520883@mail.kernel.org> <20190627181636.5EA752064A@mail.kernel.org> In-Reply-To: <20190627181636.5EA752064A@mail.kernel.org> From: Brendan Higgins Date: Fri, 28 Jun 2019 01:09:44 -0700 Message-ID: Subject: Re: [PATCH v5 01/18] kunit: test: add KUnit test runner core Content-Type: text/plain; charset="UTF-8" Sender: linux-kbuild-owner@vger.kernel.org List-ID: To: 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 On Thu, Jun 27, 2019 at 11:16 AM Stephen Boyd wrote: > > Quoting Brendan Higgins (2019-06-26 16:00:40) > > On Tue, Jun 25, 2019 at 8:41 PM Stephen Boyd wrote: > > > > > scenario like below, but where it is a problem. There could be three > > > CPUs, or even one CPU and three threads if you want to describe the > > > extra thread scenario. > > > > > > Here's my scenario where it isn't needed: > > > > > > CPU0 CPU1 > > > ---- ---- > > > kunit_run_test(&test) > > > test_case_func() > > > .... > > > [mock hardirq] > > > kunit_set_success(&test) > > > [hardirq ends] > > > ... > > > complete(&test_done) > > > wait_for_completion(&test_done) > > > kunit_get_success(&test) > > > > > > We don't need to care about having locking here because success or > > > failure only happens in one place and it's synchronized with the > > > completion. > > > > Here is the scenario I am concerned about: > > > > CPU0 CPU1 CPU2 > > ---- ---- ---- > > kunit_run_test(&test) > > test_case_func() > > .... > > schedule_work(foo_func) > > [mock hardirq] foo_func() > > ... ... > > kunit_set_success(false) kunit_set_success(false) > > [hardirq ends] ... > > ... > > complete(&test_done) > > wait_for_completion(...) > > kunit_get_success(&test) > > > > In my scenario, since both CPU1 and CPU2 update the success status of > > the test simultaneously, even though they are setting it to the same > > value. If my understanding is correct, this could result in a > > write-tear on some architectures in some circumstances. I suppose we > > could just make it an atomic boolean, but I figured locking is also > > fine, and generally preferred. > > This is what we have WRITE_ONCE() and READ_ONCE() for. Maybe you could > just use that in the getter and setters and remove the lock if it isn't > used for anything else. > > It may also be a good idea to have a kunit_fail_test() API that fails > the test passed in with a WRITE_ONCE(false). Otherwise, the test is > assumed successful and it isn't even possible for a test to change the > state from failure to success due to a logical error because the API > isn't available. Then we don't really need to have a generic > kunit_set_success() function at all. We could have a kunit_test_failed() > function too that replaces the kunit_get_success() function. That would > read better in an if condition. You know what, I think you are right. Sorry, for not realizing this earlier, I think you mentioned something along these lines a long time ago. Thanks for your patience! > > > > Also, to be clear, I am onboard with dropping then IRQ stuff for now. > > I am fine moving to a mutex for the time being. > > > > Ok. Thanks! From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-x544.google.com ([2607:f8b0:4864:20::544]) by bombadil.infradead.org with esmtps (Exim 4.92 #3 (Red Hat Linux)) id 1hglxE-0006bO-Ri for linux-um@lists.infradead.org; Fri, 28 Jun 2019 08:09:59 +0000 Received: by mail-pg1-x544.google.com with SMTP id g15so341921pgi.4 for ; Fri, 28 Jun 2019 01:09:56 -0700 (PDT) MIME-Version: 1.0 References: <20190617082613.109131-1-brendanhiggins@google.com> <20190617082613.109131-2-brendanhiggins@google.com> <20190620001526.93426218BE@mail.kernel.org> <20190626034100.B238520883@mail.kernel.org> <20190627181636.5EA752064A@mail.kernel.org> In-Reply-To: <20190627181636.5EA752064A@mail.kernel.org> From: Brendan Higgins Date: Fri, 28 Jun 2019 01:09:44 -0700 Message-ID: Subject: Re: [PATCH v5 01/18] kunit: test: add KUnit test runner core 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: Stephen Boyd 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 On Thu, Jun 27, 2019 at 11:16 AM Stephen Boyd wrote: > > Quoting Brendan Higgins (2019-06-26 16:00:40) > > On Tue, Jun 25, 2019 at 8:41 PM Stephen Boyd wrote: > > > > > scenario like below, but where it is a problem. There could be three > > > CPUs, or even one CPU and three threads if you want to describe the > > > extra thread scenario. > > > > > > Here's my scenario where it isn't needed: > > > > > > CPU0 CPU1 > > > ---- ---- > > > kunit_run_test(&test) > > > test_case_func() > > > .... > > > [mock hardirq] > > > kunit_set_success(&test) > > > [hardirq ends] > > > ... > > > complete(&test_done) > > > wait_for_completion(&test_done) > > > kunit_get_success(&test) > > > > > > We don't need to care about having locking here because success or > > > failure only happens in one place and it's synchronized with the > > > completion. > > > > Here is the scenario I am concerned about: > > > > CPU0 CPU1 CPU2 > > ---- ---- ---- > > kunit_run_test(&test) > > test_case_func() > > .... > > schedule_work(foo_func) > > [mock hardirq] foo_func() > > ... ... > > kunit_set_success(false) kunit_set_success(false) > > [hardirq ends] ... > > ... > > complete(&test_done) > > wait_for_completion(...) > > kunit_get_success(&test) > > > > In my scenario, since both CPU1 and CPU2 update the success status of > > the test simultaneously, even though they are setting it to the same > > value. If my understanding is correct, this could result in a > > write-tear on some architectures in some circumstances. I suppose we > > could just make it an atomic boolean, but I figured locking is also > > fine, and generally preferred. > > This is what we have WRITE_ONCE() and READ_ONCE() for. Maybe you could > just use that in the getter and setters and remove the lock if it isn't > used for anything else. > > It may also be a good idea to have a kunit_fail_test() API that fails > the test passed in with a WRITE_ONCE(false). Otherwise, the test is > assumed successful and it isn't even possible for a test to change the > state from failure to success due to a logical error because the API > isn't available. Then we don't really need to have a generic > kunit_set_success() function at all. We could have a kunit_test_failed() > function too that replaces the kunit_get_success() function. That would > read better in an if condition. You know what, I think you are right. Sorry, for not realizing this earlier, I think you mentioned something along these lines a long time ago. Thanks for your patience! > > > > Also, to be clear, I am onboard with dropping then IRQ stuff for now. > > I am fine moving to a mutex for the time being. > > > > Ok. Thanks! _______________________________________________ linux-um mailing list linux-um@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-um