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
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
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
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
linux-kselftest-mirror@lists.linaro.org
-
David Gow -
Johannes Berg