lkml.org 
[lkml]   [2008]   [Jun]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] sched: CPU hotplug events must not destroy scheduler domains created by the cpusets
    I think this patch is ok --- though (1) perhaps it should have been two patches,
    as it seems to fix two bugs, and (2) I only managed to get my head about 80% of
    the way around this, so while I confident enough to Ack this one, I'm not
    as certain as I might be.

    Acked-by: Paul Jackson <pj@sgi.com>


    I did see a couple of places where the code could be more clearly presented,
    and a couple of lines ending in space characters in *.c files (the Doc files
    are hopelessly failing this space terminator test, so I gave up on them.)

    Your patch includes the following. Note the embedded "line ends in space"'s.

    > @@ -7058,6 +7071,7 @@ static int update_sched_domains(struct notifier_block *nfb,
    > case CPU_DOWN_PREPARE:
    > case CPU_DOWN_PREPARE_FROZEN:
    > detach_destroy_domains(&cpu_online_map);
    > + free_sched_domains();
    > return NOTIFY_OK;
    >
    > case CPU_UP_CANCELED:
    > @@ -7076,8 +7090,16 @@ static int update_sched_domains(struct notifier_block *nfb,
    > return NOTIFY_DONE;
    > }
    >
    > +#ifndef CONFIG_CPUSETS
    > + /*

    Above line ends in space.

    > + * Create default domain partitioning if cpusets are disabled.
    > + * Otherwise we let cpusets rebuild the domains based on the

    Above line ends in space.

    > + * current setup.
    > + */
    > +
    > /* The hotplug lock is already held by cpu_up/cpu_down */
    > arch_init_sched_domains(&cpu_online_map);
    > +#endif
    >
    > return NOTIFY_OK;
    > }


    The above patch, on top of the code already there, ends up with the
    following code for update_sched_domains():

    ============================== begin ==============================
    static int update_sched_domains(struct notifier_block *nfb,
    unsigned long action, void *hcpu)
    {
    switch (action) {
    case CPU_UP_PREPARE:
    case CPU_UP_PREPARE_FROZEN:
    case CPU_DOWN_PREPARE:
    case CPU_DOWN_PREPARE_FROZEN:
    detach_destroy_domains(&cpu_online_map);
    free_sched_domains();
    return NOTIFY_OK;

    case CPU_UP_CANCELED:
    case CPU_UP_CANCELED_FROZEN:
    case CPU_DOWN_FAILED:
    case CPU_DOWN_FAILED_FROZEN:
    case CPU_ONLINE:
    case CPU_ONLINE_FROZEN:
    case CPU_DEAD:
    case CPU_DEAD_FROZEN:
    /*
    * Fall through and re-initialise the domains.
    */
    break;
    default:
    return NOTIFY_DONE;
    }

    #ifndef CONFIG_CPUSETS
    /*
    * Create default domain partitioning if cpusets are disabled.
    * Otherwise we let cpusets rebuild the domains based on the
    * current setup.
    */

    /* The hotplug lock is already held by cpu_up/cpu_down */
    arch_init_sched_domains(&cpu_online_map);
    #endif

    return NOTIFY_OK;
    }
    =============================== end ===============================

    The "Fall through", break, ifndef and out of the switch call seem to be
    a more convoluted way of writing that routine than is necessary.

    How about:
    1) Defining arch_init_sched_domains() for all CONFIGs, CPUSET or not
    (see for example how free_sched_groups() is defined, NUMA or not)
    2) Then the ifndef can be removed from here.
    3) Then the arch_init_sched_domains() can be moved comfortably inside
    the switch statement.

    The end result would be a update_sched_domains() routine that looked
    something like the following:

    ============================== begin ==============================
    static int update_sched_domains(struct notifier_block *nfb,
    unsigned long action, void *hcpu)
    {
    switch (action) {
    case CPU_UP_PREPARE:
    case CPU_UP_PREPARE_FROZEN:
    case CPU_DOWN_PREPARE:
    case CPU_DOWN_PREPARE_FROZEN:
    detach_destroy_domains(&cpu_online_map);
    free_sched_domains();
    return NOTIFY_OK;
    case CPU_UP_CANCELED:
    case CPU_UP_CANCELED_FROZEN:
    case CPU_DOWN_FAILED:
    case CPU_DOWN_FAILED_FROZEN:
    case CPU_ONLINE:
    case CPU_ONLINE_FROZEN:
    case CPU_DEAD:
    case CPU_DEAD_FROZEN:
    arch_init_sched_domains(&cpu_online_map);
    return NOTIFY_OK;
    default:
    return NOTIFY_DONE;
    }
    }
    =============================== end ===============================


    ===

    This routine:

    static void free_sched_domains(void)
    {
    ndoms_cur = 0;
    if (doms_cur != &fallback_doms)
    kfree(doms_cur);
    doms_cur = &fallback_doms;
    }

    might be easier to read as:

    static void free_sched_domains(void)
    {
    ndoms_cur = 0;
    if (doms_cur != &fallback_doms) {
    kfree(doms_cur);
    doms_cur = &fallback_doms;
    }
    }


    --
    I won't rest till it's the best ...
    Programmer, Linux Scalability
    Paul Jackson <pj@sgi.com> 1.940.382.4214


    \
     
     \ /
      Last update: 2008-06-02 17:21    [W:0.039 / U:59.512 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site