Re: [PATCH] wireguard: convert selftest/{counter,ratelimiter}.c to KUnit

From: Daniel Latypov
Date: Mon Oct 19 2020 - 19:31:04 EST

Next message: Benjamin Herrenschmidt: "Re: [PATCH v1 1/2] net: ftgmac100: move phy connect out from ftgmac100_setup_mdio"
Previous message: Benjamin Herrenschmidt: "Re: [PATCH 1/4] ftgmac100: Fix race issue on TX descriptor[0]"
In reply to: Jason A. Donenfeld: "Re: [PATCH] wireguard: convert selftest/{counter,ratelimiter}.c to KUnit"
Next in thread: kernel test robot: "Re: [PATCH] wireguard: convert selftest/{counter,ratelimiter}.c to KUnit"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Mon, Oct 19, 2020 at 3:36 PM Jason A. Donenfeld <Jason@xxxxxxxxx> wrote:
>
> Hi Daniel,
>
> Thanks for this patch. KUnit looks interesting. But I'm not totally
> sure what this would gain here, except more complicated infrastructure
> to handle. We're already running these tests in our CI on every single
> commit made to a variety of trees on a variety of architectures -- see
> https://www.wireguard.com/build-status/ -- runnable via `make -C
> tools/testing/selftests/wireguard/qemu -j$(nproc)`. It looks like this
> commit breaks that while making everything slightly more complex. Is

Thanks for the informative response and the pointer to the
build-status/ page, I was unaware of its existence.
Digging into that a bit deeper, I'd agree with you that this patch
isn't worth it.

Yes, this would stop these two tests from running under selftests, and
thus in your CI.
A not-too-long glance at the code made it seem like the specific
code-under-test here was reasonably arch-independent, but yes, this
would make it more annoying to test different arches.

> there a good reason to switch over to this other than fad? From a
> development perspective, I don't see this as really helping with much.

In my mind, the breakdown is

Pros:
* more minimal environment
* config file is 6 lines, instead of 87
* doesn't rely on a userspace or a custom init, etc.
* slightly faster build times on my machine (with -j8)
* the option to provide a bit more structure via its MACROS
* but that's optional, can fall back to `if (success) KUNIT_FAIL("my
message")`

Cons:
* separate set of tooling needed to run tests
* needs to be then integrated into WireGuard's CI
* not as mature, so it lacks integration via KernelCI, etc.
* Brendan (CC'd) is working on this KernelCI integration in particular.
* qemu/init.c has more features that KUnit currently lacks, like kmemleak checks
* and others I'm not able to think of.

WG's tooling is really nice, so these cons are much more apparent.

I think a feature that might make this worth looking at again later on
is if selftest modules could be written using KUnit.
Commit c475c77d5b56 ("kunit: allow kunit tests to be loaded as a
module") was necessary but not sufficient here.

If that becomes possible, KUnit would mainly provide the boilerplate
for tracking pass-fail, generating error messages, and the *option* of
running the tests via either KUnit or Kselftest.
Until that time, I'd agree that WG is better off as-is.

Cheers,
Daniel

>
> Jason
>
> On Mon, Oct 19, 2020 at 10:24 PM Daniel Latypov <dlatypov@xxxxxxxxxx> wrote:
> >
> > These tests already focus on testing individual functions that can run
> > in a more minimal environment like KUnit.
> >
> > The primary motivation for this change it to make it faster and easier
> > to run these tests, and thus encourage the addition of more test cases.
> >
> > E.g.
> > Test timing after make mrproper: 47.418s building, 0.000s running
> > With an incremental build: 3.891s building, 0.000s running
> >
> > KUnit also provides a bit more structure, like tracking overall
> > pass/fail status and printing failure messages like
> > > # wg_packet_counter_test: EXPECTATION FAILED at drivers/net/wireguard/counter_test.c:32
> > > Expected counter_validate(counter, (COUNTER_WINDOW_SIZE + 1)) == false, but
> >
> > Note: so we no longer need to track test_num in counter_test.c.
> > But deleting the /*1*/ test_num comments means git (with the default
> > threshold) no longer recognizes that the file was moved.
> >
> > Signed-off-by: Daniel Latypov <dlatypov@xxxxxxxxxx>
> > Cc: Jason A. Donenfeld <Jason@xxxxxxxxx>
> > Cc: David Miller <davem@xxxxxxxxxxxxx>
> > Cc: Brendan Higgins <brendanhiggins@xxxxxxxxxx>
> > ---
> > drivers/net/Kconfig | 12 ++++
> > .../{selftest/counter.c => counter_test.c} | 45 ++++++------
> > drivers/net/wireguard/main.c | 3 +-
> > drivers/net/wireguard/queueing.h | 4 --
> > drivers/net/wireguard/ratelimiter.c | 4 +-
> > .../ratelimiter.c => ratelimiter_test.c} | 68 +++++++++++--------
> > drivers/net/wireguard/receive.c | 6 +-
> > 7 files changed, 80 insertions(+), 62 deletions(-)
> > rename drivers/net/wireguard/{selftest/counter.c => counter_test.c} (73%)
> > rename drivers/net/wireguard/{selftest/ratelimiter.c => ratelimiter_test.c} (85%)
> >
> > diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
> > index c3dbe64e628e..208ed162bcc0 100644
> > --- a/drivers/net/Kconfig
> > +++ b/drivers/net/Kconfig
> > @@ -114,6 +114,18 @@ config WIREGUARD_DEBUG
> >
> > Say N here unless you know what you're doing.
> >
> > +config WIREGUARD_KUNIT_TEST
> > + tristate "KUnit tests for WireGuard"
> > + default KUNIT_ALL_TESTS
> > + depends on KUNIT && WIREGUARD
> > + help
> > + This enables KUnit tests for Wireguard.
> > +
> > + For more information on KUnit and unit tests in general please refer
> > + to the KUnit documentation in Documentation/dev-tools/kunit/.
> > +
> > + Say N here unless you know what you're doing.
> > +
> > config EQUALIZER
> > tristate "EQL (serial line load balancing) support"
> > help
> > diff --git a/drivers/net/wireguard/selftest/counter.c b/drivers/net/wireguard/counter_test.c
> > similarity index 73%
> > rename from drivers/net/wireguard/selftest/counter.c
> > rename to drivers/net/wireguard/counter_test.c
> > index ec3c156bf91b..167153fc249f 100644
> > --- a/drivers/net/wireguard/selftest/counter.c
> > +++ b/drivers/net/wireguard/counter_test.c
> > @@ -3,32 +3,23 @@
> > * Copyright (C) 2015-2019 Jason A. Donenfeld <Jason@xxxxxxxxx>. All Rights Reserved.
> > */
> >
> > -#ifdef DEBUG
> > -bool __init wg_packet_counter_selftest(void)
> > +#include <kunit/test.h>
> > +
> > +static void wg_packet_counter_test(struct kunit *test)
> > {
> > struct noise_replay_counter *counter;
> > - unsigned int test_num = 0, i;
> > - bool success = true;
> > + unsigned int i;
> >
> > - counter = kmalloc(sizeof(*counter), GFP_KERNEL);
> > - if (unlikely(!counter)) {
> > - pr_err("nonce counter self-test malloc: FAIL\n");
> > - return false;
> > - }
> > + counter = kunit_kmalloc(test, sizeof(*counter), GFP_KERNEL);
> > + KUNIT_ASSERT_NOT_ERR_OR_NULL(test, counter);
> >
> > #define T_INIT do { \
> > memset(counter, 0, sizeof(*counter)); \
> > spin_lock_init(&counter->lock); \
> > } while (0)
> > #define T_LIM (COUNTER_WINDOW_SIZE + 1)
> > -#define T(n, v) do { \
> > - ++test_num; \
> > - if (counter_validate(counter, n) != (v)) { \
> > - pr_err("nonce counter self-test %u: FAIL\n", \
> > - test_num); \
> > - success = false; \
> > - } \
> > - } while (0)
> > +#define T(n, v) \
> > + KUNIT_EXPECT_EQ(test, counter_validate(counter, n), v)
> >
> > T_INIT;
> > /* 1 */ T(0, true);
> > @@ -102,10 +93,18 @@ bool __init wg_packet_counter_selftest(void)
> > #undef T
> > #undef T_LIM
> > #undef T_INIT
> > -
> > - if (success)
> > - pr_info("nonce counter self-tests: pass\n");
> > - kfree(counter);
> > - return success;
> > }
> > -#endif
> > +
> > +static struct kunit_case wg_packet_counter_test_cases[] = {
> > + KUNIT_CASE(wg_packet_counter_test),
> > + {}
> > +};
> > +
> > +static struct kunit_suite wg_packet_counter_test_suite = {
> > + .name = "wg_packet_counter",
> > + .test_cases = wg_packet_counter_test_cases,
> > +};
> > +
> > +kunit_test_suites(&wg_packet_counter_test_suite);
> > +
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/net/wireguard/main.c b/drivers/net/wireguard/main.c
> > index 7a7d5f1a80fc..bfd3312d5133 100644
> > --- a/drivers/net/wireguard/main.c
> > +++ b/drivers/net/wireguard/main.c
> > @@ -22,8 +22,7 @@ static int __init mod_init(void)
> > int ret;
> >
> > #ifdef DEBUG
> > - if (!wg_allowedips_selftest() || !wg_packet_counter_selftest() ||
> > - !wg_ratelimiter_selftest())
> > + if (!wg_allowedips_selftest())
> > return -ENOTRECOVERABLE;
> > #endif
> > wg_noise_init();
> > diff --git a/drivers/net/wireguard/queueing.h b/drivers/net/wireguard/queueing.h
> > index dfb674e03076..5d428ddf176f 100644
> > --- a/drivers/net/wireguard/queueing.h
> > +++ b/drivers/net/wireguard/queueing.h
> > @@ -186,8 +186,4 @@ static inline void wg_queue_enqueue_per_peer_napi(struct sk_buff *skb,
> > wg_peer_put(peer);
> > }
> >
> > -#ifdef DEBUG
> > -bool wg_packet_counter_selftest(void);
> > -#endif
> > -
> > #endif /* _WG_QUEUEING_H */
> > diff --git a/drivers/net/wireguard/ratelimiter.c b/drivers/net/wireguard/ratelimiter.c
> > index 3fedd1d21f5e..f7a7c48aee40 100644
> > --- a/drivers/net/wireguard/ratelimiter.c
> > +++ b/drivers/net/wireguard/ratelimiter.c
> > @@ -220,4 +220,6 @@ void wg_ratelimiter_uninit(void)
> > mutex_unlock(&init_lock);
> > }
> >
> > -#include "selftest/ratelimiter.c"
> > +#if IS_ENABLED(CONFIG_WIREGUARD_KUNIT_TEST)
> > +#include "ratelimiter_test.c"
> > +#endif
> > diff --git a/drivers/net/wireguard/selftest/ratelimiter.c b/drivers/net/wireguard/ratelimiter_test.c
> > similarity index 85%
> > rename from drivers/net/wireguard/selftest/ratelimiter.c
> > rename to drivers/net/wireguard/ratelimiter_test.c
> > index 007cd4457c5f..a49f508cccb2 100644
> > --- a/drivers/net/wireguard/selftest/ratelimiter.c
> > +++ b/drivers/net/wireguard/ratelimiter_test.c
> > @@ -3,8 +3,7 @@
> > * Copyright (C) 2015-2019 Jason A. Donenfeld <Jason@xxxxxxxxx>. All Rights Reserved.
> > */
> >
> > -#ifdef DEBUG
> > -
> > +#include <kunit/test.h>
> > #include <linux/jiffies.h>
> >
> > static const struct {
> > @@ -32,7 +31,7 @@ static __init unsigned int maximum_jiffies_at_index(int index)
> >
> > static __init int timings_test(struct sk_buff *skb4, struct iphdr *hdr4,
> > struct sk_buff *skb6, struct ipv6hdr *hdr6,
> > - int *test)
> > + int *test_num)
> > {
> > unsigned long loop_start_time;
> > int i;
> > @@ -51,7 +50,7 @@ static __init int timings_test(struct sk_buff *skb4, struct iphdr *hdr4,
> > if (wg_ratelimiter_allow(skb4, &init_net) !=
> > expected_results[i].result)
> > return -EXFULL;
> > - ++(*test);
> > + ++(*test_num);
> >
> > hdr4->saddr = htonl(ntohl(hdr4->saddr) + i + 1);
> > if (time_is_before_jiffies(loop_start_time +
> > @@ -59,7 +58,7 @@ static __init int timings_test(struct sk_buff *skb4, struct iphdr *hdr4,
> > return -ETIMEDOUT;
> > if (!wg_ratelimiter_allow(skb4, &init_net))
> > return -EXFULL;
> > - ++(*test);
> > + ++(*test_num);
> >
> > hdr4->saddr = htonl(ntohl(hdr4->saddr) - i - 1);
> >
> > @@ -72,7 +71,7 @@ static __init int timings_test(struct sk_buff *skb4, struct iphdr *hdr4,
> > if (wg_ratelimiter_allow(skb6, &init_net) !=
> > expected_results[i].result)
> > return -EXFULL;
> > - ++(*test);
> > + ++(*test_num);
> >
> > hdr6->saddr.in6_u.u6_addr32[0] =
> > htonl(ntohl(hdr6->saddr.in6_u.u6_addr32[0]) + i + 1);
> > @@ -81,7 +80,7 @@ static __init int timings_test(struct sk_buff *skb4, struct iphdr *hdr4,
> > return -ETIMEDOUT;
> > if (!wg_ratelimiter_allow(skb6, &init_net))
> > return -EXFULL;
> > - ++(*test);
> > + ++(*test_num);
> >
> > hdr6->saddr.in6_u.u6_addr32[0] =
> > htonl(ntohl(hdr6->saddr.in6_u.u6_addr32[0]) - i - 1);
> > @@ -95,7 +94,7 @@ static __init int timings_test(struct sk_buff *skb4, struct iphdr *hdr4,
> > }
> >
> > static __init int capacity_test(struct sk_buff *skb4, struct iphdr *hdr4,
> > - int *test)
> > + int *test_num)
> > {
> > int i;
> >
> > @@ -104,45 +103,45 @@ static __init int capacity_test(struct sk_buff *skb4, struct iphdr *hdr4,
> >
> > if (atomic_read(&total_entries))
> > return -EXFULL;
> > - ++(*test);
> > + ++(*test_num);
> >
> > for (i = 0; i <= max_entries; ++i) {
> > hdr4->saddr = htonl(i);
> > if (wg_ratelimiter_allow(skb4, &init_net) != (i != max_entries))
> > return -EXFULL;
> > - ++(*test);
> > + ++(*test_num);
> > }
> > return 0;
> > }
> >
> > -bool __init wg_ratelimiter_selftest(void)
> > +static void wg_ratelimiter_test(struct kunit *test)
> > {
> > enum { TRIALS_BEFORE_GIVING_UP = 5000 };
> > bool success = false;
> > - int test = 0, trials;
> > + int test_num = 0, trials;
> > struct sk_buff *skb4, *skb6 = NULL;
> > struct iphdr *hdr4;
> > struct ipv6hdr *hdr6 = NULL;
> >
> > if (IS_ENABLED(CONFIG_KASAN) || IS_ENABLED(CONFIG_UBSAN))
> > - return true;
> > + return;
> >
> > BUILD_BUG_ON(MSEC_PER_SEC % PACKETS_PER_SECOND != 0);
> >
> > if (wg_ratelimiter_init())
> > goto out;
> > - ++test;
> > + ++test_num;
> > if (wg_ratelimiter_init()) {
> > wg_ratelimiter_uninit();
> > goto out;
> > }
> > - ++test;
> > + ++test_num;
> > if (wg_ratelimiter_init()) {
> > wg_ratelimiter_uninit();
> > wg_ratelimiter_uninit();
> > goto out;
> > }
> > - ++test;
> > + ++test_num;
> >
> > skb4 = alloc_skb(sizeof(struct iphdr), GFP_KERNEL);
> > if (unlikely(!skb4))
> > @@ -151,7 +150,7 @@ bool __init wg_ratelimiter_selftest(void)
> > hdr4 = (struct iphdr *)skb_put(skb4, sizeof(*hdr4));
> > hdr4->saddr = htonl(8182);
> > skb_reset_network_header(skb4);
> > - ++test;
> > + ++test_num;
> >
> > #if IS_ENABLED(CONFIG_IPV6)
> > skb6 = alloc_skb(sizeof(struct ipv6hdr), GFP_KERNEL);
> > @@ -164,7 +163,7 @@ bool __init wg_ratelimiter_selftest(void)
> > hdr6->saddr.in6_u.u6_addr32[0] = htonl(1212);
> > hdr6->saddr.in6_u.u6_addr32[1] = htonl(289188);
> > skb_reset_network_header(skb6);
> > - ++test;
> > + ++test_num;
> > #endif
> >
> > for (trials = TRIALS_BEFORE_GIVING_UP;;) {
> > @@ -173,16 +172,16 @@ bool __init wg_ratelimiter_selftest(void)
> > ret = timings_test(skb4, hdr4, skb6, hdr6, &test_count);
> > if (ret == -ETIMEDOUT) {
> > if (!trials--) {
> > - test += test_count;
> > + test_num += test_count;
> > goto err;
> > }
> > msleep(500);
> > continue;
> > } else if (ret < 0) {
> > - test += test_count;
> > + test_num += test_count;
> > goto err;
> > } else {
> > - test += test_count;
> > + test_num += test_count;
> > break;
> > }
> > }
> > @@ -192,13 +191,13 @@ bool __init wg_ratelimiter_selftest(void)
> >
> > if (capacity_test(skb4, hdr4, &test_count) < 0) {
> > if (!trials--) {
> > - test += test_count;
> > + test_num += test_count;
> > goto err;
> > }
> > msleep(50);
> > continue;
> > }
> > - test += test_count;
> > + test_num += test_count;
> > break;
> > }
> >
> > @@ -216,11 +215,20 @@ bool __init wg_ratelimiter_selftest(void)
> > /* Uninit one extra time to check underflow detection. */
> > wg_ratelimiter_uninit();
> > out:
> > - if (success)
> > - pr_info("ratelimiter self-tests: pass\n");
> > - else
> > - pr_err("ratelimiter self-test %d: FAIL\n", test);
> > -
> > - return success;
> > + if (!success)
> > + KUNIT_FAIL(test, "test #%d failed", test_num);
> > }
> > -#endif
> > +
> > +static struct kunit_case wg_ratelimiter_test_cases[] = {
> > + KUNIT_CASE(wg_ratelimiter_test),
> > + {}
> > +};
> > +
> > +static struct kunit_suite wg_ratelimiter_test_suite = {
> > + .name = "wg_ratelimiter",
> > + .test_cases = wg_ratelimiter_test_cases,
> > +};
> > +
> > +kunit_test_suites(&wg_ratelimiter_test_suite);
> > +
> > +MODULE_LICENSE("GPL v2");
> > diff --git a/drivers/net/wireguard/receive.c b/drivers/net/wireguard/receive.c
> > index 2c9551ea6dc7..30d3d9685e8d 100644
> > --- a/drivers/net/wireguard/receive.c
> > +++ b/drivers/net/wireguard/receive.c
> > @@ -336,8 +336,6 @@ static bool counter_validate(struct noise_replay_counter *counter, u64 their_cou
> > return ret;
> > }
> >
> > -#include "selftest/counter.c"
> > -
> > static void wg_packet_consume_data_done(struct wg_peer *peer,
> > struct sk_buff *skb,
> > struct endpoint *endpoint)
> > @@ -588,3 +586,7 @@ void wg_packet_receive(struct wg_device *wg, struct sk_buff *skb)
> > err:
> > dev_kfree_skb(skb);
> > }
> > +
> > +#if IS_ENABLED(CONFIG_WIREGUARD_KUNIT_TEST)
> > +#include "counter_test.c"
> > +#endif
> >
> > base-commit: 7cf726a59435301046250c42131554d9ccc566b8
> > --
> > 2.29.0.rc1.297.gfa9743e501-goog

Next message: Benjamin Herrenschmidt: "Re: [PATCH v1 1/2] net: ftgmac100: move phy connect out from ftgmac100_setup_mdio"
Previous message: Benjamin Herrenschmidt: "Re: [PATCH 1/4] ftgmac100: Fix race issue on TX descriptor[0]"
In reply to: Jason A. Donenfeld: "Re: [PATCH] wireguard: convert selftest/{counter,ratelimiter}.c to KUnit"
Next in thread: kernel test robot: "Re: [PATCH] wireguard: convert selftest/{counter,ratelimiter}.c to KUnit"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]