* [PATCH] call_usermodehelper_setup() should use GFP_KERNEL
@ 2008-05-08 6:41 KOSAKI Motohiro
2008-05-08 7:23 ` Li Zefan
0 siblings, 1 reply; 12+ messages in thread
From: KOSAKI Motohiro @ 2008-05-08 6:41 UTC (permalink / raw
To: Paul Menage, Li Zefan, Jeremy Fitzhardinge, Andi Kleen, LKML,
Rusty Russell
Cc: 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;
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] call_usermodehelper_setup() should use GFP_KERNEL
2008-05-08 6:41 [PATCH] call_usermodehelper_setup() should use GFP_KERNEL KOSAKI Motohiro
@ 2008-05-08 7:23 ` Li Zefan
2008-05-08 7:31 ` KOSAKI Motohiro
0 siblings, 1 reply; 12+ messages in thread
From: Li Zefan @ 2008-05-08 7:23 UTC (permalink / raw
To: KOSAKI Motohiro
Cc: Paul Menage, Jeremy Fitzhardinge, Andi Kleen, LKML, Rusty Russell
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;
>
>
>
>
>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] call_usermodehelper_setup() should use GFP_KERNEL
2008-05-08 7:23 ` Li Zefan
@ 2008-05-08 7:31 ` KOSAKI Motohiro
2008-05-08 8:29 ` Andrew Morton
0 siblings, 1 reply; 12+ messages in thread
From: KOSAKI Motohiro @ 2008-05-08 7:31 UTC (permalink / raw
To: Li Zefan
Cc: kosaki.motohiro, Paul Menage, Jeremy Fitzhardinge, Andi Kleen,
LKML, Rusty Russell
> 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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] call_usermodehelper_setup() should use GFP_KERNEL
2008-05-08 7:31 ` KOSAKI Motohiro
@ 2008-05-08 8:29 ` Andrew Morton
2008-05-08 9:40 ` KOSAKI Motohiro
2008-05-08 10:16 ` Jeremy Fitzhardinge
0 siblings, 2 replies; 12+ messages in thread
From: Andrew Morton @ 2008-05-08 8:29 UTC (permalink / raw
To: KOSAKI Motohiro
Cc: Li Zefan, Paul Menage, Jeremy Fitzhardinge, Andi Kleen, LKML,
Rusty Russell
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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] call_usermodehelper_setup() should use GFP_KERNEL
2008-05-08 8:29 ` Andrew Morton
@ 2008-05-08 9:40 ` KOSAKI Motohiro
2008-05-08 10:16 ` Jeremy Fitzhardinge
1 sibling, 0 replies; 12+ messages in thread
From: KOSAKI Motohiro @ 2008-05-08 9:40 UTC (permalink / raw
To: Andrew Morton
Cc: kosaki.motohiro, Li Zefan, Paul Menage, Jeremy Fitzhardinge, LKML,
Rusty Russell
> > > 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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] call_usermodehelper_setup() should use GFP_KERNEL
2008-05-08 8:29 ` Andrew Morton
2008-05-08 9:40 ` KOSAKI Motohiro
@ 2008-05-08 10:16 ` Jeremy Fitzhardinge
2008-05-08 10:38 ` KOSAKI Motohiro
1 sibling, 1 reply; 12+ messages in thread
From: Jeremy Fitzhardinge @ 2008-05-08 10:16 UTC (permalink / raw
To: Andrew Morton
Cc: KOSAKI Motohiro, Li Zefan, Paul Menage, Jeremy Fitzhardinge,
Andi Kleen, LKML, Rusty Russell
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] call_usermodehelper_setup() should use GFP_KERNEL
2008-05-08 10:16 ` Jeremy Fitzhardinge
@ 2008-05-08 10:38 ` KOSAKI Motohiro
2008-05-08 12:58 ` Jeremy Fitzhardinge
0 siblings, 1 reply; 12+ messages in thread
From: KOSAKI Motohiro @ 2008-05-08 10:38 UTC (permalink / raw
To: Jeremy Fitzhardinge
Cc: kosaki.motohiro, Andrew Morton, Li Zefan, Paul Menage,
Jeremy Fitzhardinge, LKML, Rusty Russell
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);
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] call_usermodehelper_setup() should use GFP_KERNEL
2008-05-08 10:38 ` KOSAKI Motohiro
@ 2008-05-08 12:58 ` Jeremy Fitzhardinge
2008-05-08 23:46 ` KOSAKI Motohiro
0 siblings, 1 reply; 12+ messages in thread
From: Jeremy Fitzhardinge @ 2008-05-08 12:58 UTC (permalink / raw
To: KOSAKI Motohiro
Cc: Andrew Morton, Li Zefan, Paul Menage, Jeremy Fitzhardinge, LKML,
Rusty Russell
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] call_usermodehelper_setup() should use GFP_KERNEL
2008-05-08 12:58 ` Jeremy Fitzhardinge
@ 2008-05-08 23:46 ` KOSAKI Motohiro
2008-05-08 23:53 ` Jeremy Fitzhardinge
0 siblings, 1 reply; 12+ messages in thread
From: KOSAKI Motohiro @ 2008-05-08 23:46 UTC (permalink / raw
To: Jeremy Fitzhardinge
Cc: kosaki.motohiro, Andrew Morton, Li Zefan, Paul Menage,
Jeremy Fitzhardinge, LKML, Rusty Russell
> >> 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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] call_usermodehelper_setup() should use GFP_KERNEL
2008-05-08 23:46 ` KOSAKI Motohiro
@ 2008-05-08 23:53 ` Jeremy Fitzhardinge
2008-05-09 0:04 ` KOSAKI Motohiro
0 siblings, 1 reply; 12+ messages in thread
From: Jeremy Fitzhardinge @ 2008-05-08 23:53 UTC (permalink / raw
To: KOSAKI Motohiro
Cc: Andrew Morton, Li Zefan, Paul Menage, Jeremy Fitzhardinge, LKML,
Rusty Russell
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] call_usermodehelper_setup() should use GFP_KERNEL
2008-05-08 23:53 ` Jeremy Fitzhardinge
@ 2008-05-09 0:04 ` KOSAKI Motohiro
2008-05-09 0:08 ` Jeremy Fitzhardinge
0 siblings, 1 reply; 12+ messages in thread
From: KOSAKI Motohiro @ 2008-05-09 0:04 UTC (permalink / raw
To: Jeremy Fitzhardinge
Cc: kosaki.motohiro, Andrew Morton, Li Zefan, Paul Menage,
Jeremy Fitzhardinge, LKML, Rusty Russell
> 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.
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] call_usermodehelper_setup() should use GFP_KERNEL
2008-05-09 0:04 ` KOSAKI Motohiro
@ 2008-05-09 0:08 ` Jeremy Fitzhardinge
0 siblings, 0 replies; 12+ messages in thread
From: Jeremy Fitzhardinge @ 2008-05-09 0:08 UTC (permalink / raw
To: KOSAKI Motohiro
Cc: Andrew Morton, Li Zefan, Paul Menage, Jeremy Fitzhardinge, LKML,
Rusty Russell
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
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2008-05-09 0:08 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-05-08 6:41 [PATCH] call_usermodehelper_setup() should use GFP_KERNEL KOSAKI Motohiro
2008-05-08 7:23 ` Li Zefan
2008-05-08 7:31 ` KOSAKI Motohiro
2008-05-08 8:29 ` Andrew Morton
2008-05-08 9:40 ` KOSAKI Motohiro
2008-05-08 10:16 ` Jeremy Fitzhardinge
2008-05-08 10:38 ` KOSAKI Motohiro
2008-05-08 12:58 ` Jeremy Fitzhardinge
2008-05-08 23:46 ` KOSAKI Motohiro
2008-05-08 23:53 ` Jeremy Fitzhardinge
2008-05-09 0:04 ` KOSAKI Motohiro
2008-05-09 0:08 ` Jeremy Fitzhardinge
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.