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=-10.3 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI,NICE_REPLY_A, SPF_HELO_NONE,USER_AGENT_SANE_1 autolearn=ham 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 3EC3DC433B4 for ; Tue, 6 Apr 2021 10:57:59 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id D1441613C4 for ; Tue, 6 Apr 2021 10:57:58 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org D1441613C4 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=suse.cz Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 501236B007E; Tue, 6 Apr 2021 06:57:58 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 4B1CE6B0080; Tue, 6 Apr 2021 06:57:58 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 352A16B0081; Tue, 6 Apr 2021 06:57:58 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0240.hostedemail.com [216.40.44.240]) by kanga.kvack.org (Postfix) with ESMTP id 173C06B007E for ; Tue, 6 Apr 2021 06:57:58 -0400 (EDT) Received: from smtpin34.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id C047A8249980 for ; Tue, 6 Apr 2021 10:57:57 +0000 (UTC) X-FDA: 78001642194.34.571FA54 Received: from mx2.suse.de (mx2.suse.de [195.135.220.15]) by imf19.hostedemail.com (Postfix) with ESMTP id 3742790009F3 for ; Tue, 6 Apr 2021 10:57:50 +0000 (UTC) X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id BEFFAB14C; Tue, 6 Apr 2021 10:57:55 +0000 (UTC) To: Marco Elver , Daniel Latypov Cc: glittao@gmail.com, Christoph Lameter , Pekka Enberg , David Rientjes , Joonsoo Kim , Andrew Morton , Linux Kernel Mailing List , Linux Memory Management List , KUnit Development , Brendan Higgins References: <20210331085156.5028-1-glittao@gmail.com> From: Vlastimil Babka Subject: Re: [PATCH v3 1/2] kunit: add a KUnit test for SLUB debugging functionality Message-ID: <11886d4f-8826-0cd6-b5fd-defc65470ed5@suse.cz> Date: Tue, 6 Apr 2021 12:57:54 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.8.1 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Language: en-US X-Stat-Signature: mmo5hz3m9w7f41d5numnz8m9iejreosf X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 3742790009F3 Received-SPF: none (suse.cz>: No applicable sender policy available) receiver=imf19; identity=mailfrom; envelope-from=""; helo=mx2.suse.de; client-ip=195.135.220.15 X-HE-DKIM-Result: none/none X-HE-Tag: 1617706670-901296 Content-Transfer-Encoding: quoted-printable X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On 4/1/21 11:24 PM, Marco Elver wrote: > On Thu, 1 Apr 2021 at 21:04, Daniel Latypov wrote= : >> > } >> > #else >> > static inline bool slab_add_kunit_errors(void) { return fals= e; } >> > #endif >> > >> > And anywhere you want to increase the error count, you'd call >> > slab_add_kunit_errors(). >> > >> > Another benefit of this approach is that if KUnit is disabled, there= is >> > zero overhead and no additional code generated (vs. the current >> > approach). >> >> The resource approach looks really good, but... >> You'd be picking up a dependency on >> https://lore.kernel.org/linux-kselftest/20210311152314.3814916-2-dlaty= pov@google.com/ >> current->kunit_test will always be NULL unless CONFIG_KASAN=3Dy && >> CONFIG_KUNIT=3Dy at the moment. >> My patch drops the CONFIG_KASAN requirement and opens it up to all tes= ts. >=20 > Oh, that's a shame, but hopefully it'll be in -next soon. >=20 >> At the moment, it's just waiting another look over from Brendan or Dav= id. >> Any ETA on that, folks? :) >> >> So if you don't want to get blocked on that for now, I think it's fine= to add: >> #ifdef CONFIG_SLUB_KUNIT_TEST >> int errors; >> #endif >=20 > Until kunit fixes setting current->kunit_test, a cleaner workaround > that would allow to do the patch with kunit_resource, is to just have > an .init/.exit function that sets it ("current->kunit_test =3D test;"). > And then perhaps add a note ("FIXME: ...") to remove it once the above > patch has landed. >=20 > At least that way we get the least intrusive change for mm/slub.c, and > the test is the only thing that needs a 2-line patch to clean up > later. So when testing internally Oliver's new version with your suggestions (th= anks again for those), I got lockdep splats because slab_add_kunit_errors is c= alled also from irq disabled contexts, and kunit_find_named_resource will call spin_lock(&test->lock) that's not irq safe. Can we make the lock irq safe= ? I tried the change below and it makde the problem go away. If you agree, th= e question is how to proceed - make it part of Oliver's patch series and le= t Andrew pick it all with eventually kunit team's acks on this patch, or wh= atnot. ----8<---- commit ab28505477892e9824c57ac338c88aec2ec0abce Author: Vlastimil Babka Date: Tue Apr 6 12:28:07 2021 +0200 kunit: make test->lock irq safe diff --git a/include/kunit/test.h b/include/kunit/test.h index 49601c4b98b8..524d4789af22 100644 --- a/include/kunit/test.h +++ b/include/kunit/test.h @@ -515,8 +515,9 @@ kunit_find_resource(struct kunit *test, void *match_data) { struct kunit_resource *res, *found =3D NULL; + unsigned long flags; =20 - spin_lock(&test->lock); + spin_lock_irqsave(&test->lock, flags); =20 list_for_each_entry_reverse(res, &test->resources, node) { if (match(test, res, (void *)match_data)) { @@ -526,7 +527,7 @@ kunit_find_resource(struct kunit *test, } } =20 - spin_unlock(&test->lock); + spin_unlock_irqrestore(&test->lock, flags); =20 return found; } diff --git a/lib/kunit/test.c b/lib/kunit/test.c index ec9494e914ef..2c62eeb45b82 100644 --- a/lib/kunit/test.c +++ b/lib/kunit/test.c @@ -442,6 +442,7 @@ int kunit_add_resource(struct kunit *test, void *data) { int ret =3D 0; + unsigned long flags; =20 res->free =3D free; kref_init(&res->refcount); @@ -454,10 +455,10 @@ int kunit_add_resource(struct kunit *test, res->data =3D data; } =20 - spin_lock(&test->lock); + spin_lock_irqsave(&test->lock, flags); list_add_tail(&res->node, &test->resources); /* refcount for list is established by kref_init() */ - spin_unlock(&test->lock); + spin_unlock_irqrestore(&test->lock, flags); =20 return ret; } @@ -515,9 +516,11 @@ EXPORT_SYMBOL_GPL(kunit_alloc_and_get_resource); =20 void kunit_remove_resource(struct kunit *test, struct kunit_resource *re= s) { - spin_lock(&test->lock); + unsigned long flags; + + spin_lock_irqsave(&test->lock, flags); list_del(&res->node); - spin_unlock(&test->lock); + spin_unlock_irqrestore(&test->lock, flags); kunit_put_resource(res); } EXPORT_SYMBOL_GPL(kunit_remove_resource); @@ -597,6 +600,7 @@ EXPORT_SYMBOL_GPL(kunit_kfree); void kunit_cleanup(struct kunit *test) { struct kunit_resource *res; + unsigned long flags; =20 /* * test->resources is a stack - each allocation must be freed in the @@ -608,9 +612,9 @@ void kunit_cleanup(struct kunit *test) * protect against the current node being deleted, not the next. */ while (true) { - spin_lock(&test->lock); + spin_lock_irqsave(&test->lock, flags); if (list_empty(&test->resources)) { - spin_unlock(&test->lock); + spin_unlock_irqrestore(&test->lock, flags); break; } res =3D list_last_entry(&test->resources, @@ -621,7 +625,7 @@ void kunit_cleanup(struct kunit *test) * resource, and this can't happen if the test->lock * is held. */ - spin_unlock(&test->lock); + spin_unlock_irqrestore(&test->lock, flags); kunit_remove_resource(test, res); } #if (IS_ENABLED(CONFIG_KASAN) && IS_ENABLED(CONFIG_KUNIT))