Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Wed, 27 Apr 2022 14:01:55 +0800
From:   Pingfan Liu <kernelfans@gmail.com>
To:     Thomas Gleixner <tglx@linutronix.de>
Cc:     linux-kernel@vger.kernel.org,
        Valentin Schneider <valentin.schneider@arm.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Frederic Weisbecker <frederic@kernel.org>,
        Baokun Li <libaokun1@huawei.com>,
        Mark Rutland <mark.rutland@arm.com>,
        Steven Price <steven.price@arm.com>,
        "Jason A. Donenfeld" <Jason@zx2c4.com>,
        Yuan ZhaoXiong <yuanzhaoxiong@baidu.com>,
        YueHaibing <yuehaibing@huawei.com>,
        Randy Dunlap <rdunlap@infradead.org>
Subject: Re: [PATCH 7/9] irq: remove needless lock in takedown_cpu()
Message-ID: <YmjcUxvct7aw82DH@piliu.users.ipa.redhat.com>
References: <20220420140521.45361-1-kernelfans@gmail.com>
 <20220420140521.45361-8-kernelfans@gmail.com>
 <87y1zys9f7.ffs@tglx>
 <YmYOAfz3Oh1bYiVi@piliu.users.ipa.redhat.com>
 <87czh533dk.ffs@tglx>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <87czh533dk.ffs@tglx>
Precedence: bulk

On Mon, Apr 25, 2022 at 11:43:03AM +0200, Thomas Gleixner wrote:
> On Mon, Apr 25 2022 at 10:57, Pingfan Liu wrote:
> > On Thu, Apr 21, 2022 at 06:11:56PM +0200, Thomas Gleixner wrote:
> >> > -	irq_lock_sparse();
> >> 
> >> Not everything is about RCU here. You really need to look at all moving
> >> parts:
> >> 
> >> irq_migrate_all_off_this_cpu() relies on the allocated_irqs bitmap and
> >> the sparse tree to be in consistent state, which is only guaranteed when
> >> the sparse lock is held.
> >> 
> >
> > For the irq which transfer from active to inactive(disappearing) after
> > fetching, desc->lock can serve the sync purpose. In this case,
> > irq_lock_sparse() is not needed. For a emergeing irq, I am not sure
> > about it.
> 
> No, it's required for the free case. The alloc case is
> uninteresting. Care to look into the code?
> 

Yes, it is a good exercise. Thanks for the enlightenment.

> irq_free_descs()
>    lock(sparse);
>    free_descs();
>    bitmap_clear(allocated_irqs, from, cnt);
>    unlock_sparse);
>  
> As free_descs() sets the sparse tree entry to NULL, up to the point
> where bitmap_clear() finishes the state is inconsistent.
> 
> Now look at irq_migrate_all_off_this_cpu() and figure out what happens
> when stop_machine() hits into the inconsistent state.
> 

So the following code should fix the inconsistence between bitmap and
sparse tree.
diff --git a/kernel/irq/cpuhotplug.c b/kernel/irq/cpuhotplug.c
index 1ed2b1739363..cd0d180f082d 100644
--- a/kernel/irq/cpuhotplug.c
+++ b/kernel/irq/cpuhotplug.c
@@ -161,6 +161,8 @@ void irq_migrate_all_off_this_cpu(void)
                bool affinity_broken;

                desc = irq_to_desc(irq);
+               if (!desc)
+                       continue;
                raw_spin_lock(&desc->lock);
                affinity_broken = migrate_one_irq(desc);
                raw_spin_unlock(&desc->lock);

> This can be fixed, but not by making mysterious claims about RCU and
> desc->lock.
> 

But I still think that desc->lock is critical to the consistence of the
irq _affinity_ if removing sparse lock in takedown_cpu().

For the free case, after applying the above patch, it should work.
void irq_migrate_all_off_this_cpu(void)
{
	for_each_active_irq(irq) {

		desc = irq_to_desc(irq);
		if (!desc)
			continue;
			                               ---> if breaking
						       in by free, then
						       migrate_one_irq()
						       will skip it
						       since the irq is
						       not activated any
						       long
		raw_spin_lock(&desc->lock);
		affinity_broken = migrate_one_irq(desc);
		raw_spin_unlock(&desc->lock);
		...
	}
}

But for the alloc case, it could be a problem.
void irq_migrate_all_off_this_cpu(void)
{
	for_each_active_irq(irq) {

		desc = irq_to_desc(irq);
		if (!desc)
			continue;
		raw_spin_lock(&desc->lock);
		affinity_broken = migrate_one_irq(desc);
		raw_spin_unlock(&desc->lock);
		...
                                                   ---> any new irq will
						   not be detected. But alloc_descs(start, cnt, node, affinity)
						   still associate the
						   irq with this cpu.
						   There is _no_
						   opportunity to clear
						   out this cpu from
						   desc->irq_common_data.affinity.

						   This is the affinity
						   inconsistent problem.
}


Thanks,

	Pingfan