MIME-Version: 1.0
In-Reply-To: <20171218142138.f1b84a8c1ca072b15b54af33@linux-foundation.org>
References: <1513504167-4118-1-git-send-email-pravin.shedge4linux@gmail.com> <20171218142138.f1b84a8c1ca072b15b54af33@linux-foundation.org>
From: Pravin Shedge <pravin.shedge4linux@gmail.com>
Date: Tue, 19 Dec 2017 23:10:00 +0530
Message-ID: <CALSsfODvGLKTMitvyypWwVHXtqtdNYg3JSJNFuwf+g78Ciimkw@mail.gmail.com>
Subject: Re: [PATCH] lib: add module unload support to sort tests
To: Andrew Morton <akpm@linux-foundation.org>
Cc: fkostenzer@live.at, andriy.shevchenko@linux.intel.com,
        geert@linux-m68k.org, paul.gortmaker@windriver.com,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3266
Lines: 100

On Tue, Dec 19, 2017 at 3:51 AM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> On Sun, 17 Dec 2017 15:19:27 +0530 Pravin Shedge <pravin.shedge4linux@gmail.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()

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.

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