Re: [PATCH] Correction to kmod.c control loop

Re: [PATCH] Correction to kmod.c control loop

[borsa77]

On 21 Dec 2005 at 0:11, Jesper Juhl wrote:

> > That is the right way of course, but the portion of kmod.c I look is the
> > same from 2.2.x series.
> >
> That may be so, but if you don't work against the latest kernel your
> patches are unlikely to apply cleanly (other nearby bits of the file
> may have changed causing patch to become confused), and also,
> maintainers will usually request that you submit a re-diff'ed patch
> against the latest kernel *anyway* before they will consider merging
> your patch, so by working against an old kernel you are only creating
> more work for both yourself and people reviewing and/or considering
> applying your patches.
> Work to make review and merging as easy on everyone else as you can -
> starting out by submitting patches only against latest 2.4.x or 2.6.x
> is a good place to start.

I look in the kmod.c for 2.6.x that you have linked below, actually it was 
reimplemented, so the patch is only for 2.4.x.

> > > Why are you changing the type of waitpid_result ?
> >
> > Because the man page for waitpid function tells the return type is pid_t.
> >
> Ok, but you just created a potential problem by then later returning
> pid_t from a function supposed to be returning int.
> When making changes like this you should explain in your mail that go
> along with your patch *why* you are changing things and how you've
> made sure they are safe - you need to be able to explain that.

It would be only a formal change, and as you have reported below pid_t 
is an integer.

> > > 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.
> >
> >
> > It is a _static_ local variable so it is assigned automatically to zero, I
> > think.
> >
> Not good enough. Either you *know* that it will be initialized to
> zero, and if so you should state that in your explanation of what the
> patch does or you should explicitly initialize it.

I have read the K&R: now I know.

> > > >         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?
> >
> >
> > For to grow up the previously zero value at each failure loop.
> > See (ii) below.
> >
> Yes, I understand that you wish to grow it, it's just not clear to me
> why you want to grow it like that.

To tune system capability to user request.

> > > >                 return -ENOMEM;
> > > >         }
> > > >
> > > > @@ -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...
> >
> >
> > True, I had better control with grep that pid_t is an integer type.
> >
> pid_t is defined as
>   typedef __kernel_pid_t          pid_t;
> and __kernel_pid_t is
>   typedef int                     __kernel_pid_t;
> on all archs as far as I know, so you end up actually being safe, but
> the problem is you didn't make that clear.
> But, the point I was trying to make was mainly that you shouldn't
> count on that always being true. if the function needs to return an
> int you should return an int, not a pid_t.
> 
> Also, take a look at how the function is implemented in recent kernels :
> http://sosdg.org/~coywolf/lxr/source/kernel/kmod.c#L66
> 
> >
> > > >         }
> > > >         return 0;
> > > >  }

Re: [PATCH] Correction to kmod.c control loop

[borsa77]

On 21 Dec 2005 at 0:23, Jesper Juhl wrote:

> On 12/21/05, Jesper Juhl <jesper.juhl@gmail.com> wrote:
> > > > > -       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.
> > >
> > >
> > > It is a _static_ local variable so it is assigned automatically to zero, I
> > > think.
> > >
> Hmm, yes, I wonder at that a bit as well, why static?

To maintain the value through a call of modprobe and a following one, 
so if the first fails you can maybe have the loading with the others.

[PATCH] Correction to kmod.c control loop

[borsa77]

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. 

--- ./kmod.bak	2005-12-19 12:48:56.000000000 +0100 
+++ ../kernel/kmod.c	2005-12-19 13:29:44.000000000 +0100 
@@ -175,13 +175,11 @@ 
  */ 
 int request_module(const char * module_name) 
 { 
-	pid_t pid; 
-	int waitpid_result; 
+	pid_t pid, 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; 

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) 
 		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; 
 		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; 
 	} 
 	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. 
Regards, Marco Borsari.

Re: [PATCH] Correction to kmod.c control loop

[Jesper Juhl]

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