linux-kselftest.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Alan Maguire <alan.maguire@oracle.com>
To: Brendan Higgins <brendanhiggins@google.com>
Cc: Alan Maguire <alan.maguire@oracle.com>,
	Kees Cook <keescook@chromium.org>,
	"open list:KERNEL SELFTEST FRAMEWORK" 
	<linux-kselftest@vger.kernel.org>,
	Shuah Khan <skhan@linuxfoundation.org>,
	Luis Chamberlain <mcgrof@kernel.org>,
	Iurii Zaikin <yzaikin@google.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Masahiro Yamada <yamada.masahiro@socionext.com>,
	catalin.marinas@arm.com, joe.lawrence@redhat.com,
	penguin-kernel@i-love.sakura.ne.jp, schowdary@nvidia.com,
	urezki@gmail.com, andriy.shevchenko@linux.intel.com,
	changbin.du@intel.com, kunit-dev@googlegroups.com,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	linux-fsdevel <linux-fsdevel@vger.kernel.org>,
	Knut Omang <knut.omang@oracle.com>
Subject: Re: [PATCH v2 linux-kselftest-test 1/3] kunit: allow kunit tests to be loaded as a module
Date: Thu, 17 Oct 2019 19:32:18 +0100 (BST)	[thread overview]
Message-ID: <alpine.LRH.2.20.1910171930410.21739@dhcp-10-175-161-223.vpn.oracle.com> (raw)
In-Reply-To: <20191016230116.GA82401@google.com>

On Wed, 16 Oct 2019, Brendan Higgins wrote:

> On Fri, Oct 11, 2019 at 11:25:33AM +0100, Alan Maguire wrote:
> > On Fri, 11 Oct 2019, Brendan Higgins wrote:
> > 
> > > Sorry for the delayed reply. I will be on vacation until Wednesday,
> > > October 16th.
> > > 
> > > On Wed, Oct 9, 2019 at 9:36 AM Alan Maguire <alan.maguire@oracle.com> wrote:
> > > >
> > > > On Tue, 8 Oct 2019, Brendan Higgins wrote:
> > > >
> > > > > On Tue, Oct 08, 2019 at 03:55:44PM +0100, Alan Maguire wrote:
> > > [...]
> > > > > > diff --git a/lib/kunit/string-stream.c b/lib/kunit/string-stream.c
> > > > > > index e6d17aa..e4f3a97 100644
> > > > > > --- a/lib/kunit/string-stream.c
> > > > > > +++ b/lib/kunit/string-stream.c
> > > > > > @@ -100,6 +100,7 @@ int string_stream_vadd(struct string_stream *stream,
> > > > > >
> > > > > >     return 0;
> > > > > >  }
> > > > > > +EXPORT_SYMBOL_GPL(string_stream_vadd);
> > > > >
> > > > > Is this actually needed by anything other than lib/kunit/test.c right
> > > > > now? Maybe we should move the include file into the kunit/ directory to
> > > > > hide these so no one else can use them.
> > > > >
> > > >
> > > > I tried this, and it's the right answer I think but it exposes
> > > > a problem with symbol visibility when kunit is compiled as a module.
> > > > More on this below...
> > > >
> > > > > >  int string_stream_add(struct string_stream *stream, const char *fmt, ...)
> > > > > >  {
> > > > > > @@ -112,6 +113,7 @@ int string_stream_add(struct string_stream *stream, const char *fmt, ...)
> > > > > >
> > > > > >     return result;
> > > > > >  }
> > > > > > +EXPORT_SYMBOL_GPL(string_stream_add);
> > > > > [...]
> > > > > > diff --git a/lib/kunit/test.c b/lib/kunit/test.c
> > > > > > index c83c0fa..e7896f1 100644
> > > > > > --- a/lib/kunit/test.c
> > > > > > +++ b/lib/kunit/test.c
> > > > > [...]
> > > > > > @@ -50,6 +51,7 @@ static unsigned long kunit_test_timeout(void)
> > > > > >      * For more background on this topic, see:
> > > > > >      * https://mike-bland.com/2011/11/01/small-medium-large.html
> > > > > >      */
> > > > > > +#ifndef MODULE
> > > > >
> > > > > Why is this block of code "ifndef MODULE"?
> > > > >
> > > >
> > > > Symbol visibility is the problem again; sysctl_hung_task_timeout_secs
> > > > isn't exported so when kunit is a module it can't find the symbol.
> > > >
> > > > I think I saw Kees mentioned something about symbol lookup too; in KTF
> > > > Knut solved this by defining ktf_find_symbol(). I'd suggest we may need a
> > > > kunit_find_symbol() with a function signature
> 
> Based on what you said below, I think the kunit_find_symbol() may have
> value for writing tests; however, I do not think it is the right way to
> handle resources needed by test.c. I think exporting the symbols in this
> case is the lesser of the two evils.
> 

The only symbol we need in core kunit from the kernel when compiling kunit 
as a module is sysctl_hung_task_timeout_secs; it's a core kernel symbol.
There's no issue with symbols within kunit in this case.
 
I've come up with a new way to handle variables and functions in the 
v3 patch set 3 [1] I've sent out. While not being perfect, it attempts to 
satisfy some of the requirements you describe below.   It will generate 
compiler errors if there is a mismatch between local symbol definition and 
the target symbol type.

Symbol variable definitions are handled such that the same symbol name can 
be used; see try-catch.c in patch 5 where we assign 
sysctl_hung_task_timeout_secs.

Unfortunately the same scheme won't work for functions. The reason for 
this is we've already #included a definiton of the function, so if we 
attempt to redefine that same name as a function pointer we get a 
compile-time that we are redefining the symbol.  As a consequence the
approach I took is for us to define a local function pointer and it gets
assigned either to

 - the results of kunit_find_symbol() (module case)
 - the function itself (builtin case)

