Hi all, On Tue, Oct 5, 2021 at 6:19 PM Brendan Higgins <brendanhiggins@xxxxxxxxxx> wrote: > > +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. I'm glad to hear this :) I'd also like to point out that "George Spelvin" is a rather unusual figure [3] so we shouldn't be expecting much back from him, anyway. Maybe I should've looked that up before trying to tackle this patch, as I've no idea who might be able to properly review the hash part of it. > > > > 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. Thanks for your comprehension! > > > > 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. > Thanks a lot for your interest in this patch :) We were a little worried about who might be able to get it upstream, so we appreciate that you also thought of this! > > 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 > > > I'm sorry for the delay in my response, but I think I can send a V3 soon, probably until next week. [3] - https://lwn.net/Articles/688216/ Cheers, -- Isabella Basso
