From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1750920AbdLSRkC (ORCPT ); Tue, 19 Dec 2017 12:40:02 -0500 Received: from mail-io0-f196.google.com ([209.85.223.196]:46180 "EHLO mail-io0-f196.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750735AbdLSRkB (ORCPT ); Tue, 19 Dec 2017 12:40:01 -0500 X-Google-Smtp-Source: ACJfBosxFWasSzZJZgl46JEXG9WZ95tlURsHvn9J1wQnWq+TfI5UHNdRfFLlGRPhHkjMUTJzRXpJdTaRr9NfnxLFk5A= 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 Date: Tue, 19 Dec 2017 23:10:00 +0530 Message-ID: Subject: Re: [PATCH] lib: add module unload support to sort tests To: Andrew Morton 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 List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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() 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