Re: [PATCH] lib: add module unload support to sort tests

[Pravin Shedge]

On Wed, Dec 20, 2017 at 9:43 AM, Paul Gortmaker
<paul.gortmaker@xxxxxxxxxxxxx> wrote:
> [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@xxxxxxxxxxxxxxxxxxxx> wrote:
>> > On Sun, 17 Dec 2017 15:19:27 +0530 Pravin Shedge <pravin.shedge4linux@xxxxxxxxx> 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.
> --

Paul I am completely agree with you.
Auto-unloading module features might break the consistency from users
point of view.
All one-shot software tests should aligned with the same behavior that follow
loading module, running test and then wait for external unload trigger
for consistency.

Other test that I refer, lib/percpu_test.c and lib/rbtree_test.c also
suffers from
consistency point of view and needs to update to achieve the consistency goal.

I will update the diff by adding module_exit() which gives chance to trigger
external unload for consistency.

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