linux-kselftest.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeremy Kerr <jk@codeconstruct.com.au>
To: Daniel Latypov <dlatypov@google.com>,
	Brendan Higgins <brendanhiggins@google.com>
Cc: davidgow@google.com, linux-kernel@vger.kernel.org,
	kunit-dev@googlegroups.com, linux-kselftest@vger.kernel.org,
	skhan@linuxfoundation.org
Subject: Re: [RFC PATCH] kunit: flatten kunit_suite*** to kunit_suite** in executor
Date: Thu, 14 Oct 2021 09:55:07 +0800	[thread overview]
Message-ID: <0f85025124359304c8a2a97d007b66d5655645c1.camel@codeconstruct.com.au> (raw)
In-Reply-To: <CAGS_qxoziNGNVpsUfvUfOReADY0PdriV2gJJ7+LUzzd+7BU-Ow@mail.gmail.com>

Hi Daniel,

> > I like it! This seems to make a lot of logic simpler (and from the
> > sounds makes Jeremy's proposed module patch easier?).
> 
> This would make the subsequent cleanup easier.
> 
> Jemery, correct me if I misread, but it sounded like you were thinking
> of leaving the current kunit_suite*** layout until afterwards. Then
> we'd flatten it as a cleanup.

Yep, that was my rough plan. From the current situation:

An object (module or built-in) defines a:

    kunit_test_suite(my_suite)

[or, equivalently, a kunit_test_suites(&my_suite). The only place that I
can see where we add more than one suite through kunit_test_suites is
the kunit code, so let's ignore that for now..]

That expands to:

    static struct kunit_suite *unique_array[] = { &my_suite, NULL };
    static struct kunit_suite **unique_suites
    __used __section(".kunit_test_suites") = unique_array;

[assume actual unique values for unique_array & unique_suites]

So the .kunit_test_suites section content ends up being: an array of
pointers to (null-terminated) arrays of pointers to struct kunit_suite.
But those latter arrays only have one entry anyway.

Once we're reading the suites out of the section, we have the section
size too, so can determine the number of "things"
(suites/arrays-of-suites/whatever) from that.

So, given we only have one entry in the array, we can probably drop the
null-termination, and expand the kunit_test_suite() macro to:

    kunit_test_suite(my_suite) ->

	static struct kunit_suite *unique_array[] = { &my_suite };
	kunit_test_suites_for_module(unique_array);
	static struct kunit_suite **unique_suites
	__used __section(".kunit_test_suites") = unique_array;

Then, the array becomes pretty pointless, so we can reduce that to:

    kunit_test_suite(my_suite) ->

	static struct kunit_suite *unique_array = &my_suite;
	kunit_test_suites_for_module(unique_array);
	static struct kunit_suite *unique_suites
	__used __section(".kunit_test_suites") = unique_array;

But then there's no need for the double pointers, so we could just do:

    kunit_test_suite(my_suite) ->

	static struct kunit_suite *unique_suite
	__used __section(".kunit_test_suites") = &my_suite;

Resulting in the .kunit_test_suites section just being a set of
contiguous pointers to struct kunit_suite. We get the number of suites
from the section size.

[We could go one step further and put the suites themselves in
.kunit_test_suites, rather than the pointers. However, the downside
there is that we'd need macros for the suite declarations, which isn't
as clean]

Modules with multiple suites (currently: kunit itself) can just have
multiple occurrences of kunit_test_suite(), as they no longer conflict,
and we'd no longer need kunit_test_suites() at all.

That was my thinking, anyway. I think it probably makes sense to do that
cleanup after the section patch, as that means we don't need any
post-processing on the suites arrays.

Cheers,


Jeremy


  reply	other threads:[~2021-10-14  1:55 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-10-13 19:13 [RFC PATCH] kunit: flatten kunit_suite*** to kunit_suite** in executor Daniel Latypov
2021-10-13 20:03 ` Brendan Higgins
2021-10-13 20:15   ` Daniel Latypov
2021-10-14  1:55     ` Jeremy Kerr [this message]
2022-01-28 21:19       ` Daniel Latypov
2022-01-28 21:27         ` Brendan Higgins
2021-10-14  2:16 ` David Gow

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0f85025124359304c8a2a97d007b66d5655645c1.camel@codeconstruct.com.au \
    --to=jk@codeconstruct.com.au \
    --cc=brendanhiggins@google.com \
    --cc=davidgow@google.com \
    --cc=dlatypov@google.com \
    --cc=kunit-dev@googlegroups.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=skhan@linuxfoundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).