lkml.org 
[lkml]   [2021]   [Oct]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v2 0/5] test_hash.c: refactor into KUnit
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

\
 
 \ /
  Last update: 2021-10-28 18:49    [W:3.937 / U:2.032 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site