[PATCH] call_usermodehelper_setup() should use GFP_KERNEL

[PATCH] call_usermodehelper_setup() should use GFP_KERNEL

[KOSAKI Motohiro]

Now, call_usermodehelper_setup() use GFP_ATOMIC.
but it is slighly odd.
because call_usermodehelper() is always called process context.

GFP_KERNEL is robust and better.


Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
CC: "Paul Menage" <menage@google.com>
CC: Li Zefan <lizf@cn.fujitsu.com>
CC: Jeremy Fitzhardinge <jeremy@xensource.com>
CC: Andi Kleen <ak@suse.de>
CC: Rusty Russell <rusty@rustcorp.com.au> 

---
 kernel/kmod.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Index: b/kernel/kmod.c
===================================================================
--- a/kernel/kmod.c	2008-04-29 17:37:41.000000000 +0900
+++ b/kernel/kmod.c	2008-05-06 20:20:02.000000000 +0900
@@ -360,7 +360,7 @@ struct subprocess_info *call_usermodehel
 						  char **argv, char **envp)
 {
 	struct subprocess_info *sub_info;
-	sub_info = kzalloc(sizeof(struct subprocess_info),  GFP_ATOMIC);
+	sub_info = kzalloc(sizeof(struct subprocess_info),  GFP_KERNEL);
 	if (!sub_info)
 		goto out;
 

Re: [PATCH] call_usermodehelper_setup() should use GFP_KERNEL

[Li Zefan]

KOSAKI Motohiro wrote:
> Now, call_usermodehelper_setup() use GFP_ATOMIC.
> but it is slighly odd.
> because call_usermodehelper() is always called process context.
> 

Are you sure? I found the following call chain:

  static irqreturn_t power_handler(int irq, void *dev_id)
  ->orderly_poweroff(true);
    ->call_usermodehelper_setup()

> GFP_KERNEL is robust and better.
> 
> 
> Signed-off-by: KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com>
> CC: "Paul Menage" <menage@google.com>
> CC: Li Zefan <lizf@cn.fujitsu.com>
> CC: Jeremy Fitzhardinge <jeremy@xensource.com>
> CC: Andi Kleen <ak@suse.de>
> CC: Rusty Russell <rusty@rustcorp.com.au> 
> 
> ---
>  kernel/kmod.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> Index: b/kernel/kmod.c
> ===================================================================
> --- a/kernel/kmod.c	2008-04-29 17:37:41.000000000 +0900
> +++ b/kernel/kmod.c	2008-05-06 20:20:02.000000000 +0900
> @@ -360,7 +360,7 @@ struct subprocess_info *call_usermodehel
>  						  char **argv, char **envp)
>  {
>  	struct subprocess_info *sub_info;
> -	sub_info = kzalloc(sizeof(struct subprocess_info),  GFP_ATOMIC);
> +	sub_info = kzalloc(sizeof(struct subprocess_info),  GFP_KERNEL);
>  	if (!sub_info)
>  		goto out;
>  
> 
> 
> 
> 

Re: [PATCH] call_usermodehelper_setup() should use GFP_KERNEL

[KOSAKI Motohiro]

> KOSAKI Motohiro wrote:
> > Now, call_usermodehelper_setup() use GFP_ATOMIC.
> > but it is slighly odd.
> > because call_usermodehelper() is always called process context.
> > 
> 
> Are you sure? I found the following call chain:
> 
>   static irqreturn_t power_handler(int irq, void *dev_id)
>   ->orderly_poweroff(true);
>     ->call_usermodehelper_setup()

sorry, you are right.
I'll make patch again.

Re: [PATCH] call_usermodehelper_setup() should use GFP_KERNEL

[Andrew Morton]

On Thu, 08 May 2008 16:31:29 +0900 KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:

> > KOSAKI Motohiro wrote:
> > > Now, call_usermodehelper_setup() use GFP_ATOMIC.
> > > but it is slighly odd.
> > > because call_usermodehelper() is always called process context.
> > > 
> > 
> > Are you sure? I found the following call chain:
> > 
> >   static irqreturn_t power_handler(int irq, void *dev_id)
> >   ->orderly_poweroff(true);
> >     ->call_usermodehelper_setup()
> 
> sorry, you are right.
> I'll make patch again.

How many times do we have to make this mistake :(

Only the caller knows what allocation mode the callee can use. 
call_usermodehelper_setup() should be extended to take a gfp_t argument.

Re: [PATCH] call_usermodehelper_setup() should use GFP_KERNEL

[KOSAKI Motohiro]

> > > Are you sure? I found the following call chain:
> > > 
> > >   static irqreturn_t power_handler(int irq, void *dev_id)
> > >   ->orderly_poweroff(true);
> > >     ->call_usermodehelper_setup()
> > 
> > sorry, you are right.
> > I'll make patch again.
> 
> How many times do we have to make this mistake :(
> 
> Only the caller knows what allocation mode the callee can use. 
> call_usermodehelper_setup() should be extended to take a gfp_t argument.

Agreed.
I'm testing so patch.
I'll post tommorow, myabe.

Re: [PATCH] call_usermodehelper_setup() should use GFP_KERNEL

[Jeremy Fitzhardinge]

Andrew Morton wrote:
> On Thu, 08 May 2008 16:31:29 +0900 KOSAKI Motohiro <kosaki.motohiro@jp.fujitsu.com> wrote:
>
>   
>>> KOSAKI Motohiro wrote:
>>>       
>>>> Now, call_usermodehelper_setup() use GFP_ATOMIC.
>>>> but it is slighly odd.
>>>> because call_usermodehelper() is always called process context.
>>>>
>>>>         
>>> Are you sure? I found the following call chain:
>>>
>>>   static irqreturn_t power_handler(int irq, void *dev_id)
>>>   ->orderly_poweroff(true);
>>>     ->call_usermodehelper_setup()
>>>       
>> sorry, you are right.
>> I'll make patch again.
>>     
>
> How many times do we have to make this mistake :(
>
> Only the caller knows what allocation mode the callee can use. 
> call_usermodehelper_setup() should be extended to take a gfp_t argument.
>   

Yeah, but making the caller need to know about the internal 
implementation details of the callee (ie, whether it needs to allocate 
memory or not) leads to pretty warty interfaces.  In this case, you 
could push the gfp_t up to the call_usermodehelper_setup() level, but 
pushing it any higher wouldn't make much sense.

Not that I have a better answer.

    J

Re: [PATCH] call_usermodehelper_setup() should use GFP_KERNEL

[KOSAKI Motohiro]

Hi

Thanks good comment!

> > How many times do we have to make this mistake :(
> >
> > Only the caller knows what allocation mode the callee can use. 
> > call_usermodehelper_setup() should be extended to take a gfp_t argument.
> 
> Yeah, but making the caller need to know about the internal 
> implementation details of the callee (ie, whether it needs to allocate 
> memory or not) leads to pretty warty interfaces.  In this case, you 
> could push the gfp_t up to the call_usermodehelper_setup() level, but 
> pushing it any higher wouldn't make much sense.

No problem :)
almost caller doesn't call call_usermodehelper_setup() directly.

thus, call_usermodehelper_setup() chage is hided in call_usermodehelper().


----------------chunk of my current testing patch-----------------------------

@@ -68,8 +69,9 @@ static inline int
 call_usermodehelper(char *path, char **argv, char **envp, enum umh_wait wait)
 {
 	struct subprocess_info *info;
+	gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
 
-	info = call_usermodehelper_setup(path, argv, envp);
+	info = call_usermodehelper_setup(path, argv, envp, gfp_mask);
 	if (info == NULL)
 		return -ENOMEM;
 	return call_usermodehelper_exec(info, wait);

Re: [PATCH] call_usermodehelper_setup() should use GFP_KERNEL

[Jeremy Fitzhardinge]

KOSAKI Motohiro wrote:
> Hi
>
> Thanks good comment!
>
>   
>>> How many times do we have to make this mistake :(
>>>
>>> Only the caller knows what allocation mode the callee can use. 
>>> call_usermodehelper_setup() should be extended to take a gfp_t argument.
>>>       
>> Yeah, but making the caller need to know about the internal 
>> implementation details of the callee (ie, whether it needs to allocate 
>> memory or not) leads to pretty warty interfaces.  In this case, you 
>> could push the gfp_t up to the call_usermodehelper_setup() level, but 
>> pushing it any higher wouldn't make much sense.
>>     
>
> No problem :)
> almost caller doesn't call call_usermodehelper_setup() directly.
>
> thus, call_usermodehelper_setup() chage is hided in call_usermodehelper().
>   

Yep, seems reasonable.  Are there any UMH_NO_WAIT callers who could be 
using GFP_KERNEL?

> ----------------chunk of my current testing patch-----------------------------
>
> @@ -68,8 +69,9 @@ static inline int
>  call_usermodehelper(char *path, char **argv, char **envp, enum umh_wait wait)
>  {
>  	struct subprocess_info *info;
> +	gfp_t gfp_mask = (wait == UMH_NO_WAIT) ? GFP_ATOMIC : GFP_KERNEL;
>  
> -	info = call_usermodehelper_setup(path, argv, envp);
> +	info = call_usermodehelper_setup(path, argv, envp, gfp_mask);
>  	if (info == NULL)
>  		return -ENOMEM;
>  	return call_usermodehelper_exec(info, wait);
>
>   

    J

Re: [PATCH] call_usermodehelper_setup() should use GFP_KERNEL

[KOSAKI Motohiro]

> >> Yeah, but making the caller need to know about the internal 
> >> implementation details of the callee (ie, whether it needs to allocate 
> >> memory or not) leads to pretty warty interfaces.  In this case, you 
> >> could push the gfp_t up to the call_usermodehelper_setup() level, but 
> >> pushing it any higher wouldn't make much sense.
> >
> > No problem :)
> > almost caller doesn't call call_usermodehelper_setup() directly.
> >
> > thus, call_usermodehelper_setup() chage is hided in call_usermodehelper().
> 
> Yep, seems reasonable.  Are there any UMH_NO_WAIT callers who could be 
> using GFP_KERNEL?

UMH_WAIT_EXEC and UMH_WAIT_PROC mean wait on finish of exec or process exit.
I can't imagine situation that exec is waitable *and* allocate isn't waitable.

Re: [PATCH] call_usermodehelper_setup() should use GFP_KERNEL

[Jeremy Fitzhardinge]

KOSAKI Motohiro wrote:
> UMH_WAIT_EXEC and UMH_WAIT_PROC mean wait on finish of exec or process exit.
> I can't imagine situation that exec is waitable *and* allocate isn't waitable.
>   

Yeah, that's why I was asking about UMH_NO_WAIT, where the caller may 
not want to wait, but the context would allow the allocation to block if 
necessary.

    J

Re: [PATCH] call_usermodehelper_setup() should use GFP_KERNEL

[KOSAKI Motohiro]

> KOSAKI Motohiro wrote:
> > UMH_WAIT_EXEC and UMH_WAIT_PROC mean wait on finish of exec or process exit.
> > I can't imagine situation that exec is waitable *and* allocate isn't waitable.
> 
> Yeah, that's why I was asking about UMH_NO_WAIT, where the caller may 
> not want to wait, but the context would allow the allocation to block if 
> necessary.

sorry, I misunderstood ;)

Yeah, you are right.
but it is no regression.
I agree with my plan have improve chance, but it is better than current.

Re: [PATCH] call_usermodehelper_setup() should use GFP_KERNEL

[Jeremy Fitzhardinge]

KOSAKI Motohiro wrote:
> Yeah, you are right.
> but it is no regression.
> I agree with my plan have improve chance, but it is better than current.
>   

Sure.  I was just curious and wondered if you'd already looked at it.

    J