Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 15 Jul 2021 14:31:36 -0700
Message-Id: <20210715213138.1363079-1-dlatypov@google.com>
Mime-Version: 1.0
Subject: [RFC v1 0/2] crypto: tcrypt: small changes to run under KUnit
From:   Daniel Latypov <dlatypov@google.com>
To:     herbert@gondor.apana.org.au
Cc:     davem@davemloft.net, linux-crypto@vger.kernel.org,
        brendanhiggins@google.com, davidgow@google.com,
        linux-kernel@vger.kernel.org, kunit-dev@googlegroups.com,
        Daniel Latypov <dlatypov@google.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

tcrypt.c calls itself a "[q]uick & dirty crypto testing module."
One that "will only exist until we have a better testing mechanism."

This RFC seeks to start a discussion if KUnit can fill the role of that
"better testing mechanism."

As-is, these example changes don't make the test code much cleaner.
But they do provide a new way of running the test that's hopefully more
accessible, namely
$ ./tools/testing/kunit/kunit.py run --kunitconfig=crypto
...
[16:53:16] Starting KUnit Kernel ...
[16:53:19] ============================================================
[16:53:19] ======== [PASSED] tcrypt ========
[16:53:19] [PASSED] tcrypt
[16:53:19] ============================================================
[16:53:19] Testing complete. 1 tests run. 0 failed. 0 crashed. 0 skipped.
[16:53:19] Elapsed time: 8.806s total, 0.001s configuring, 5.764s building, 0.000s running

This series contains
* an initial patch with the boilerplate needed to run
under KUnit and track pass/fail status in the `test` context object
instead of passing around an int.
* another change to plumb the `test` context object to other test cases
that could previously fail w/o affecting the overall pass/fail status.

I haven't reformatted the code for now just to make the changes a bit
easier to read and skim over. checkpatch.pl isn't happy about the
spacing in the second patch.

== Other pros ==

If we want, we can go down the path of trying to structure the tests in
more idiomatic KUnit fashion to get some benefits like 
* automatically freeing memory at the end of tests when allocated using
  kunit_kmalloc() and friends instead of having to maintain labels
  * can be made to call arbitrary cleanup functions as well
  * this hopefully makes the tests more readable, at the expense of a
  bit more runtime overhead dynamically tracking these allocations
  * doing this just for the kmalloc's and __get_free_page() directly in
  * tcrypt.c saves 100+ lines of code.

* flagging test cases with KASAN issues (and eventually UBSAN)

* a bit easier way to dynamically run subsets of tests via glob
  * e.g. kunit.py run 'crypto.*sha512*'
  * KUnit currently only supports filtering by the suite ("crypto")
  right now, but we should have test-level filtering support soonish.

== Cons ==

The main cons are we'd be slightly changing how these tests are built
and run with these example patches

These changes are mainly
* building the test now requires CONFIG_KUNIT
* Running `insmod` on the module will always return 0, even if tests
  failed
  * the test instead prints out (K)TAP output to denote pass/fail
  * this is the format kselftest uses, but it's not fully standardized

And if we eventually try to restructure the test as mentioned above:
* more disruptive changes to how the tests are run
  * we'd have to move away from using the "mode" parameter
* a decent amount of code churn

Daniel Latypov (2):
  crypto: tcrypt: minimal conversion to run under KUnit
  crypto: tcrypt: call KUNIT_FAIL() instead of pr_err()

 crypto/Kconfig  |    5 +-
 crypto/tcrypt.c | 1063 +++++++++++++++++++++++------------------------
 2 files changed, 524 insertions(+), 544 deletions(-)


base-commit: e9338abf0e186336022293d2e454c106761f262b
-- 
2.32.0.402.g57bb445576-goog