Re: [PATCH] Fix interrupt distribution in ppc970

[Milton Miller <miltonm@bga.com>]

On Apr 19, 2007, at 6:52 AM, Mohan Kumar M wrote:

> On Wed, Apr 11, 2007 at 11:16:41AM +1000, Michael Ellerman wrote:
>> On Tue, 2007-04-10 at 11:59 -0500, Milton Miller wrote:
>>>
>>> This means we are doing population counts of two masks, when we 
>>> really
>>> just care that they are the same.   How about using
>>> cpus_equal(cpu_online_mask, cpu_present_mask)?
>>
>> Yep that's sensible.
>>
>>> This shows how close these two functions are.  The difference is
>>> what happens when cpus_empty is true -- we default in one and
>>> ignore in the other.
>>>
>>> How about adding another arg to get_server, that says to fail
>>> or default for the empty case, with failure being -1?
>
> I modified the patch based on suggestions given by Milton and Mike.
> Milton is this patch okay?

Well, its part of the way there.

>
> Tested on 2.6.21-rc7.
> Index: linux-2.6.21-rc4/arch/powerpc/platforms/pseries/xics.c
> ===================================================================
> --- linux-2.6.21-rc4.orig/arch/powerpc/platforms/pseries/xics.c
> +++ linux-2.6.21-rc4/arch/powerpc/platforms/pseries/xics.c
> @@ -156,9 +156,9 @@ static inline void lpar_qirr_info(int n_
>
>
>  #ifdef CONFIG_SMP
> -static int get_irq_server(unsigned int virq)
> +static int get_irq_server(unsigned int virq, unsigned int 
> strict_check)
>  {
> -	unsigned int server;
> +	int server;
>  	/* For the moment only implement delivery to all cpus or one cpu */
>  	cpumask_t cpumask = irq_desc[virq].affinity;
>  	cpumask_t tmp = CPU_MASK_NONE;
> @@ -167,21 +167,25 @@ static int get_irq_server(unsigned int v
>  		return default_server;
>
>  	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
> -		server = default_distrib_server;
> +		if (cpus_equal(cpu_online_map, cpu_present_map))
> +			server = default_distrib_server;
> +		else
> +			server = default_server;
>  	} else {
>  		cpus_and(tmp, cpu_online_map, cpumask);
>
> -		if (cpus_empty(tmp))
> +		if (cpus_empty(tmp) && !strict_check)
>  			server = default_distrib_server;
> +		else if(cpus_empty(tmp) && strict_check)
> +			server = -1;
>  		else
>  			server = get_hard_smp_processor_id(first_cpu(tmp));
>  	}

Don't test cpus_empty twice, instead do a nested if.

Actually, looking at this a bit, you need your default_distrib or
default_server check again.

How about reorder to be if (!cpus_equal(...ALL)) {
     cpus_and
     if !cpus_empty(tmp
         return get_hard(first);
     if (strict)
         return -1;
}

if (cpus_equal(online, preseent)
return distrib
return default_server

For that matter, can we just call first_cpu, and check its <= NUM_CPUS
elminating the call to cpus_empty ?   Only call get_hard_cpu if its
valid.

...
> @@ -415,7 +419,10 @@ static void xics_set_affinity(unsigned i
...
>  	/* For the moment only implement delivery to all cpus or one cpu */
>  	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
> -		newmask = default_distrib_server;
> +		if (cpus_equal(cpu_online_map, cpu_present_map))
>

this was supposed to be the call with strict = 1

milton