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 Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 72530C433FE for ; Tue, 22 Nov 2022 02:21:56 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id C93D96B0071; Mon, 21 Nov 2022 21:21:55 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id C448D8E0002; Mon, 21 Nov 2022 21:21:55 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B0BD48E0001; Mon, 21 Nov 2022 21:21:55 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0014.hostedemail.com [216.40.44.14]) by kanga.kvack.org (Postfix) with ESMTP id A123F6B0071 for ; Mon, 21 Nov 2022 21:21:55 -0500 (EST) Received: from smtpin22.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 6C8C0120F15 for ; Tue, 22 Nov 2022 02:21:55 +0000 (UTC) X-FDA: 80159477790.22.B064331 Received: from mail-yw1-f171.google.com (mail-yw1-f171.google.com [209.85.128.171]) by imf10.hostedemail.com (Postfix) with ESMTP id 0187EC000B for ; Tue, 22 Nov 2022 02:21:54 +0000 (UTC) Received: by mail-yw1-f171.google.com with SMTP id 00721157ae682-3691e040abaso131224207b3.9 for ; Mon, 21 Nov 2022 18:21:54 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=DKbxaJtTyZO5Z9ZBbZTMOsOtk3yKuoTVReeAVGJ3Pdw=; b=th40tW4wJgE8ZiaU/N3/Vp/zUV3VRt5UMK0PdcOPvQHtQP2r/uyRVg0+OO5SlBIcQC jws9pp8MNuXe4Tdy0WIAs8zTg18CNBl9qIqzEuxRbUhiHcHEB1+BVhMCCq8zRb5l2NH7 LI9dCeopLGYOyUDchySF3vlWK2NmFiSreav5Ti8C6PO9iry70Du6sNSWK+12+1638g/8 iIy6Uylx15vNePLram5wH1FQjOjZFb382lZ5JQ8EbvOh9GFkaEL2x5MP+UIZui0LF4v/ CJzHTw3pV5UULKOtO66iJpIzm5PjlLCMgVVg2xs7UWrmuBuA691D6TI71Pg2psEwuZwL kZrw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=DKbxaJtTyZO5Z9ZBbZTMOsOtk3yKuoTVReeAVGJ3Pdw=; b=kWRjbJkdGNDs83Pi22+LqsOBPjlym23L9/XwlKcg6KbjO/nCxiH/M7rDedqjhgPhS+ 6qTtKE+ji4B/o8RG57qD+Ce5gZH+w4jbvu0ro+/Uar514mtacmqv/KWEcjxVARuLZxgr 9Y1a1Ax2Kk52N8yWvYmJqlca1kHcGbxNgV88v9ErdylkefK1nZm7/sMXGFEhGjCKbXEy MRT83lfWWsEftS5UU+fkIPFFmAoj2WeZ9eJKHJVJX7uTb95J2DfchI27b9DpYifnJw4L CoAR55EGjclqWDkDP//lj+S55xoFycE2QVesfUyEMmTfSWjcIy9RRFTPYzfqrquUuoL0 6vZA== X-Gm-Message-State: ANoB5plJQbIXDG9rIvgVz8PoyVkc1AQCnCceTSITTFGjFaW/RGfTRTR8 AXNaEeiQvJibtjfY3Yijgu27aEa+LYoX5qejPNlL9g== X-Google-Smtp-Source: AA0mqf6ObbGFOG0xl7JeJ2WAVW0SXydm9VOOQCDq40H+EH4fN1M84S3y4P3BkTpJTxJHwIE4iR7GZri9IbvoRetEYZE= X-Received: by 2002:a81:7b89:0:b0:388:dd9b:f3ee with SMTP id w131-20020a817b89000000b00388dd9bf3eemr2279367ywc.164.1669083714099; Mon, 21 Nov 2022 18:21:54 -0800 (PST) MIME-Version: 1.0 References: <20221119081252.3864249-1-davidgow@google.com> <20221119081252.3864249-2-davidgow@google.com> In-Reply-To: <20221119081252.3864249-2-davidgow@google.com> From: Daniel Latypov Date: Mon, 21 Nov 2022 18:21:43 -0800 Message-ID: Subject: Re: [PATCH v3 2/3] kunit: Use the static key when retrieving the current test To: David Gow Cc: Brendan Higgins , Shuah Khan , kunit-dev@googlegroups.com, linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org, Kees Cook , linux-doc@vger.kernel.org, linux-mm@kvack.org, Jonathan Corbet , Sadiya Kazi Content-Type: text/plain; charset="UTF-8" ARC-Authentication-Results: i=1; imf10.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=th40tW4w; spf=pass (imf10.hostedemail.com: domain of dlatypov@google.com designates 209.85.128.171 as permitted sender) smtp.mailfrom=dlatypov@google.com; dmarc=pass (policy=reject) header.from=google.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1669083715; a=rsa-sha256; cv=none; b=VvPKLaLvxvRmTAGBGt4TVqfh3yIYxedWKKlNigoAU7NcXeLoaVaRaWM8g2iWmZtek8X9LR 618lJ761VMlUncFvTRE6mUuLA3GnD1sywpFD5vbwGdrujbpisXmKYWOLvsGrFvBhP61zC+ UXPmrQjrdp+oUdbJftKBvsEVrnvwplU= ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1669083715; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=DKbxaJtTyZO5Z9ZBbZTMOsOtk3yKuoTVReeAVGJ3Pdw=; b=eWNlnTCUkF945gA5fBZtAO1IAVYs7fS/veKZ1OCUGaEtIXcbsr/ME0nlEvP/A0PW00EpsO sU8N+tgv1otmul5GovR+nkWtQDvz67FfewY9Eip6rwoSkOj4jl+XQEamW9To5FaVg67RC9 W13ykJwL64wg8ltyX48Ue2g0tBdV478= X-Rspam-User: Authentication-Results: imf10.hostedemail.com; dkim=pass header.d=google.com header.s=20210112 header.b=th40tW4w; spf=pass (imf10.hostedemail.com: domain of dlatypov@google.com designates 209.85.128.171 as permitted sender) smtp.mailfrom=dlatypov@google.com; dmarc=pass (policy=reject) header.from=google.com X-Stat-Signature: fd35hp6emmksb8xpnzs7otpjgpgqn4jy X-Rspamd-Queue-Id: 0187EC000B X-Rspamd-Server: rspam09 X-HE-Tag: 1669083714-218200 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 Sat, Nov 19, 2022 at 12:13 AM 'David Gow' via KUnit Development wrote: > > In order to detect if a KUnit test is running, and to access its > context, the 'kunit_test' member of the current task_struct is used. > Usually, this is accessed directly or via the kunit_fail_current_task() > function. > > In order to speed up the case where no test is running, add a wrapper, > kunit_get_current_test(), which uses the static key to fail early. > Equally, Speed up kunit_fail_current_test() by using the static key. > > This should make it convenient for code to call this > unconditionally in fakes or error paths, without worrying that this will > slow the code down significantly. > > If CONFIG_KUNIT=n (or m), this compiles away to nothing. If > CONFIG_KUNIT=y, it will compile down to a NOP (on most architectures) if > no KUnit test is currently running. > > Note that kunit_get_current_test() does not work if KUnit is built as a > module. This mirrors the existing restriction on kunit_fail_current_test(). > > Note that the definition of kunit_fail_current_test() still wraps an > empty, inline function if KUnit is not built-in. This is to ensure that > the printf format string __attribute__ will still work. > > Also update the documentation to suggest users use the new > kunit_get_current_test() function, update the example, and to describe > the behaviour when KUnit is disabled better. > > Cc: Jonathan Corbet > Cc: Sadiya Kazi > Signed-off-by: David Gow Reviewed-by: Daniel Latypov Looks good to me, but some small optional nits about the Documentation. I'm a bit sad that the kunit_fail_current_test() macro is now a bit more complicated (has two definitions). Optional: perhaps it's long enough now that we should have a comment after the #endif, i.e. #endif /* IS_BUILTIN(CONFIG_KUNIT) */ > > diff --git a/Documentation/dev-tools/kunit/usage.rst b/Documentation/dev-tools/kunit/usage.rst > index 2737863ef365..e70014b82350 100644 > --- a/Documentation/dev-tools/kunit/usage.rst > +++ b/Documentation/dev-tools/kunit/usage.rst > @@ -625,17 +625,21 @@ as shown in next section: *Accessing The Current Test*. > Accessing The Current Test > -------------------------- > > -In some cases, we need to call test-only code from outside the test file. > -For example, see example in section *Injecting Test-Only Code* or if > -we are providing a fake implementation of an ops struct. Using > -``kunit_test`` field in ``task_struct``, we can access it via > -``current->kunit_test``. > +In some cases, we need to call test-only code from outside the test file, > +for example, when providing a fake implementation of a function, or to fail nit: there are two spaces after "for example, " Personal preference: I'd rather keep "For example," as the start of a new sentence. > +any current test from within an error handler. > +We can do this via the ``kunit_test`` field in ``task_struct``, which we can > +access using the ``kunit_get_current_test`` function in ``kunit/test-bug.h``. Personal preference: kunit_get_current_test() IMO that would make it easier to pick up when the reader is quickly scanning over. > > -The example below includes how to implement "mocking": > +``kunit_get_current_test`` requires KUnit be built-in to the kernel, i.e. > +``CONFIG_KUNIT=y``. It is safe to call even if KUnit is not enabled, is built as a module, > +or no test is currently running, in which case it will quickly return ``NULL``. I find this sentence a bit confusing. I think it's trying to convey that * it can be called no matter how the kernel is built or what cmdline args are given * but it doesn't work properly for CONFIG_KUNIT=m * for CONFIG_KUNIT=n, it's a static inline func that just returns NULL * when booting with `kunit.enabled=0`, it's fast (thanks to static keys) But the current wording basically says "the func requires CONFIG_KUNIT=y" then says it's safe to call it even if CONFIG_KUNIT=n. It feels a bit whiplashy. Should this be reworded to say the function can be used regardless of whether KUnit is enabled but add a `note` block about how it doesn't properly for CONFIG_KUNIT=m? > + > +The example below uses this to implement a "mock" implementation of a function, ``foo``: > > .. code-block:: c > > - #include /* for current */ > + #include /* for kunit_get_current_test */ > > struct test_data { > int foo_result; > @@ -644,7 +648,7 @@ The example below includes how to implement "mocking": > > static int fake_foo(int arg) > { > - struct kunit *test = current->kunit_test; > + struct kunit *test = kunit_get_current_test(); > struct test_data *test_data = test->priv; > > KUNIT_EXPECT_EQ(test, test_data->want_foo_called_with, arg); > @@ -675,7 +679,7 @@ Each test can have multiple resources which have string names providing the same > flexibility as a ``priv`` member, but also, for example, allowing helper > functions to create resources without conflicting with each other. It is also > possible to define a clean up function for each resource, making it easy to > -avoid resource leaks. For more information, see Documentation/dev-tools/kunit/api/test.rst. > +avoid resource leaks. For more information, see Documentation/dev-tools/kunit/api/resource.rst. Oops, thanks for cleaning this up. I guess I forgot to update this when splitting out resource.rst or my change raced with the rewrite of this file. > > Failing The Current Test > ------------------------ > @@ -703,3 +707,6 @@ structures as shown below: > static void my_debug_function(void) { } > #endif > > +Note that ``kunit_fail_current_test`` requires KUnit be built-in to the kernel, i.e. > +``CONFIG_KUNIT=y``. It is safe to call even if KUnit is not enabled, is built as a module, > +or no test is currently running, but will do nothing. This is the same wording as above. I think it's clearer since what it's trying to convey is simpler, so it's probably fine. But if we do end up thinking of a good way to reword the previous bit, we might want to reword it here too.