try_then_request_module

try_then_request_module

[Rusty Russell]

> ChangeSet 2003/05/17 12:39:18-07:00, torvalds @ home.transmeta.com [diffview]
> 
> Make request_module() take a printf-like vararg argument instead of a string.
> 
> This is what a lot of the callers really wanted.

Excellent!  I'll close my older version of the same thing.

If someone is feeling eager, many callers could change to
try_then_request_module(), eg:

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal linux-2.5.69-bk13/drivers/net/ppp_generic.c working-2.5.69-bk13-try/drivers/net/ppp_generic.c
--- linux-2.5.69-bk13/drivers/net/ppp_generic.c	2003-05-19 10:53:44.000000000 +1000
+++ working-2.5.69-bk13-try/drivers/net/ppp_generic.c	2003-05-19 11:38:40.000000000 +1000
@@ -1959,13 +1959,8 @@ ppp_set_compress(struct ppp *ppp, unsign
 	    || ccp_option[1] < 2 || ccp_option[1] > data.length)
 		goto out;
 
-	cp = find_compressor(ccp_option[0]);
-#ifdef CONFIG_KMOD
-	if (cp == 0) {
-		request_module("ppp-compress-%d", ccp_option[0]);
-		cp = find_compressor(ccp_option[0]);
-	}
-#endif /* CONFIG_KMOD */
+	cp = try_then_request_module(find_compressor(ccp_option[0]),
+				     "ppp-compress-%d", ccp_option[0]);
 	if (cp == 0)
 		goto out;
 

Thanks!
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: try_then_request_module

[Ingo Oeser]

Hi Rusty,
hi LKML,

On Mon, May 19, 2003 at 11:41:20AM +1000, Rusty Russell wrote:
> If someone is feeling eager, many callers could change to
> try_then_request_module(), eg:

[search || request_module]

Many implementation do this with a search and retry the search 
(if clever with a goto and a flag variable to save kernel size)
after module loading.

All that implemented in the search routine, which you have to
supply anyway.

So try_then_request_module() will consolidate the the
branch or in the worst case just duplicating that code
everywhere (depends on wether you implement it as a non-inline
function or define).

Usally this is all as simple as:

   int module_loaded_flag=0;
retry_with_module_loaded:
   
   /* search code */
   
   if (!module_loaded_flag && !found) {
      module_loaded_flag=1;
      if (!request_module(bla))
         goto retry_with_module_loaded;
   }
   return found;


which is very space effecient and also still readable.

Regards

Ingo Oeser

RE: try_then_request_module

[Riley Williams]

Hi Ingo.

 > Usually this is all as simple as:
 > 
 >    int module_loaded_flag=0;
 >
 > retry_with_module_loaded:
 >    
 >    /* search code */
 >    
 >    if (!module_loaded_flag && !found) {
 >       module_loaded_flag=1;
 >       if (!request_module(bla))
 >          goto retry_with_module_loaded;
 >    }
 >    return found;
 >
 > which is very space efficient and also still readable.

Out of curiosity, what exactly is the purpose of the goto in the
above code? Since we set module_loaded_flag just prior to it, the
first if statement must fail after the goto, so we just fall down
to where we would have been without the goto.

It all looks a tad pointless to me...

Best wishes from Riley.
---
 * Nothing as pretty as a smile, nothing as ugly as a frown.

