Re: [PATCH] Fix interrupt distribution in ppc970

[Michael Ellerman <michael@ellerman.id.au>]

[-- Attachment #1: Type: text/plain, Size: 3278 bytes --]

On Tue, 2007-04-10 at 11:59 -0500, Milton Miller wrote:
> On Apr 10, 2007, at 2:06 AM, Michael Ellerman wrote:
> 
> > On Mon, 2007-04-09 at 14:27 +0530, Mohan Kumar M wrote:
> >> On Wed, Mar 07, 2007 at 11:52:32AM +0100, Michael Ellerman wrote:
> >>> There's already maxcpus in init/main.c, that would probably be 
> >>> better,
> >>> though still ugly.
> >>>
> >> Based on Mike's suggestions, I modified the patch. The attached patch
> >> refers max_cpus variable to check whether the kernel is booted with
> >> maxcpus=1 parameter and if maxcpus=1 is specified the patch assigns 
> >> only
> >> the current boot cpu to be the default distribution server.
> >
> > So the core of the problem is that if we haven't onlined all cpus then
> > we can't use the default_distrib_server value given to us by firmware,
> > because some of the cpus in that queue won't be online.
> >
> > We can detect this situation by comparing the number of cpus that are
> > online vs the number that are present (not possible). This might even
> > work if you boot with maxcpus=1 and then hotplug the rest in.
> >
> > How about this:
> >
> > Index: powerpc/arch/powerpc/platforms/pseries/xics.c
> > ===================================================================
> > --- powerpc.orig/arch/powerpc/platforms/pseries/xics.c
> > +++ powerpc/arch/powerpc/platforms/pseries/xics.c
> > @@ -167,7 +167,10 @@ static int get_irq_server(unsigned int v
> >  		return default_server;
> >
> >  	if (cpus_equal(cpumask, CPU_MASK_ALL)) {
> > -		server = default_distrib_server;
> > +		if (num_online_cpus() == num_present_cpus())
> > +			server = default_distrib_server;
> > +		else
> > +			server = default_server;
> >  	} else {
> >  		cpus_and(tmp, cpu_online_map, cpumask);
> 
> 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.

> >
> > @@ -415,7 +418,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 (num_online_cpus() == num_present_cpus())
> > +			newmask = default_distrib_server;
> > +		else
> > +			newmask = default_server;
> >  	} else {
> >  		cpus_and(tmp, cpu_online_map, cpumask);
> >  		if (cpus_empty(tmp))
> 
> 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'll try to code this up, but it might be a day or two until i get
> the time.

Yeah it annoyed me that the code was so similar, but it is a small but
important semantic difference.

I'll let you code it up, you know that code better than me.

cheers

-- 
Michael Ellerman
OzLabs, IBM Australia Development Lab

wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)

We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 189 bytes --]