Message-ID: <20171220041324.GZ5437@windriver.com>
Date:   Tue, 19 Dec 2017 23:13:24 -0500
From:   Paul Gortmaker <paul.gortmaker@...driver.com>
To:     Pravin Shedge <pravin.shedge4linux@...il.com>
CC:     Andrew Morton <akpm@...ux-foundation.org>, <fkostenzer@...e.at>,
        <andriy.shevchenko@...ux.intel.com>, <geert@...ux-m68k.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] lib: add module unload support to sort tests

[Re: [PATCH] lib: add module unload support to sort tests] On 19/12/2017 (Tue 23:10) Pravin Shedge wrote:

> On Tue, Dec 19, 2017 at 3:51 AM, Andrew Morton
> <akpm@...ux-foundation.org> wrote:
> > On Sun, 17 Dec 2017 15:19:27 +0530 Pravin Shedge <pravin.shedge4linux@...il.com> wrote:
> >
> >> test_sort.c perform array-based and linked list sort test. Code allows to
> >> compile either as a loadable modules or builtin into the kernel.
> >>
> >> Current code is not allow to unload the test_sort.ko module after
> >> successful completion.
> >>
> >> This patch add support to unload the "test_sort.ko" module.
> >>
> >> ...
> >>
> >> --- a/lib/test_sort.c
> >> +++ b/lib/test_sort.c
> >> @@ -13,11 +13,12 @@ static int __init cmpint(const void *a, const void *b)
> >>
> >>  static int __init test_sort_init(void)
> >>  {
> >> -     int *a, i, r = 1, err = -ENOMEM;
> >> +     int *a, i, r = 1;
> >> +     int err = -EAGAIN; /* Fail will directly unload the module */
> >>
> >>       a = kmalloc_array(TEST_LEN, sizeof(*a), GFP_KERNEL);
> >>       if (!a)
> >> -             return err;
> >> +             return -ENOMEM;
> >>
> >>       for (i = 0; i < TEST_LEN; i++) {
> >>               r = (r * 725861) % 6599;
> >> @@ -26,13 +27,12 @@ static int __init test_sort_init(void)
> >>
> >>       sort(a, TEST_LEN, sizeof(*a), cmpint, NULL);
> >>
> >> -     err = -EINVAL;
> >>       for (i = 0; i < TEST_LEN-1; i++)
> >>               if (a[i] > a[i+1]) {
> >>                       pr_err("test has failed\n");
> >> +                     err = -EINVAL;
> >>                       goto exit;
> >>               }
> >> -     err = 0;
> >>       pr_info("test passed\n");
> >>  exit:
> >>       kfree(a);
> >
> > I'm struggling to understand this.
> >
> > It seems that the current pattern for lib/test_*.c is:
> >
> > - if test fails, module_init() handler returns -EFOO
> > - if test succeeds, module_init() handler returns 0
> >
> 
> Not true for all lib/*.c
> I refer following modules lib/percpu_test.c and lib/rbtree_test.c.
> 
> > So the module will be auto-unloaded if it failed and will require an
> > rmmod if it succeeded.
> >
> > Correct?
> Right. There are two approaches that I saw modules present in lib/*.c
> Few modules execute set of test cases from module_init() and at the end
> on successful completion they unload the module by returning -EAGAIN
> from module_init()

So, I'd make the argument that the two approaches is not ideal.  Start
by considering the two common use cases:

#1 - Fred builds everything in non-module;  boots and checks "dmesg" to
see what passed and failed.  He does not care about unload because the
machine will reboot for the next test in less than five minutes.

#2 - Bob wrote a test suite.  It does "dmesg -c" and loads a single test
and checks dmesg.  It then rmmod and restarts with the next module.

If we have two approaches, then Bob has a problem.  He has to track
which module he has to unload and which auto-unload.  Or he
unconditionally does an unload and ignores the error if any.  Which is
bad if the error code is -EBUSY due to dependencies or similar.

The auto-unload might seem like a nice optimization, but it encourages
inconsistent behaviour.  And behaviour that is different from all other
normal modules.

And finally, if the test is one shot, how do you justify leaving it
loaded in the kernel when it passed, but removing it when it failed?
That makes sense for driver probes but not one-shot software tests.

I'd suggest load, run test and wait for external unload trigger for
consistency.  And not to abuse the module_init() return code as a
communication channel for pass/fail.

> 
> Those code can compile as in-build in kernel or as a module.
> That means those testcases execute at the time of boot
> 
> Help from the make menuconfig for CONFIG_TEST_LIST_SORT shows:
> 
> "Enable this to turn on 'list_sort()' function test.
> This test is executed only once during system boot (so affects only
> boot time), or at module load time."
> 
> If test case is going affects only at boot time or at module load
> time, it's smart decision to unload module
> automatically on successful completion.

See above - I don't think it is smart, and the choice of which one
stays and which one auto-unloads is arbitrary and inconsistent.

Imagine something as simple as this:

	for i in $LIST ; do
		modprobe $i
		lsmod | grep -q $i
		if [ $? != 0 ]; then echo Module $i is not present! ; fi
	done

OK, not ideal code, ignoring the modprobe return -- but what it reports
is true -- your test module (if it passed) will NOT be present.

> 
> >
> > And it appears that lib/test_sort.c currently implements the above.
> > And that your patch changes it so that the module_init() handler always
> > returns -EFOO, so the module will be aut-unloaded whether or not the
> > testing passed.
> >
> > Correct?
> Right. On any case test case is going show log in syslog either on
> it's failure or successful case.

Look at "dmesg" after booting on any modern machine.  There are
thousands of lines.  Since you are already adding at least one more line
regardless, then do not redundantly abuse the return code of
module_init().  Instead maybe propose a standard for tests reporting in
dmesg/syslog, like:

Selftest: <name>  <PASS/FAIL> <optional status/reason string>

Then people who care about adding them to a self-test suite will have a
much easier job.  And work towards transitioning other tests to this.

All that said, I also agree with akpm that none of this really matters
in the big picture.  :)  But if we do change things, I think consistency
should be the #1 goal.  We have thousands of modules, and if there is
not consistency, end users will just get frustrated. 

I don't want to be the one explaining to a user "Oh, that is a test
module so it does things differently than the other 99% of modules."
Good way to alienate new users....

Paul.
--

> >
> > If so, why do you think we shiould alter lib/test_sort.c to behave in
> > this atypical fashion?
> 
> If test case is going affects only at boot time or at module load
> time, it's smart decision to unload module
> automatically on successful completion.
> >
> 
> Thanks & Regards,
>   PraviN

Powered by blists - more mailing lists