---
Outgoing mail is certified Virus Free.
Checked by AVG anti-virus system (http://www.grisoft.com).
Version: 6.0.481 / Virus Database: 277 - Release Date: 13-May-2003

Re: try_then_request_module

[Ingo Oeser]

Hi Riley,

On Mon, May 19, 2003 at 07:50:02PM +0100, Riley Williams wrote:
>  >    int module_loaded_flag=0;

Tell that we don't know, whether the module is loaded
>  > retry_with_module_loaded:
>  >    
>  >    /* search code */

Do the search.    

>  >    if (!module_loaded_flag && !found) {

Test, whether we did not yet explicitly load the module and not
found the entry either.

>  >       module_loaded_flag=1;

Tell that we loaded it (if we cannot load it, then we fall
through).

>  >       if (!request_module(bla))
>  >          goto retry_with_module_loaded;

Restart search after successful module load.

>  >    }
>  >    return found;

> Out of curiosity, what exactly is the purpose of the goto in the
> above code? Since we set module_loaded_flag just prior to it, the
> first if statement must fail after the goto, so we just fall down
> to where we would have been without the goto.
 
That is intended. I just reuse the search code here instead of
duplicating it. 

Since I load the module to broaden my search range, I can also
try to load the module there. Without module support this goto
will never execute and most of that code there compiled away.

That's why I consider try_then_request_module() not needed.
But people seem to have big problems with using gotos and still
reading the code (although it's quite common in the kernel), so
try_then_request_module() might solve this *social* problem ;-)

Regards

Ingo Oeser

RE: try_then_request_module

[Riley Williams]

Hi Ingo.

 >>>    int module_loaded_flag=0;

 > Tell that we don't know, whether the module is loaded

 >>> retry_with_module_loaded:
 >>>    
 >>>    /* search code */

 > Do the search.    

 >>>    if (!module_loaded_flag && !found) {

 > Test, whether we did not yet explicitly load the module and not
 > found the entry either.

So if either we explicitly loaded the problem or we found it, the
test fails and we skip the entire body of that if statement.

Remember, if module_loaded_flag is non-zero then !module_loaded_flag
fails. Since you're using && and the link, !found isn't even tested
in that case.

 >>>       module_loaded_flag=1;

 > Tell that we loaded it (if we cannot load it, then we fall
 > through).

 >>>       if (!request_module(bla))
 >>>          goto retry_with_module_loaded;

 > Restart search after successful module load.

Doesn't happen - the logic on the if test at the top will always fail
and we'll just fall straight back down to just below this line.

 >>>    }
 >>>    return found;

 >> Out of curiosity, what exactly is the purpose of the goto in the
 >> above code? Since we set module_loaded_flag just prior to it, the
 >> first if statement must fail after the goto, so we just fall down
 >> to where we would have been without the goto.

 > That is intended. I just reuse the search code here instead of
 > duplicating it. 

It doesn't get reused though...

 > Since I load the module to broaden my search range, I can also
 > try to load the module there. Without module support this goto
 > will never execute and most of that code there compiled away.

True.

 > That's why I consider try_then_request_module() not needed.
 > But people seem to have big problems with using gotos and still
 > reading the code (although it's quite common in the kernel), so
 > try_then_request_module() might solve this *social* problem ;-)

Personally, I need a good reason for using goto's, more so than too
many of the kernel developers, and your code is a perfect example
of the reason why - the goto accomplishes absolutely nothing because
of the faulty logic in the if statement - but the faulty logic is
actually hidden by the goto in this case.

Best wishes from Riley.
---
 * Nothing as pretty as a smile, nothing as ugly as a frown.

---
Outgoing mail is certified Virus Free.
Checked by AVG anti-virus system (http://www.grisoft.com).
Version: 6.0.483 / Virus Database: 279 - Release Date: 19-May-2003

Re: try_then_request_module

[Rusty Russell]

In message <20030519110832.G626@nightmaster.csn.tu-chemnitz.de> you write:
> So try_then_request_module() will consolidate the the
> branch or in the worst case just duplicating that code
> everywhere (depends on wether you implement it as a non-inline
> function or define).

I'm not speculating: here it is from kmod.h:

	#define try_then_request_module(x, mod...) ((x) ?: request_module(mod), (x))

It really should be:

#ifdef CONFIG_KMOD
#define try_then_request_module(x, mod...) ((x) ?: request_module(mod), (x))
#else
#define try_then_request_module(x, mod...) (x)
#endif /* CONFIG_KMOD */

Patches welcome.

Getting rid of the CONFIG_KMOD's in general code without leaving
unused code around is the aim here.

Hope that clarifies!
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.