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.
Signed-off-by: Pravin Shedge <[email protected]>
---
lib/test_sort.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/lib/test_sort.c b/lib/test_sort.c
index d389c1c..26c98d0 100644
--- 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);
--
2.7.4
On Sun, 17 Dec 2017 15:19:27 +0530 Pravin Shedge <[email protected]> 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
So the module will be auto-unloaded if it failed and will require an
rmmod if it succeeded.
Correct?
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?
If so, why do you think we shiould alter lib/test_sort.c to behave in
this atypical fashion?
On Tue, Dec 19, 2017 at 3:51 AM, Andrew Morton
<[email protected]> wrote:
> On Sun, 17 Dec 2017 15:19:27 +0530 Pravin Shedge <[email protected]> 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
On Tue, 19 Dec 2017 23:10:00 +0530 Pravin Shedge <[email protected]> wrote:
> >
> > 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.
OK.
I think it does make sense for a lib/text_*.ko type module to unload
itself after successful completion of the test. However:
- returning a fake error code from the module's module_init() is a
daft way of doing that. We should find a way to let the
module_init() handler tell do_init_module() "I succeeded, but please
unload me anyway". So the initial sys_init_module() call doesn't say
"it failed". Could create a new, kernel-internal errno for this and
have do_init_module() rewrite that to 0.
Maybe. A sys_init_module() caller's expectation is that if the
syscall succeeded then the module is now loaded.
Well. Except for the test_*.ko modules, which are special.
- Changing any test module so that it now auto-unloads on success is
a non-back-compat change. The practical effect of which will be very
minor: a subsequent rmmod finds that the module isn't there.
I'm not sure what to do, really. Does any of this matter much at all?
On 12/19/17 14:51, Andrew Morton wrote:
> On Tue, 19 Dec 2017 23:10:00 +0530 Pravin Shedge <[email protected]> wrote:
>
>>>
>>> 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.
>
> OK.
>
> I think it does make sense for a lib/text_*.ko type module to unload
> itself after successful completion of the test. However:
>
> - returning a fake error code from the module's module_init() is a
> daft way of doing that. We should find a way to let the
> module_init() handler tell do_init_module() "I succeeded, but please
> unload me anyway". So the initial sys_init_module() call doesn't say
> "it failed". Could create a new, kernel-internal errno for this and
> have do_init_module() rewrite that to 0.
>
> Maybe. A sys_init_module() caller's expectation is that if the
> syscall succeeded then the module is now loaded.
>
> Well. Except for the test_*.ko modules, which are special.
>
> - Changing any test module so that it now auto-unloads on success is
> a non-back-compat change. The practical effect of which will be very
> minor: a subsequent rmmod finds that the module isn't there.
>
> I'm not sure what to do, really. Does any of this matter much at all?
Nope. But I would merge this patch that returns fake-error so that
the module doesn't remain loaded, similar to what some others also do.
I don't think it's worth a new return value for success_but_unload_me.
--
~Randy
[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
> <[email protected]> wrote:
> > On Sun, 17 Dec 2017 15:19:27 +0530 Pravin Shedge <[email protected]> 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
On Wed, Dec 20, 2017 at 9:43 AM, Paul Gortmaker
<[email protected]> 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
>> <[email protected]> wrote:
>> > On Sun, 17 Dec 2017 15:19:27 +0530 Pravin Shedge <[email protected]> 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
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 by
adding module_exit support.
Previous patch was implemented auto unload support by returning
-EAGAIN from module_init() function on successful case, but this
approach is not ideal.
The auto-unload might seem like a nice optimization, but it encourages
inconsistent behaviour. And behaviour that is different from all other
normal modules.
Signed-off-by: Pravin Shedge <[email protected]>
---
lib/test_sort.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/lib/test_sort.c b/lib/test_sort.c
index d389c1c..385c0ed 100644
--- a/lib/test_sort.c
+++ b/lib/test_sort.c
@@ -39,5 +39,11 @@ static int __init test_sort_init(void)
return err;
}
+static void __exit test_sort_exit(void)
+{
+}
+
module_init(test_sort_init);
+module_exit(test_sort_exit);
+
MODULE_LICENSE("GPL");
--
2.7.4