lkml.org 
[lkml]   [2012]   [May]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH] pidmap: Use GFP_ATOMIC to allocate page (was: Re: [ 00/54] 3.0.32-stable review)
On Sun, 20 May 2012, Linus Torvalds wrote:

> > Why wasn't this caught by gfp_allowed_mask in slab_pre_alloc_hook()?
> > GFP_KERNEL should be allowed in this context.
>
> We set gfp_allowed_mask to allow all allocations before this point: it
> happens when we enable interrupts fairly early during start_kernel().
>
> So by the time pidmap_init() is called, GFP_KERNEL does imply that
> scheduling can happen.
>

Steven notes that he's using CONFIG_PREEMPT_VOLUNTARY which means all
might_sleep()'s in the kernel actually get turned into cond_resched()'s.
I had thought that might_sleep() and might_sleep_if() such as in the slab
allocators were simply for CONFIG_DEBUG_ATOMIC_SLEEP, but it has been
hijacked for other purposes. Do we really want to infuse cond_resched()'s
on every slab allocation for desktop users who are urged in the Kconfig to
enable CONFIG_PREEMPT_VOLUNTARY?

> Which does imply that we set gfp_allowed_mask *much* too early. We
> still cannot schedule at that point (well, at least there's a comment
> saying so):
>
> /*
> * Disable preemption - early bootup scheduling is extremely
> * fragile until we cpu_idle() for the first time.
> */
> preempt_disable();
>
> so logically we should move the gfp_allowed_mask setting down to where
> we really are properly alive.
>
> How about moving it down to after we've done the full smp_init() and
> after we've actually done the first schedule and have proper idle
> CPU's?
>
> Something like the attached (UNTESTED!) patch?
>
> init/main.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/init/main.c b/init/main.c
> index 44b2433334c7..0cf00943d755 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -560,9 +560,6 @@ asmlinkage void __init start_kernel(void)
> early_boot_irqs_disabled = false;
> local_irq_enable();
>
> - /* Interrupts are enabled now so all GFP allocations are safe. */
> - gfp_allowed_mask = __GFP_BITS_MASK;
> -
> kmem_cache_init_late();
>
> /*
> @@ -861,6 +858,9 @@ static int __init kernel_init(void * unused)
> smp_init();
> sched_init_smp();
>
> + /* Now we're finally set up and can do any allocation */
> + gfp_allowed_mask = __GFP_BITS_MASK;
> +
> do_basic_setup();
>
> /* Open the /dev/console on the rootfs, this should never fail */

I think this may be unnecessarily too late; smp_init() will rely on the
arch-dependent cpu_up to guarantee that cpu_idle() has been called and
sched_init_smp() seems to think we can do GFP_KERNEL.

So putting this in between smp_init() and sched_init_smp() may be the
first time we can absolutely guarantee that we are scheduable. BUT, we
typically become schedulable when kernel_init() returns from the
wait_for_completion(&kthreadd_done) because cpu_idle() has been called in
rest_init() right after doing complete(&kthreadd_done) so it's slightly
racy.

I think we should look in between that wait_for_completion() and
smp_init() to determine if there's anything that would benefit from
GFP_KERNEL over GFP_NOWAIT and I can't see anything. I do think
sched_init_smp() unnecessarily suppresses __GFP_WAIT with your patch, so
perhaps it should be moved right before it if we're actually going to
allow CONFIG_PREEMPT_VOLUNTARY to schedule on might_sleep()'s.


\
 
 \ /
  Last update: 2012-05-21 01:41    [W:0.213 / U:1.668 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site