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=-18.2 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,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, 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 B7314C433B4 for ; Thu, 8 Apr 2021 10:30:03 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 1AA1261164 for ; Thu, 8 Apr 2021 10:30:03 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 1AA1261164 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 85D156B0078; Thu, 8 Apr 2021 06:30:02 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 80D886B007E; Thu, 8 Apr 2021 06:30:02 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 65FEA6B0080; Thu, 8 Apr 2021 06:30:02 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0216.hostedemail.com [216.40.44.216]) by kanga.kvack.org (Postfix) with ESMTP id 499076B0078 for ; Thu, 8 Apr 2021 06:30:02 -0400 (EDT) Received: from smtpin09.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 010A687CC for ; Thu, 8 Apr 2021 10:30:01 +0000 (UTC) X-FDA: 78008829444.09.A750B14 Received: from mail-oo1-f54.google.com (mail-oo1-f54.google.com [209.85.161.54]) by imf24.hostedemail.com (Postfix) with ESMTP id A184CA000390 for ; Thu, 8 Apr 2021 10:29:57 +0000 (UTC) Received: by mail-oo1-f54.google.com with SMTP id c12-20020a4ae24c0000b02901bad05f40e4so388715oot.4 for ; Thu, 08 Apr 2021 03:30:01 -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=z5Jk69Eodvn8mB9Y/1oitzOjfLDng5NAhnlUtQRke6o=; b=hhrXI6/8Law1BByCC2ndfnbNBuU9E2RLS86u6jXlcb67cAtrHqCmzuLBSijXi099RH oRmddHnwgRQWw21f+rYzsYyrwexvANN5UH2Jj3FqL6XcNc5hXSYN9RdW7GdGrAPOBys8 AAnIEEgXB+IQ1A3ea8uCpy951WnPuUHjWV+XyhK7QJltvXup56NsEze+MQh4Udp2L0bu 7E/rgINuinGyVu5U3AwIojJ362vhmbp0Ry0d8d4fZ4m+08jVKM3cg1Duq6rGWq1rb7xs fy+2gDezoRoU3zaQ/sTnynWZH2hcBoT23aHuKRxS4+GyS8bG9wwOFPibE2Me46QieAn8 9A4w== 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=z5Jk69Eodvn8mB9Y/1oitzOjfLDng5NAhnlUtQRke6o=; b=A6+RwlLs0khkXwzxaEWozyi8xTHWJB2L4R5uKME68ckA9yEwgt2ojlUw3g/pfbyyRt W6c6RFr9H4rga60hjvQd4Dse+nvvWM8AxPU2pihR3yn+0ZVJVTz1J4kxMc+NyWPWHy/v TCo+zao2Efttj6Uv78eSBQl8al3ACKGG7t/nYtHl666R1hS8LEo5UcMRxKqubWIkJBS3 2aJhKjYJ6s+b7cu/+PGF1icmMYe77qsxDFlv+1uYprLcG+cX48QrVxLTdSt9SFB6R3Yh 1T4PLK3m0bXwGaEAik2+4Eyb6G32+EvbBSwbNKHtyQU8TNb5n2ouiHPDvSw8bFUbN+lE jLZA== X-Gm-Message-State: AOAM53088nT4TklRWvbp2A8395gCsuaTiTLSJqFHiPiu8D97XR8E1Qi7 i1mhJMWKRQYDb60lBEXTbKy20nWdU9ocBhPYx8NXtA== X-Google-Smtp-Source: ABdhPJxu1iJpsLan+Jp9cwaurMcGWnf7sM8GOfKTaxtbFbX//ta2pm+21tpuXx4BUhPvMkEiRXwgF30QQ4dg5BYEuS8= X-Received: by 2002:a4a:d0ce:: with SMTP id u14mr6895828oor.36.1617877800640; Thu, 08 Apr 2021 03:30:00 -0700 (PDT) MIME-Version: 1.0 References: <20210331085156.5028-1-glittao@gmail.com> <11886d4f-8826-0cd6-b5fd-defc65470ed5@suse.cz> In-Reply-To: <11886d4f-8826-0cd6-b5fd-defc65470ed5@suse.cz> From: Marco Elver Date: Thu, 8 Apr 2021 12:29:49 +0200 Message-ID: Subject: Re: [PATCH v3 1/2] kunit: add a KUnit test for SLUB debugging functionality To: Vlastimil Babka Cc: Daniel Latypov , 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-Rspamd-Server: rspam01 X-Rspamd-Queue-Id: A184CA000390 X-Stat-Signature: ooad5q57513msdaf7wpuabp4jinr6hqi Received-SPF: none (google.com>: No applicable sender policy available) receiver=imf24; identity=mailfrom; envelope-from=""; helo=mail-oo1-f54.google.com; client-ip=209.85.161.54 X-HE-DKIM-Result: pass/pass X-HE-Tag: 1617877797-531796 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 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.) KUnit maintainers, do you have any preferences? > ----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))