Date: Wed, 30 Dec 2015 18:25:35 +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 4/5] x86/irq: Fix a race condition between vector
 assigning and cleanup
In-Reply-To: <1450880014-11741-4-git-send-email-jiang.liu@linux.intel.com>
Message-ID: <alpine.DEB.2.11.1512301814030.28591@nanos>
References: <alpine.DEB.2.11.1512192130450.28591@nanos> <1450880014-11741-1-git-send-email-jiang.liu@linux.intel.com> <1450880014-11741-4-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: 2248
Lines: 68

On Wed, 23 Dec 2015, Jiang Liu wrote:
>  static void clear_irq_vector(int irq, struct apic_chip_data *data)
>  {
> -	struct irq_desc *desc;
> +	struct irq_desc *desc = irq_to_desc(irq);
>  	int cpu, vector = data->cfg.vector;
>  
>  	BUG_ON(!vector);
> @@ -236,10 +235,6 @@ static void clear_irq_vector(int irq, struct apic_chip_data *data)
>  	data->cfg.vector = 0;
>  	cpumask_clear(data->domain);
>  
> -	if (likely(!data->move_in_progress))
> -		return;

Why are you removing this?

> -
> -	desc = irq_to_desc(irq);
>  	for_each_cpu_and(cpu, data->old_domain, cpu_online_mask) {
>  		for (vector = FIRST_EXTERNAL_VECTOR; vector < NR_VECTORS;
>  		     vector++) {
> @@ -421,10 +416,13 @@ static void __setup_vector_irq(int cpu)
>  		struct irq_data *idata = irq_desc_get_irq_data(desc);
>  
>  		data = apic_chip_data(idata);
> -		if (!data || !cpumask_test_cpu(cpu, data->domain))
> -			continue;
> -		vector = data->cfg.vector;
> -		per_cpu(vector_irq, cpu)[vector] = desc;
> +		if (data) {
> +			cpumask_clear_cpu(cpu, data->old_domain);

Why would the newly online cpu be in data->old_domain?

> +			if (cpumask_test_cpu(cpu, data->domain)) {
> +				vector = data->cfg.vector;
> +				per_cpu(vector_irq, cpu)[vector] = desc;
> +			}
> +		}
> @@ -563,14 +558,10 @@ asmlinkage __visible void smp_irq_move_cleanup_interrupt(void)
>  			goto unlock;
>  
>  		/*
> -		 * Check if the irq migration is in progress. If so, we
> -		 * haven't received the cleanup request yet for this irq.
> +		 * Nothing to cleanup if this cpu is not set
> +		 * in the old_domain mask.
>  		 */
> -		if (data->move_in_progress)
> -			goto unlock;

Removing this is broken. If data->move_in_progress is set, then you cannot
clear the vector. If there are two interrupt moves pending then the IPI of the
first one will clear the second one as well, which might be still targeted to
this cpu.

This whole patch set is way too complex. A lot of tiny changes here and there
and none of them is documented.

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/