new kunit infrastructure

new kunit infrastructure

[Johannes Berg]

Hi all,

Is there an established process for new kunit infrastructure?

For example, we have this macro that makes KUNIT_ARRAY_PARAM easier by
letting you just declare an array of test cases:

/* Similar to KUNIT_ARRAY_PARAM, but avoiding an extra function */
#define KUNIT_ARRAY_PARAM_DESC(name, array, desc_member)					\
	static const void *name##_gen_params(const void *prev, char *desc)			\
	{											\
		typeof((array)[0]) *__next = prev ? ((typeof(__next)) prev) + 1 : (array);	\
		if (__next - (array) < ARRAY_SIZE((array))) {					\
			strscpy(desc, __next->desc_member, KUNIT_PARAM_DESC_SIZE);		\
			return __next;								\
		}										\
		return NULL;									\
	}


Also, since we're working on wifi and thus networking, we want e.g. SKBs
to be resource-managed, and added some helper macros/functions for using
kunit_alloc_resource() with SKBs, that will be used at least in cfg80211
and mac80211 soon, so it would seem appropriate to have
include/kunit/skb.h (and a corresponding C file somewhere) with these
helpers.


Is all of this just a case of "nobody needed it so far", or is there no
expectation to add such infrastructure more generally?

johannes

Re: new kunit infrastructure

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 3166 bytes --]

On Tue, 28 Mar 2023 at 18:35, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> Hi all,
>
> Is there an established process for new kunit infrastructure?

Hi Johannes,

"established process" is probably overselling it, but we're more than
happy to accept improvements and additions to KUnit.

>
> For example, we have this macro that makes KUNIT_ARRAY_PARAM easier by
> letting you just declare an array of test cases:
>
> /* Similar to KUNIT_ARRAY_PARAM, but avoiding an extra function */
> #define KUNIT_ARRAY_PARAM_DESC(name, array, desc_member)                                        \
>         static const void *name##_gen_params(const void *prev, char *desc)                      \
>         {                                                                                       \
>                 typeof((array)[0]) *__next = prev ? ((typeof(__next)) prev) + 1 : (array);      \
>                 if (__next - (array) < ARRAY_SIZE((array))) {                                   \
>                         strscpy(desc, __next->desc_member, KUNIT_PARAM_DESC_SIZE);              \
>                         return __next;                                                          \
>                 }                                                                               \
>                 return NULL;                                                                    \
>         }
>

Very neat! I'm more than happy to see this added.

>
> Also, since we're working on wifi and thus networking, we want e.g. SKBs
> to be resource-managed, and added some helper macros/functions for using
> kunit_alloc_resource() with SKBs, that will be used at least in cfg80211
> and mac80211 soon, so it would seem appropriate to have
> include/kunit/skb.h (and a corresponding C file somewhere) with these
> helpers.
>

We're definitely in favour of adding these sorts of helpers. Thus far,
these have mostly lived alongside the tests or subsystem being tested,
but if they're widely useful then they can sit alongside the KUnit
code.

My personal feeling is that it's better to have these sorts of
subsystem-specific helpers be written and maintained as part of the
subsystems (like the tests themselves), largely because that's where
the subsystem expertise lies, but we're definitely happy to review any
such patches to make sure they fit into the KUnit infrastructure well.
(And, of course, if something spans several subsystems, then lib/kunit
may be the best place to keep it.)

>
> Is all of this just a case of "nobody needed it so far", or is there no
> expectation to add such infrastructure more generally?
>

Yeah, it's a combination of "no-one has needed it yet", "no-one
working on KUnit understands it well enough", and "we haven't had the
time yet". We are a bit hesitant to add these features without having
tests that use them, too: often things will be coded by hand for one
or two tests, and only then refactored out into a common helper.

There are a few other similar helpers being worked on at the moment,
mostly around providing test-managed "struct device"s, so this is
definitely an active field of development.

Thanks,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]

Re: new kunit infrastructure

[Johannes Berg]

Hi David,

Thanks for the quick reply!

> "established process" is probably overselling it, but we're more than
> happy to accept improvements and additions to KUnit.

Yeah I guess I was expecting that. Was more wondering if you had
anything in mind other than sending it to kunit-dev/linux-kselftest
lists. I'll assume no for now :-)

> > For example, we have this macro that makes KUNIT_ARRAY_PARAM easier by
> > letting you just declare an array of test cases:
> > 
> > /* Similar to KUNIT_ARRAY_PARAM, but avoiding an extra function */
> > #define KUNIT_ARRAY_PARAM_DESC(name, array, desc_member)                                        \
> >         static const void *name##_gen_params(const void *prev, char *desc)                      \
> >         {                                                                                       \
> >                 typeof((array)[0]) *__next = prev ? ((typeof(__next)) prev) + 1 : (array);      \
> >                 if (__next - (array) < ARRAY_SIZE((array))) {                                   \
> >                         strscpy(desc, __next->desc_member, KUNIT_PARAM_DESC_SIZE);              \
> >                         return __next;                                                          \
> >                 }                                                                               \
> >                 return NULL;                                                                    \
> >         }
> > 
> 
> Very neat! I'm more than happy to see this added.

Credits to Benjamin, FWIW. We'll send a patch.

