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:2.063 / U:0.000 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site