| Message ID | 20210926223322.848641-1-isabellabdoamaral@usp.br (mailing list archive) |
|---|---|
| Headers | show |
| Series | test_hash.c: refactor into KUnit | expand |
On Mon, Sep 27, 2021 at 6:33 AM Isabella Basso <isabellabdoamaral@usp.br> 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@mail.gmail.com/ > 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? 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 >
+Shuah Khan On Sat, Oct 2, 2021 at 12:30 AM David Gow <davidgow@google.com> wrote: > > On Mon, Sep 27, 2021 at 6:33 AM Isabella Basso <isabellabdoamaral@usp.br> 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@mail.gmail.com/ > > > > 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 > >
Hi all, On Tue, Oct 5, 2021 at 6:19 PM Brendan Higgins <brendanhiggins@google.com> wrote: > > +Shuah Khan > > On Sat, Oct 2, 2021 at 12:30 AM David Gow <davidgow@google.com> wrote: > > > > On Mon, Sep 27, 2021 at 6:33 AM Isabella Basso <isabellabdoamaral@usp.br> 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@mail.gmail.com/ > > > > > > > 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