Messages in this thread | | | Date | Tue, 20 Dec 2005 22:08:52 +0100 | From | Jesper Juhl <> | Subject | Re: [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/
| |