> We're definitely in favour of adding these sorts of helpers. Thus far,
> these have mostly lived alongside the tests or subsystem being tested,
> but if they're widely useful then they can sit alongside the KUnit
> code.
> 
> My personal feeling is that it's better to have these sorts of
> subsystem-specific helpers be written and maintained as part of the
> subsystems (like the tests themselves), largely because that's where
> the subsystem expertise lies, but we're definitely happy to review any
> such patches to make sure they fit into the KUnit infrastructure well.

Right, that all makes sense. But ...

> (And, of course, if something spans several subsystems, then lib/kunit
> may be the best place to keep it.)

Exactly. Even for wifi, being split between cfg80211 and mac80211 makes
things harder than they should be and less clean if it's part of
cfg80211 and then somehow exposed to other bits, and then possibly
refactored into somewhere else in net if that starts using it.

So I think in the case of test-managed SKBs, it would make sense to put
it into lib/kunit.

> Yeah, it's a combination of "no-one has needed it yet", "no-one
> working on KUnit understands it well enough", and "we haven't had the
> time yet". We are a bit hesitant to add these features without having
> tests that use them, too: often things will be coded by hand for one
> or two tests, and only then refactored out into a common helper.

Right, we're already at that place adding tests to cfg80211 and mac80211
now, basically.

> There are a few other similar helpers being worked on at the moment,
> mostly around providing test-managed "struct device"s, so this is
> definitely an active field of development.

Great.

I think we'll just send a couple of patches to start out with, for SKBs
and the test case array macro above.

Thanks!

johannes

Re: new kunit infrastructure

[David Gow]

[-- Attachment #1: Type: text/plain, Size: 4051 bytes --]

On Tue, 28 Mar 2023 at 20:54, Johannes Berg <johannes@sipsolutions.net> wrote:
>
> Hi David,
>
> Thanks for the quick reply!
>
> > "established process" is probably overselling it, but we're more than
> > happy to accept improvements and additions to KUnit.
>
> Yeah I guess I was expecting that. Was more wondering if you had
> anything in mind other than sending it to kunit-dev/linux-kselftest
> lists. I'll assume no for now :-)

Those lists (plus the lists / maintainers of anything the feature is
going to heavily interact with) are the right spot. There's also
#kunit on oftc, or feel free to reach out to Brendan or I to try to
sort out a video meeting if you'd rather something more real-time.

But the lists are probably best. :-)

>
> > > For example, we have this macro that makes KUNIT_ARRAY_PARAM easier by
> > > letting you just declare an array of test cases:
> > >
> > > /* Similar to KUNIT_ARRAY_PARAM, but avoiding an extra function */
> > > #define KUNIT_ARRAY_PARAM_DESC(name, array, desc_member)                                        \
> > >         static const void *name##_gen_params(const void *prev, char *desc)                      \
> > >         {                                                                                       \
> > >                 typeof((array)[0]) *__next = prev ? ((typeof(__next)) prev) + 1 : (array);      \
> > >                 if (__next - (array) < ARRAY_SIZE((array))) {                                   \
> > >                         strscpy(desc, __next->desc_member, KUNIT_PARAM_DESC_SIZE);              \
> > >                         return __next;                                                          \
> > >                 }                                                                               \
> > >                 return NULL;                                                                    \
> > >         }
> > >
> >
> > Very neat! I'm more than happy to see this added.
>
> Credits to Benjamin, FWIW. We'll send a patch.
>
> > We're definitely in favour of adding these sorts of helpers. Thus far,
> > these have mostly lived alongside the tests or subsystem being tested,
> > but if they're widely useful then they can sit alongside the KUnit
> > code.
> >
> > My personal feeling is that it's better to have these sorts of
> > subsystem-specific helpers be written and maintained as part of the
> > subsystems (like the tests themselves), largely because that's where
> > the subsystem expertise lies, but we're definitely happy to review any
> > such patches to make sure they fit into the KUnit infrastructure well.
>
> Right, that all makes sense. But ...
>
> > (And, of course, if something spans several subsystems, then lib/kunit
> > may be the best place to keep it.)
>
> Exactly. Even for wifi, being split between cfg80211 and mac80211 makes
> things harder than they should be and less clean if it's part of
> cfg80211 and then somehow exposed to other bits, and then possibly
> refactored into somewhere else in net if that starts using it.
>
> So I think in the case of test-managed SKBs, it would make sense to put
> it into lib/kunit.
>

Yeah, my only other thought is 'somewhere in net', but you'd know net
better than me. :-)

> > Yeah, it's a combination of "no-one has needed it yet", "no-one
> > working on KUnit understands it well enough", and "we haven't had the
> > time yet". We are a bit hesitant to add these features without having
> > tests that use them, too: often things will be coded by hand for one
> > or two tests, and only then refactored out into a common helper.
>
> Right, we're already at that place adding tests to cfg80211 and mac80211
> now, basically.
>
> > There are a few other similar helpers being worked on at the moment,
> > mostly around providing test-managed "struct device"s, so this is
> > definitely an active field of development.
>
> Great.
>
> I think we'll just send a couple of patches to start out with, for SKBs
> and the test case array macro above.
>

Sounds good!

Cheers,
-- David

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]