RE: [PATCH] (2/3) Initial load balancing

[Pallipadi, Venkatesh]

> -----Original Message-----
> From: Martin J. Bligh [mailto:[e-mail]] 
> -#ifdef CONFIG_NUMA
> +#if CONFIG_NUMA
> +/*
> + * If dest_cpu is allowed for this process, migrate the task to it.
> + * This is accomplished by forcing the cpu_allowed mask to only
> + * allow dest_cpu, which will force the cpu onto dest_cpu.  Then
> + * the cpu_allowed mask is restored.
> + */
> +static void sched_migrate_task(task_t *p, int dest_cpu)
> +{
> +	unsigned long old_mask;
> +
> +	old_mask = p->cpus_allowed;
> +	if (!(old_mask & (1UL << dest_cpu)))
> +		return;
> +	/* force the process onto the specified CPU */
> +	set_cpus_allowed(p, 1UL << dest_cpu);
> +
> +	/* restore the cpus allowed mask */
> +	set_cpus_allowed(p, old_mask);
> +}

It may be better to add a _note_ to this function saying that it is not
supposed
to be called by multiple callers at the same time. As of now, as it is
called 
at exec time only, I think it is safe. But, if it get used at other
places,
(or called once+preempt) we may have situations where we may loose the
cpus_allowed mask
or miss some sched_migrate_task(). I am looking at, what if some
sched_migrate_task() 
or user set_affinity gets initiated in between two set_cpus_allowed in
this routine. 

> +
> +/*
> + * Find the least loaded CPU.  Slightly favor the current CPU by
> + * setting its runqueue length as the minimum to start.
> + */
> +static int sched_best_cpu(struct task_struct *p)
> +{
> +	int i, minload, load, best_cpu, node = 0;
> +	unsigned long cpumask;
> +
> +	best_cpu = task_cpu(p);
> +	if (cpu_rq(best_cpu)->nr_running <= 2)
> +		return best_cpu;
> +
> +	minload = 10000000;
> +	for (i = 0; i < numnodes; i++) {
> +		load = atomic_read(&node_nr_running[i]);
> +		if (load < minload) {
> +			minload = load;
> +			node = i;
> +		}
> +	}
> +
> +	minload = 10000000;

Can we use some common #defines instead of numbers here. Also, is
There anything magical about this number or should we use MAXINT?

I saw a similar number used in patch 3. 
(100*load > NODE_THRESHOLD*this_load)
Can it be moved to #defined as well. Is this test just to find out the
25%
significant difference? A line of comment may help..

Otherwise patch looks simple and neat. And should help in HT case too.

Thanks,
-Venkatesh