lkml.org 
[lkml]   [2005]   [Dec]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] Correction to kmod.c control loop
On 12/20/05, borsa77@libero.it <borsa77@libero.it> wrote:
> I tried this patch on my system Slackware 10.1 with the version kernel
> 2.4.29 with any problem, below it is in broken form to allow comment
> to the source.
>

2.4.29 is a pretty old kernel.

If you want to work on 2.4.x, then work against the latest version -
2.4.32 currently.

Even better is to work on 2.6.x instead (or both 2.6 & 2.4), and if
you do you should generally send patches against latest Linus kernel -
currently that's 2.6.15-rc5-git1

Working against an old kernel like 2.4.29 is often a waste of time
since whatever you are trying to fix may very well already be fixed in
a newer kernel (something you should at the very least check), so by
working against the latest kernel you save both your own and everyone
elses time.

Anyway, a few comments below.


> --- ./kmod.bak 2005-12-19 12:48:56.000000000 +0100
> +++ ../kernel/kmod.c 2005-12-19 13:29:44.000000000 +0100

Please make patches that can be applied with patch -p1


> @@ -175,13 +175,11 @@
> */
> int request_module(const char * module_name)
> {
> - pid_t pid;
> - int waitpid_result;
> + pid_t pid, waitpid_result;

Why are you changing the type of waitpid_result ?


> sigset_t tmpsig;
> int i;
> static atomic_t kmod_concurrent = ATOMIC_INIT(0);
> -#define MAX_KMOD_CONCURRENT 50 /* Completely arbitrary
> value - KAO */
> - static int kmod_loop_msg;
> + static int MAX_KMOD_CONCURRENT, kmod_loop_msg;
>
Please don't name variables all upper case, that's how we name
constants (#define's).

> The man page for waitpid function tells the return type is pid_t.
>
> /* Don't allow request_module() before the root fs is mounted! */
> if ( ! current->fs->root ) {
> @@ -192,7 +190,7 @@
>
>
> /* If modprobe needs a service that is in a module, we get a
> recursive
> * loop. Limit the number of running kmod threads to
> max_threads/2 or
> - * MAX_KMOD_CONCURRENT, whichever is the smaller. A
> cleaner method
> + * MAX_KMOD_CONCURRENT, whichever is the larger. A
> cleaner method
> * would be to run the parents of this process, counting how
> many times
> * kmod was invoked. That would mean accessing the internals
> of the
> * process tables to get the command line, proc_pid_cmdline is
> static
> @@ -200,7 +198,7 @@
> * KAO.
> */
> i = max_threads/2;
> - if (i > MAX_KMOD_CONCURRENT)
> + if (i < MAX_KMOD_CONCURRENT)

You changed MAX_KMOD_CONCURRENT from a constant to a variable above,
but you never assign a value to it, so here you are comparing i to an
uninitialized variable, not good.


> i = MAX_KMOD_CONCURRENT;
> atomic_inc(&kmod_concurrent);
> if (atomic_read(&kmod_concurrent) > i) {
> @@ -208,6 +206,7 @@
> printk(KERN_ERR
> "kmod: runaway modprobe loop assumed
> and stopped\n");
> atomic_dec(&kmod_concurrent);
> + MAX_KMOD_CONCURRENT =
> 2*MAX_KMOD_CONCURRENT+1;

why multiply by two and add 1 here?


> return -ENOMEM;
> }
>
> Two advantages: (i) you do not worry about the choice of an arbitrary
> value, (ii) you can reiterate modprobe command until the module is
> loaded because MAX_KMOD_CONCURRENT grows with arithmetic
> progression.
>
> @@ -237,6 +236,7 @@
> if (waitpid_result != pid) {
> printk(KERN_ERR "request_module[%s]: waitpid(%d,...)
> failed, errno %d\n",
> module_name, pid, -waitpid_result);
> + return waitpid_result;

Ehh, the function returns an int, but you just changed the type of
waitpid_result to pid_t above...


> }
> return 0;
> }
>
> I think here the exit point was omitted because originally the check was
> before the unblock of the signals, now it is safe because it is at the end
> so the errorcode should be handled.
>
> If you believe these corrections are valid, please you will send me
> feedback. Otherwise I am sorry for this lack of time.


--
Jesper Juhl <jesper.juhl@gmail.com>
Don't top-post http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please http://www.expita.com/nomime.html
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2005-12-20 22:11    [W:0.219 / U:1.312 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site