The latter will trigger a compile-time error if our local definition is 
out of sync.

> I am still suprised that you need to export a symbol that is 
getting
> compiled into and is only used by the kunit module.

see above - kunit needs a non-exported global kernel symbol 
(sysctl_hung_task_timeout_secs).


> In fact, I think I
> found an example in the kernel where someone else managed this. Checkout
> stp_policy_node_priv(). Looks like the symbol is used here[1] and is
> defined here[2]. You can see here[3] and here[4] that the files end up
> in the same module. Do you mind taking a look why it works for stm, but
> not here?
> 
> > > I thought we were just talking about exposing symbols for linking
> > > outside of a compilation unit (static vs. not static); nevertheless, I
> > > think you are right that it is relevant here. Kees, thoughts?
> > > 
> > > > void *kunit_find_symbol(const char *modname, const char *symbol_name);
> > > >
> > > > ...which does a [module_]kallsyms_lookup_sym().
> > > >
> > > > If the above makes sense I can look at adding it as a patch (and adding
> > > > a test of it of course!). What do you think?
> > > 
> > > So that won't work if you are trying to link against a symbol not in a
> > > module, right? Also, it won't work against a static symbol, right?
> > > 
> > 
> > Nope, works in both cases with the proviso that we need to use an
> 
> Nifty! That sounds great!
> 
> > alternative name for symbols when compiling built-in.  For example
> 
> Can you elaborate on "need[ing] to use an alternative name"?
> 

See above and patch 4 in the v3 patchset.

> > in the case of the string-stream tests, we'd use a test init callback
> > to initialize used symbols:
> > 
> > static int string_stream_test_init(struct kunit *test)
> > {
> >         _alloc_string_stream = kunit_find_symbol("alloc_string_stream");
> >         _string_stream_add = kunit_find_symbol("string_stream_add");
> >         _string_stream_get_string = kunit_find_symbol("string_stream_get_string");
> >         _string_stream_is_empty = kunit_find_symbol("string_stream_is_empty");
> >         if (IS_ERR(_alloc_string_stream) ||
> >             IS_ERR(_string_stream_add) ||
> >             IS_ERR(_string_stream_get_string) ||
> >             IS_ERR(_string_stream_is_empty))
> >                 return EINVAL;
> >         return 0;
> > } 
> > 
> > I've tested this when string-stream-test is compiled built-in and as a 
> > module.  We can of course create a wrapper macro to handle these 
> > assignments.
> 
> I've got mixed feelings on this. On one hand, that has the potential to
> solve a lot of problems with visibility and modules in a way that
> doesn't immediately cause code under test to change in undesirable ways.
> On the other hand, I feel that this has the potential to be really prone
> to breakage. It would be much nicer if the compiler could tell you that
> your symbol changed rather than having to wait until you run the test.
> Just having the test tell you that a symbol doesn't exist anymore would
> be mildly annoying, but having the signature of the symbol change could
> get downright frustrating using this method.
> 

See above; if compiled as builtin compiler errors will be generated.

Thanks!

Alan

[1] https://lkml.org/lkml/2019/10/17/801

  reply	other threads:[~2019-10-17 18:36 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-08 14:55 [PATCH v2 linux-kselftest-test 0/3] kunit: support building core/tests as modules Alan Maguire
2019-10-08 14:55 ` [PATCH v2 linux-kselftest-test 1/3] kunit: allow kunit tests to be loaded as a module Alan Maguire
2019-10-08 21:35   ` Brendan Higgins
2019-10-09 16:35     ` Alan Maguire
2019-10-11  9:47       ` Brendan Higgins
2019-10-11 10:25         ` Alan Maguire
2019-10-16 23:01           ` Brendan Higgins
2019-10-17 18:32             ` Alan Maguire [this message]
2019-10-18 12:21               ` Luis Chamberlain
2019-10-24  1:33                 ` Brendan Higgins
2019-10-08 14:55 ` [PATCH v2 linux-kselftest-test 2/3] kunit: allow kunit " Alan Maguire
2019-10-08 15:13   ` Randy Dunlap
2019-10-08 14:55 ` [PATCH v2 linux-kselftest-test 3/3] kunit: update documentation to describe module-based build Alan Maguire
2019-10-08 21:47   ` Brendan Higgins
2019-10-08 21:00 ` [PATCH v2 linux-kselftest-test 0/3] kunit: support building core/tests as modules Brendan Higgins
2019-10-14  9:20 ` Luis Chamberlain
2019-10-14 14:02   ` Alan Maguire
2019-10-16 12:47     ` Luis Chamberlain

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=alpine.LRH.2.20.1910171930410.21739@dhcp-10-175-161-223.vpn.oracle.com \
    --to=alan.maguire@oracle.com \
    --cc=akpm@linux-foundation.org \
    --cc=andriy.shevchenko@linux.intel.com \
    --cc=brendanhiggins@google.com \
    --cc=catalin.marinas@arm.com \
    --cc=changbin.du@intel.com \
    --cc=joe.lawrence@redhat.com \
    --cc=keescook@chromium.org \
    --cc=knut.omang@oracle.com \
    --cc=kunit-dev@googlegroups.com \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=mcgrof@kernel.org \
    --cc=penguin-kernel@i-love.sakura.ne.jp \
    --cc=schowdary@nvidia.com \
    --cc=skhan@linuxfoundation.org \
    --cc=urezki@gmail.com \
    --cc=yamada.masahiro@socionext.com \
    --cc=yzaikin@google.com \
    --subject='Re: [PATCH v2 linux-kselftest-test 1/3] kunit: allow kunit tests to be loaded as a module' \
    /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

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).