Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757301AbdLUCFj (ORCPT ); Wed, 20 Dec 2017 21:05:39 -0500 Received: from mail-io0-f193.google.com ([209.85.223.193]:32817 "EHLO mail-io0-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755864AbdLUCFh (ORCPT ); Wed, 20 Dec 2017 21:05:37 -0500 X-Google-Smtp-Source: ACJfBos0AprBSR8MIyNDNhMZCKaX5KSWIGgyYYfcat2vuttV5lx5aksFHzXyfQnJp9pTxeG6wUoE4x51uww5dAcN7fQ= MIME-Version: 1.0 In-Reply-To: <20171220041324.GZ5437@windriver.com> References: <1513504167-4118-1-git-send-email-pravin.shedge4linux@gmail.com> <20171218142138.f1b84a8c1ca072b15b54af33@linux-foundation.org> <20171220041324.GZ5437@windriver.com> From: Pravin Shedge Date: Thu, 21 Dec 2017 07:35:36 +0530 Message-ID: Subject: Re: [PATCH] lib: add module unload support to sort tests To: Paul Gortmaker Cc: Andrew Morton , fkostenzer@live.at, andriy.shevchenko@linux.intel.com, geert@linux-m68k.org, linux-kernel@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 7133 Lines: 186 On Wed, Dec 20, 2017 at 9:43 AM, Paul Gortmaker 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 >> wrote: >> > On Sun, 17 Dec 2017 15:19:27 +0530 Pravin Shedge 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: > > 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