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.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,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 BC840C433B4 for ; Thu, 1 Apr 2021 21:24:47 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 30901610F8 for ; Thu, 1 Apr 2021 21:24:47 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 30901610F8 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 624846B00BF; Thu, 1 Apr 2021 17:24:46 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 5D4646B00C1; Thu, 1 Apr 2021 17:24:46 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 476626B00C2; Thu, 1 Apr 2021 17:24:46 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0136.hostedemail.com [216.40.44.136]) by kanga.kvack.org (Postfix) with ESMTP id 2570F6B00BF for ; Thu, 1 Apr 2021 17:24:46 -0400 (EDT) Received: from smtpin01.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id D9F7E180424BE for ; Thu, 1 Apr 2021 21:24:45 +0000 (UTC) X-FDA: 77985077730.01.160E3F2 Received: from mail-ot1-f49.google.com (mail-ot1-f49.google.com [209.85.210.49]) by imf16.hostedemail.com (Postfix) with ESMTP id 0E22980192DD for ; Thu, 1 Apr 2021 21:24:44 +0000 (UTC) Received: by mail-ot1-f49.google.com with SMTP id g8-20020a9d6c480000b02901b65ca2432cso3383062otq.3 for ; Thu, 01 Apr 2021 14:24:45 -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=e0lNND0HV6YpmDvNBpdxNkFf2/SfSlXhkdI6Kpj/xns=; b=E/UxQ1lGJQpxazQTiwKeC5yiB6dFRaWo9vi/vWvDSWNwnbHxte+6GFMKkpRhsZU0Dx PH4DS43QWRHY3/LtpjuxHBr65DgM/z7FqVbCTmuFHSMW2HLltKDQ0SJ2A55QzFjqKjy8 KTxrQ625LSCOC6x0gIo69W8MgWWr47501N4BsG5cRLgi4vGbZtJXVFmnePjwxajk7xAi n5TuDT4AawVzLxj27fu6BiKV7ZkW71zbyVrNx5oUBWwoKvF9PP8V5gJ9W+ek9e/aUy9X xVsD+Ho8BME6DlQ68aqawc54GqouwOHPDu0H5KBSGWIKXhwjV1TED++M9fvYZQwWlWPD TWhg== 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=e0lNND0HV6YpmDvNBpdxNkFf2/SfSlXhkdI6Kpj/xns=; b=Wkc1IFAH41TE0Jp6x84zWJfrNtEbwQqAYR1OI+92KWRNQupBDu891x5bq1O5yJp5wI SdJ6rDmhdmQWNjiBz0eK1AFPg1UmtQmOqD/acuq2aJ4BBqOgLcawnXry4WGM4t5d68cg Y6Zp65YgHSDDO+umpwmaMC7ncME9HkIF+ATqd7sfaxhvvUk7ppISzL/vxtPVOofD4jOg qX5VqWW1078xqkhfA5cxekSwfbKgJTc1QE3tVDPjeEpN8N75Ln+MeR5XMFSAktI1gAv0 fx3RDqult2Hl0uETZsi7Gy8XWRUSHHlEajGFRVfpZsjYqXzeVEKKOI8fkTKiq0DZfG19 h2ZA== X-Gm-Message-State: AOAM5338wqkiXhNjfgEDYCnh60VtfuzaX0eQ2Wnf7YOpjJV9QeXZBHXo oP7e8W/9l6RDIf4U0roJSD+yECkEgE7lEEoQ41ifDg== X-Google-Smtp-Source: ABdhPJxLl6Lv9qEblE1Z3Tl33FCBxoYUWizRywN+OE0WOCtECeRu4FyQjf5wDN4JKxB8jzi3KCqhVff4W5BFOElecw0= X-Received: by 2002:a05:6830:148c:: with SMTP id s12mr8828319otq.251.1617312284641; Thu, 01 Apr 2021 14:24:44 -0700 (PDT) MIME-Version: 1.0 References: <20210331085156.5028-1-glittao@gmail.com> In-Reply-To: From: Marco Elver Date: Thu, 1 Apr 2021 23:24:32 +0200 Message-ID: Subject: Re: [PATCH v3 1/2] kunit: add a KUnit test for SLUB debugging functionality To: Daniel Latypov Cc: glittao@gmail.com, Christoph Lameter , Pekka Enberg , David Rientjes , Joonsoo Kim , Andrew Morton , Vlastimil Babka , Linux Kernel Mailing List , Linux Memory Management List , KUnit Development Content-Type: text/plain; charset="UTF-8" X-Stat-Signature: ncpm4mdxtbjgj87wjj4q9dwyw5ba8ooy X-Rspamd-Server: rspam04 X-Rspamd-Queue-Id: 0E22980192DD Received-SPF: none (google.com>: No applicable sender policy available) receiver=imf16; identity=mailfrom; envelope-from=""; helo=mail-ot1-f49.google.com; client-ip=209.85.210.49 X-HE-DKIM-Result: pass/pass X-HE-Tag: 1617312284-248905 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, 1 Apr 2021 at 21:04, Daniel Latypov wrote: ... > > > --- a/include/linux/slub_def.h > > > +++ b/include/linux/slub_def.h > > > @@ -133,6 +133,8 @@ struct kmem_cache { > > > unsigned int usersize; /* Usercopy region size */ > > > > > > struct kmem_cache_node *node[MAX_NUMNODES]; > > > + > > > + int errors; /* Number of errors in cache */ > > > > So, I think it's bad design to add a new field 'errors', just for the > > test. This will increase kmem_cache size for all builds, which is > > unnecessary. > > > > Is there use to retrieve 'errors' elsewhere? > > > > While you could guard this with #ifdef CONFIG_SLUB_DEBUG or so, there's > > a better design option if this is just for the KUnit test's benefit: use > > kunit_resource. > > > > The way it'd work is that for each test (you can add a common init > > function) you add a named resource, in this case just an 'int' I guess, > > that slab would be able to retrieve if this test is being run. > > > > In the test somewhere, you could add something like this: > > > > > > static struct kunit_resource resource; > > static int slab_errors; > > > > .......... > > > > static int test_init(struct kunit *test) > > { > > slab_errors = 0; > > kunit_add_named_resource(test, NULL, NULL, &resource, > > "slab_errors", &slab_errors); > > return 0; > > } > > > > ...... tests now check slab_errors ..... > > > > and then in slub.c you'd have: > > > > #if IS_ENABLED(CONFIG_KUNIT) > > static bool slab_add_kunit_errors(void) > > { > > struct kunit_resource *resource; > > > > if (likely(!current->kunit_test)) > > return false; > > resource = kunit_find_named_resource(current->kunit_test, "slab_errors"); > > if (!resource) > > return false; > > (*(int *)resource->data)++; > > kunit_put_resource(resource); return true; was missing. > > } > > #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. Thanks, -- Marco