Received: by 2002:a25:f815:0:0:0:0:0 with SMTP id u21csp4067493ybd; Tue, 25 Jun 2019 13:29:46 -0700 (PDT) X-Google-Smtp-Source: APXvYqwMnRUNnyoE8yWR2bezZXK/tlah8zurhn08/zUGBcj1olnWMn0BpgXzkfZ6Np9aXvTndvwt X-Received: by 2002:a17:90a:be0a:: with SMTP id a10mr761919pjs.112.1561494586022; Tue, 25 Jun 2019 13:29:46 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1561494586; cv=none; d=google.com; s=arc-20160816; b=v/LxZ+6wPK8bJ0O6HCg2n9JYnEEBURD4fTWnjxlmRO6Sgj6ex+seqJ/KCwF53mJpFf uXTv2PSe0P9GR/dSwfRToMh4hBMJOn1soL82fCOGpjHagVuz5C0zoB+H14c8OM5oHkjM EwvgRwZOEFwjnkY/rkZip0k0cN/vYKLuz4DysMnVnAUAD9XoFSLgt1iHgP6JtNNqTZNq kgMGnS9HLu9aXKCRTzVbRYwlXV7rb+c0lOFFk0D0vYF8tTlykM6sMjKlQ1zUsNvdWtDI pZwpmGVTUDVjRo2xcFo679DdOhhvxv2QBxLv3vSeaqtUYim2qBjzs+BtFcwzlm3tVVJP yBzw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:dkim-signature; bh=qq39ykktkvFBpmzRtbgM4tstWpOedFNE8kr26lh/InM=; b=DWH8u8Kq9e2IfdDVls4l3YFD3NNPq2HATjFvtPPr+TsRk0RWGsTQ8ouilnsnICsxJ6 EaayohVm4A+I3MZLCNJmD4WV8CIZyp/YKYFdAAo1YgcCXSTEyQghSutMyYRUMbSqsOvO SN7HREnIyMrI76WCtPVFS20cEXjiUtdZEJPD9WIr+u9JeX9pYj+A7VXypelhkdqazL7/ 5K8XHVnk5M5bUzRKpx0BZutxw34xVrXb1wXOG69iH+B7zjnmJcGu8MzGJI9aVHTYGXft Ux8WIJgiDZ+F9EgPzWIu3SGlhgQr23c3gnIumcQZKMZtC2o6kojNCyDYVuzq+BQMsvcH gDlw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@google.com header.s=20161025 header.b=ni94Owio; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id 36si13941176pgn.174.2019.06.25.13.29.25; Tue, 25 Jun 2019 13:29:46 -0700 (PDT) Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; dkim=pass header.i=@google.com header.s=20161025 header.b=ni94Owio; spf=pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=REJECT sp=REJECT dis=NONE) header.from=google.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728028AbfFYU2l (ORCPT + 99 others); Tue, 25 Jun 2019 16:28:41 -0400 Received: from mail-pg1-f194.google.com ([209.85.215.194]:44345 "EHLO mail-pg1-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726393AbfFYU2h (ORCPT ); Tue, 25 Jun 2019 16:28:37 -0400 Received: by mail-pg1-f194.google.com with SMTP id n2so9493431pgp.11 for ; Tue, 25 Jun 2019 13:28:37 -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=qq39ykktkvFBpmzRtbgM4tstWpOedFNE8kr26lh/InM=; b=ni94OwioZx/gzf2szVloCDZTFFpJCo2YKSFZ2X9qr6bxYnBeEpnnagfszyXnnz51L8 DkJXaethFUsoHnz8KOQVG2ZSAlP6R4YiehiVV+cY7fw97Xus3XNQzLPLdgQULmgYaWnL GEAS55DRith5+cQIVd4nDD9PnOpK5SEC0mcAjXyMYNE83XKQ2Jfbx9GzYoD6pnR9q88y VjAnVY+gy9KsY/4+Tbda/+4DRlDyvSks4s9chgBxuZrKHnC3t/s2Oqm9eCfJpWO+nPcJ ixgXgEVA9P9D5QKdeUq3VIirHZ16lxLB0iDdlrfqM9fruDTYYR+l+8zODcodp3flRnlv dArA== 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=qq39ykktkvFBpmzRtbgM4tstWpOedFNE8kr26lh/InM=; b=tqlzgClguFD4+8XxnQeiGYv9S1295tDjzjC1xb0WLThaawJ61DHnEls6UGLisewQik MLIu0ixvQnlDo/K/gH9k7YtkYzRtCID02W+770lckf76IVBVtzI9F0MsXncCE9P5ZEky Sk943FJ59SJfX242Ya2WRj0IunUvxJqYASy2tpLLVu6geQnvxwFTrLUw1DaCRDmyIRx8 M8wC2QKZz4RpYvFZf/55ha+fWvVhulG9p5/1WDKePzZ9JRIFUbo9GBApcmMFNp1EuTGt xHEP27/PUb/y6Hoi47b/7avS3Kw5wBv35r9qXzsScaLJgVnpvV7Oev9Po8OWxGT37p/Y OALg== X-Gm-Message-State: APjAAAV+8ZTKApiFYbnbeSNgLaEjzKG0yuSBTqzpMbxsEewg2LxJZ0Op wuN0K+l98LGlTKJ2Rs+VTyuR90Ignz9lAPx3jNo0Ag== X-Received: by 2002:a17:90b:f0e:: with SMTP id br14mr754020pjb.117.1561494516332; Tue, 25 Jun 2019 13:28:36 -0700 (PDT) MIME-Version: 1.0 References: <20190617082613.109131-1-brendanhiggins@google.com> <20190617082613.109131-2-brendanhiggins@google.com> <20190620001526.93426218BE@mail.kernel.org> In-Reply-To: <20190620001526.93426218BE@mail.kernel.org> From: Brendan Higgins Date: Tue, 25 Jun 2019 13:28:25 -0700 Message-ID: Subject: Re: [PATCH v5 01/18] kunit: test: add KUnit test runner core To: Stephen Boyd Cc: Frank Rowand , Greg KH , Josh Poimboeuf , Kees Cook , Kieran Bingham , Luis Chamberlain , Peter Zijlstra , Rob Herring , shuah , "Theodore Ts'o" , Masahiro Yamada , devicetree , dri-devel , kunit-dev@googlegroups.com, "open list:DOCUMENTATION" , linux-fsdevel@vger.kernel.org, linux-kbuild , Linux Kernel Mailing List , "open list:KERNEL SELFTEST FRAMEWORK" , linux-nvdimm , linux-um@lists.infradead.org, Sasha Levin , "Bird, Timothy" , Amir Goldstein , Dan Carpenter , Daniel Vetter , Jeff Dike , Joel Stanley , Julia Lawall , Kevin Hilman , Knut Omang , Logan Gunthorpe , Michael Ellerman , Petr Mladek , Randy Dunlap , Richard Weinberger , David Rientjes , Steven Rostedt , wfg@linux.intel.com Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Jun 19, 2019 at 5:15 PM Stephen Boyd wrote: > > Quoting Brendan Higgins (2019-06-17 01:25:56) > > diff --git a/kunit/test.c b/kunit/test.c > > new file mode 100644 > > index 0000000000000..d05d254f1521f > > --- /dev/null > > +++ b/kunit/test.c > > @@ -0,0 +1,210 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Base unit test (KUnit) API. > > + * > > + * Copyright (C) 2019, Google LLC. > > + * Author: Brendan Higgins > > + */ > > + > > +#include > > +#include > > + > > +static bool kunit_get_success(struct kunit *test) > > +{ > > + unsigned long flags; > > + bool success; > > + > > + spin_lock_irqsave(&test->lock, flags); > > + success = test->success; > > + spin_unlock_irqrestore(&test->lock, flags); > > I still don't understand the locking scheme in this code. Is the > intention to make getter and setter APIs that are "safe" by adding in a > spinlock that is held around getting and setting various members in the > kunit structure? Yes, your understanding is correct. It is possible for a user to write a test such that certain elements may be updated in different threads; this would most likely happen in the case where someone wants to make an assertion or an expectation in a thread created by a piece of code under test. Although this should generally be avoided, it is possible, and there are occasionally good reasons to do so, so it is functionality that we should support. Do you think I should add a comment to this effect? > In what situation is there more than one thread reading or writing the > kunit struct? Isn't it only a single process that is going to be As I said above, it is possible that the code under test may spawn a new thread that may make an expectation or an assertion. It is not a super common use case, but it is possible. > operating on this structure? And why do we need to disable irqs? Are we > expecting to be modifying the unit tests from irq contexts? There are instances where someone may want to test a driver which has an interrupt handler in it. I actually have (not the greatest) example here. Now in these cases, I expect someone to use a mock irqchip or some other fake mechanism to trigger the interrupt handler and not actual hardware; technically speaking in this case, it is not going to be accessed from a "real" irq context; however, the code under test should think that it is in an irq context; given that, I figured it is best to just treat it as a real irq context. Does that make sense? > > + > > + return success; > > +} > > + > > +static void kunit_set_success(struct kunit *test, bool success) > > +{ > > + unsigned long flags; > > + > > + spin_lock_irqsave(&test->lock, flags); > > + test->success = success; > > + spin_unlock_irqrestore(&test->lock, flags); > > +} > > + > > +static int kunit_vprintk_emit(int level, const char *fmt, va_list args) > > +{ > > + return vprintk_emit(0, level, NULL, 0, fmt, args); > > +} > > + > > +static int kunit_printk_emit(int level, const char *fmt, ...) > > +{ > > + va_list args; > > + int ret; > > + > > + va_start(args, fmt); > > + ret = kunit_vprintk_emit(level, fmt, args); > > + va_end(args); > > + > > + return ret; > > +} > > + > > +static void kunit_vprintk(const struct kunit *test, > > + const char *level, > > + struct va_format *vaf) > > +{ > > + kunit_printk_emit(level[1] - '0', "\t# %s: %pV", test->name, vaf); > > +} > > + > > +static bool kunit_has_printed_tap_version; > > Can you please move this into function local scope in the function > below? Sure, that makes sense. > > + > > +static void kunit_print_tap_version(void) > > +{ > > + if (!kunit_has_printed_tap_version) { > > + kunit_printk_emit(LOGLEVEL_INFO, "TAP version 14\n"); > > + kunit_has_printed_tap_version = true; > > + } > > +} > > + > [...] > > + > > +static bool kunit_module_has_succeeded(struct kunit_module *module) > > +{ > > + const struct kunit_case *test_case; > > + bool success = true; > > + > > + for (test_case = module->test_cases; test_case->run_case; test_case++) > > + if (!test_case->success) { > > + success = false; > > + break; > > Why not 'return false'? Also a good point. Will fix. > > + } > > + > > + return success; > > And 'return true'? Will fix. > > +} > > + > > +static size_t kunit_module_counter = 1; > > + > > +static void kunit_print_subtest_end(struct kunit_module *module) > > +{ > > + kunit_print_ok_not_ok(false, > > + kunit_module_has_succeeded(module), > > + kunit_module_counter++, > > + module->name); > > +} > > + > > +static void kunit_print_test_case_ok_not_ok(struct kunit_case *test_case, > > + size_t test_number) > > +{ > > + kunit_print_ok_not_ok(true, > > + test_case->success, > > + test_number, > > + test_case->name); > > +} > > + > > +void kunit_init_test(struct kunit *test, const char *name) > > +{ > > + spin_lock_init(&test->lock); > > + test->name = name; > > + test->success = true; > > +} > > + > > +/* > > + * Performs all logic to run a test case. > > + */ > > +static void kunit_run_case(struct kunit_module *module, > > + struct kunit_case *test_case) > > +{ > > + struct kunit test; > > + int ret = 0; > > + > > + kunit_init_test(&test, test_case->name); > > + > > + if (module->init) { > > + ret = module->init(&test); > > + if (ret) { > > + kunit_err(&test, "failed to initialize: %d\n", ret); > > + kunit_set_success(&test, false); > > + return; > > + } > > + } > > + > > + if (!ret) > > + test_case->run_case(&test); > > Do we need this if condition? ret can only be set to non-zero above but > then we'll exit the function early so it seems unnecessary. Given that, > ret should probably be moved into the module->init path. Whoops. Sorry, another instance of how it evolved over time and I forgot why I did the check. Will fix. > > + > > + if (module->exit) > > + module->exit(&test); > > + > > + test_case->success = kunit_get_success(&test); > > +} > > + Thanks!