Re: [PATCH 2/4] x86: fix cpu_mask_to_apicid_and to include cpu_online_mask

[Mike Travis <travis@sgi.com>]

Rusty Russell wrote:
> On Thursday 11 December 2008 21:58:08 Mike Travis wrote:
>> Impact: fix potential problem.
>>
>> In determining the destination apicid, there are usually three cpumasks
>> that are considered: the incoming cpumask arg, cfg->domain and the
>> cpu_online_mask.  Since we are just introducing the cpu_mask_to_apicid_and
>> function, make sure it includes the cpu_online_mask in it's evaluation.
> 
> Yerk.  Can we really "fail" cpu_mask_to_apicid_and with no repercussions?
> And does it make sense to try to fix this there?

Fail?  The only failure is if there is not a cpu that satisfies the conjunction
of the three masks, in which case it returns BAD_APICID.

The old procedure was to:

function(..., cpumask_t mask)
{
	cpumask_t tmp;

	cpus_and(tmp, mask, cfg->domain);
	...
	cpus_and(tmp, tmp, cpu_online_map);
	dest = cpu_mask_to_apicid(tmp);
	...
}

So making cpu_mask_to_apicid_and return:

	dest = cpu_mask_to_apicid(mask1 & mask2 & cpu_online_mask);

maintains this compatibility.

Turns out that there are two functions that did not AND all three masks,
but those two used TARGET_CPUS as one of the arguments.  All the x86
variations except NUMAQ, only return TARGET_CPUS which are online
(and that's probably a mistake in NUMAQ.)  So the above AND'ing is a
NOP in these cases and harmless.

> 
> This is not a new problem with the cpumask patches is it?  I toyed with a
> patch which converts flush_tlb_others, and it actually ensures that those
> cases never hand an offline mask to cpu_mask_to_apicid_and as a side
> effect (others still might).

No, I introduced the problem when I added cpu_mask_to_apicid_and(), and I
wanted to fix it before it was "officially" released.  I've been doing
benchmark testing with random HOTPLUG off and on events, and it's becoming
clear that setting the destination apicid to an off line cpu is definitely
a no-no. ;-)

Thanks,
Mike