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=-23.3 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS, USER_IN_DEF_DKIM_WL 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 EF87EC433ED for ; Thu, 8 Apr 2021 17:19:21 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 98072610CC for ; Thu, 8 Apr 2021 17:19:21 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 98072610CC Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 3832A6B006E; Thu, 8 Apr 2021 13:19:21 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 35A286B0071; Thu, 8 Apr 2021 13:19:21 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 2251B6B0075; Thu, 8 Apr 2021 13:19:21 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0212.hostedemail.com [216.40.44.212]) by kanga.kvack.org (Postfix) with ESMTP id 068DC6B006E for ; Thu, 8 Apr 2021 13:19:21 -0400 (EDT) Received: from smtpin04.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id AD6EF940D for ; Thu, 8 Apr 2021 17:19:20 +0000 (UTC) X-FDA: 78009860880.04.1827862 Received: from mail-il1-f182.google.com (mail-il1-f182.google.com [209.85.166.182]) by imf10.hostedemail.com (Postfix) with ESMTP id E310A40002D7 for ; Thu, 8 Apr 2021 17:19:16 +0000 (UTC) Received: by mail-il1-f182.google.com with SMTP id t14so2455986ilu.3 for ; Thu, 08 Apr 2021 10:19:20 -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=AGKJ5TMDA3zjXOa+bUjOOsMpKwnvT3r5d1vND1DMuTk=; b=KtD3y5Jd/oXd9jMLZ7G5Zg920b800bH/ytyAWmiEWwV1pzM2KDnyDAoUtg6U2YKAAu HhzgtAMNgiuFl0SxM5Z425vSBji2Cn6V77jZxAsYlYFm3GPilHcU45nggH5hQPo5mUCN YQqUK221JeyVfrFVRswiNQy48LzX2kMy1O6tpf8975lGRiHM73EqL8s1Obu/zuIfr5QW d8x1LVcqUIynDQI8Cm1XIlQPTCU8kIq4jZitcFkkXxpHjQVYop54OxjQ27aAShHJK8Wm 1NJSb802htKeAtrkLHFCVwm6w6zX5Hg2eQp1NLJlG0ZtHaEpiS+8YMyDjp+8eXfhCXtu 4kZA== 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=AGKJ5TMDA3zjXOa+bUjOOsMpKwnvT3r5d1vND1DMuTk=; b=p804nnlw6IuxWnc/rUilEzP84b2A8RTXrcIi2/4h77pRaRY6sBVrAWawhl52nso/n2 qE+kQLsKEM+2qLwds/37H0TIIiLN1Go60tbWdGV66NJjcWYyuJO6VwjJlDsjY/hjS9+I zfAZjbKUktOZsOW7h7gU8ONg+gxJZtsvYiIxHykj6tPSCwt9eR44gLeagdiM598pO3h/ 8xxdl7rfYZf28kTfFzY/W3FwfrzXxrXfVvZqZ0bOvQlUtzKa/UdxY+O+G6KAfhy1aUKY z+ryOLh7Bies69jhG2DNKjgAb3bVqmiwA4Dy07nIWJVxWldCHhLYJDjV5FfBZGcXNAFC y/IA== X-Gm-Message-State: AOAM533g9JTwSDugzvpo3xsgd7ptwSZo/lKXumRXDg5uqdG0VyBXcBdU l5/SYhjxgxAunRRJN+nl5hVKPAIggAs4QBjLTc7ytA== X-Google-Smtp-Source: ABdhPJzSCjdxgqM8te41Rsd1ZSR5QozA3z5PoDP/NIRSSZfzEVUFdfCs65VS+pYV5h6ZsRYxM7nJYBuTGIH8n1w3a+E= X-Received: by 2002:a92:d48c:: with SMTP id p12mr7964592ilg.136.1617902359306; Thu, 08 Apr 2021 10:19:19 -0700 (PDT) MIME-Version: 1.0 References: <20210331085156.5028-1-glittao@gmail.com> <11886d4f-8826-0cd6-b5fd-defc65470ed5@suse.cz> In-Reply-To: From: Daniel Latypov Date: Thu, 8 Apr 2021 10:19:08 -0700 Message-ID: Subject: Re: [PATCH v3 1/2] kunit: add a KUnit test for SLUB debugging functionality To: Marco Elver Cc: Vlastimil Babka , 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 , David Gow Content-Type: text/plain; charset="UTF-8" X-Stat-Signature: i6nmz5fyab3btwxs6qzg6yrad3k1ttqp X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: E310A40002D7 Received-SPF: none (google.com>: No applicable sender policy available) receiver=imf10; identity=mailfrom; envelope-from=""; helo=mail-il1-f182.google.com; client-ip=209.85.166.182 X-HE-DKIM-Result: pass/pass X-HE-Tag: 1617902356-372252 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 Thu, Apr 8, 2021 at 3:30 AM Marco Elver wrote: > > On Tue, 6 Apr 2021 at 12:57, Vlastimil Babka wrote: > > > > > > 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 false; } > > >> > #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-dlatypov@google.com/ > > >> current->kunit_test will always be NULL unless CONFIG_KASAN=y && > > >> CONFIG_KUNIT=y at the moment. > > >> My patch drops the CONFIG_KASAN requirement and opens it up to all tests. > > > > > > Oh, that's a shame, but hopefully it'll be in -next soon. > > > > > >> At the moment, it's just waiting another look over from Brendan or David. > > >> 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 > > > > > > 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 = test;"). > > > And then perhaps add a note ("FIXME: ...") to remove it once the above > > > patch has landed. > > > > > > 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 (thanks > > again for those), I got lockdep splats because slab_add_kunit_errors is called > > 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, the > > question is how to proceed - make it part of Oliver's patch series and let > > Andrew pick it all with eventually kunit team's acks on this patch, or whatnot. > > From what I can tell it should be fine to make it irq safe (ack for > your patch below). Regarding patch logistics, I'd probably add it to > the series. If that ends up not working, we'll find out sooner or > later. > > (FYI, the prerequisite patch for current->kunit_test is in -next now.) Yep. There's also two follow-up patches in https://git.kernel.org/pub/scm/linux/kernel/git/shuah/linux-kselftest.git/log/?h=kunit > > KUnit maintainers, do you have any preferences? Poked offline and Brendan and David seemed fine either way. So probably just include it in this patch series for convenience. Brendan also mentioned KUnit used to use spin_lock_irqsave/restore() but had been told to not use it until necessary. See https://lore.kernel.org/linux-kselftest/20181016235120.138227-3-brendanhiggins@google.com/ So I think there's no objections to the patch itself either. But I'd wait for Brendan to chime in to confirm. > > > ----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 = NULL; > > + unsigned long flags; > > > > - spin_lock(&test->lock); > > + spin_lock_irqsave(&test->lock, flags); > > > > 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, > > } > > } > > > > - spin_unlock(&test->lock); > > + spin_unlock_irqrestore(&test->lock, flags); > > > > 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 = 0; > > + unsigned long flags; > > > > res->free = free; > > kref_init(&res->refcount); > > @@ -454,10 +455,10 @@ int kunit_add_resource(struct kunit *test, > > res->data = data; > > } > > > > - 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); > > > > return ret; > > } > > @@ -515,9 +516,11 @@ EXPORT_SYMBOL_GPL(kunit_alloc_and_get_resource); > > > > void kunit_remove_resource(struct kunit *test, struct kunit_resource *res) > > { > > - 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; > > > > /* > > * 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 = 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))