Re: [PATCH v2 0/5] test_hash.c: refactor into KUnit

[Brendan Higgins]

+Shuah Khan

On Sat, Oct 2, 2021 at 12:30 AM David Gow <davidgow@xxxxxxxxxx> wrote:
>
> On Mon, Sep 27, 2021 at 6:33 AM Isabella Basso <isabellabdoamaral@xxxxxx> wrote:
> >
> > We refactored the lib/test_hash.c file into KUnit as part of the student
> > group LKCAMP [1] introductory hackathon for kernel development.
> >
> > This test was pointed to our group by Daniel Latypov [2], so its full
> > conversion into a pure KUnit test was our goal in this patch series, but
> > we ran into many problems relating to it not being split as unit tests,
> > which complicated matters a bit, as the reasoning behind the original
> > tests is quite cryptic for those unfamiliar with hash implementations.
> >
> > Some interesting developments we'd like to highlight are:
> >
> > - In patch 1/5 we noticed that there was an unused define directive that
> >   could be removed.
> > - In patch 4/5 we noticed how stringhash and hash tests are all under
> >   the lib/test_hash.c file, which might cause some confusion, and we
> >   also broke those kernel config entries up.
> >
> > Overall KUnit developments have been made in the other patches in this
> > series:
> >
> > In patches 2/5, 3/5 and 5/5 we refactored the lib/test_hash.c
> > file so as to make it more compatible with the KUnit style, whilst
> > preserving the original idea of the maintainer who designed it (i.e.
> > George Spelvin), which might be undesirable for unit tests, but we
> > assume it is enough for a first patch.
> >
> > This is our first patch series so we hope our contributions are
> > interesting and also hope to get some useful criticism from the
> > community. :)
> >
> > Changes since V1:
> > - Fixed compilation on parisc and m68k.
> > - Fixed whitespace mistakes.
> > - Renamed a few functions.
> > - Refactored globals into struct for test function params, thus removing
> >   a patch.
> > - Reworded some commit messages.
> >
> > [1] - https://lkcamp.dev/
> > [2] - https://lore.kernel.org/linux-kselftest/CAGS_qxojszgM19u=3HLwFgKX5bm5KhywvsSunuBAt5RtR+GyxQ@xxxxxxxxxxxxxx/
> >
>
> Thanks: I've gone through this new revision, and it still works fine,
> and my prior comments have been addressed. The commit messages in
> particular are much clearer, thank you! I've reviewed the various
> patches and left a few comments here and there, but there's nothing
> too drastic.
>
> I'm pretty happy with this from the KUnit side, but it would be ideal
> if someone with more knowledge of the hash functions looked over it.
> Unfortunately, George's email is bouncing, and no-one else has made
> any particularly major changes to this.
>
> My only remaining comment on the tests themselves is that it'd be nice
> to have them split up further into more, smaller tests. Given that
> it's a port of an existing test, though, I understand the desire not
> to change things too drastically.
>
> We also need to work out how this is going to go upstream: does it go
> through the kunit branch (via Shuah's kselftest repo), or directly to
> Linus (who's handled most of the other recent-ish changes here.
> Brendan, any thoughts?

I think Shuah should take them in 5.16.

Shuah, let me know if you are OK taking these in 5.16 and I will
update the patch tracker.

> Cheers,
> -- David
>
>
>
> > Isabella Basso (5):
> >   hash.h: remove unused define directive
> >   test_hash.c: split test_int_hash into arch-specific functions
> >   test_hash.c: split test_hash_init
> >   lib/Kconfig.debug: properly split hash test kernel entries
> >   test_hash.c: refactor into kunit
> >
> >  include/linux/hash.h       |   5 +-
> >  lib/Kconfig.debug          |  28 ++++-
> >  lib/Makefile               |   3 +-
> >  lib/test_hash.c            | 247 +++++++++++++++++--------------------
> >  tools/include/linux/hash.h |   5 +-
> >  5 files changed, 139 insertions(+), 149 deletions(-)
> >
> > --
> > 2.33.0
> >