Re: [Bugfix v2 2/5] x86/irq: Enhance __assign_irq_vector() to rollback in case of failure

[Thomas Gleixner <tglx@linutronix.de>]

On Wed, 23 Dec 2015, Jiang Liu wrote:
> @@ -167,11 +170,13 @@ next:
>  
>  		if (test_bit(vector, used_vectors))
>  			goto next;
> -
>  		for_each_cpu_and(new_cpu, vector_cpumask, cpu_online_mask) {
>  			if (!IS_ERR_OR_NULL(per_cpu(vector_irq, new_cpu)[vector]))
>  				goto next;
>  		}
> +		if (apic->cpu_mask_to_apicid_and(mask, vector_cpumask, &dest))
> +			goto next;

This is silly. If the checks in cpu_mask_to_apicid_and() fail, then there is
no point to try the next vector number simply because the checks will fail
with the next vector again. You need a new vector_cpumask() in order to make
progress.

> +
>  		/* Found one! */
>  		current_vector = vector;
>  		current_offset = offset;
> @@ -190,8 +195,7 @@ next:
>  
>  	if (!err) {
>  		/* cache destination APIC IDs into cfg->dest_apicid */
> -		err = apic->cpu_mask_to_apicid_and(mask, d->domain,
> -						   &d->cfg.dest_apicid);

I have serious doubts that this is the proper solution.

d->domain is @vector_cpumask. @vector_cpumask gets assigned by the call to
apic->vector_allocation_domain()

So the only way this can fail is when:

   vector_cpumask & mask & cpu_online_mask == 0

So we can do that check way earlier, i.e. right after the call to
apic->vector_allocation_domain(). Once we have established that this check
does not fail, we know that the call to apic->cpu_mask_to_apicid_and() wont
fail either once we have a vector number to use.

Thanks,

	tglx