Re: [PATCH] sched: CPU hotplug events must not destroy scheduler domains created by the cpusets

[Paul Jackson <pj@sgi.com>]

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