Date: Wed, 30 Dec 2015 19:52:20 +0100 (CET)
From: Thomas Gleixner <tglx@linutronix.de>
To: Jiang Liu <jiang.liu@linux.intel.com>
cc: Joe Lawrence <joe.lawrence@stratus.com>, Ingo Molnar <mingo@redhat.com>,
        "H. Peter Anvin" <hpa@zytor.com>, x86@kernel.org,
        Jeremiah Mahler <jmmahler@gmail.com>, Borislav Petkov <bp@alien8.de>,
        andy.shevchenko@gmail.com, Guenter Roeck <linux@roeck-us.net>,
        linux-kernel@vger.kernel.org
Subject: Re: [Bugfix v2 2/5] x86/irq: Enhance __assign_irq_vector() to rollback
 in case of failure
In-Reply-To: <1450880014-11741-2-git-send-email-jiang.liu@linux.intel.com>
Message-ID: <alpine.DEB.2.11.1512301947370.28591@nanos>
References: <alpine.DEB.2.11.1512192130450.28591@nanos> <1450880014-11741-1-git-send-email-jiang.liu@linux.intel.com> <1450880014-11741-2-git-send-email-jiang.liu@linux.intel.com>
User-Agent: Alpine 2.11 (DEB 23 2013-08-11)
MIME-Version: 1.0
Content-Type: TEXT/PLAIN; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1684
Lines: 51

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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/