All of lore.kernel.org
 help / color / mirror / Atom feed
From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Stefan Wiehler <stefan.wiehler@nokia.com>, Will Deacon <will@kernel.org>
Cc: Joel Fernandes <joel@joelfernandes.org>,
	"Paul E. McKenney" <paulmck@kernel.org>,
	Josh Triplett <josh@joshtriplett.org>,
	Boqun Feng <boqun.feng@gmail.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
	Lai Jiangshan <jiangshanlai@gmail.com>,
	Zqiang <qiang.zhang1211@gmail.com>,
	linux-arm-kernel@lists.infradead.org, rcu@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] arm: smp: Avoid false positive CPU hotplug Lockdep-RCU splat
Date: Sat, 9 Mar 2024 09:57:04 +0000	[thread overview]
Message-ID: <ZewycILled+mZhwe@shell.armlinux.org.uk> (raw)
In-Reply-To: <66fdce3a-c7f6-4ef4-ab56-7c9ece0b00e2@nokia.com>

On Sat, Mar 09, 2024 at 08:45:35AM +0100, Stefan Wiehler wrote:
> > I agree with the problem but disagree with the patch because it feels like a
> > terrible workaround.
> > 
> > Can we just use arch_spin_lock() for the cpu_asid_lock? This might require
> > acquiring the raw_lock within the raw_spinlock_t, but there is precedent:
> > 
> > arch/powerpc/kvm/book3s_hv_rm_mmu.c:245:
> > arch_spin_lock(&kvm->mmu_lock.rlock.raw_lock);
> > 
> > IMO, lockdep tracking of this lock is not necessary or possible considering the
> > hotplug situation.
> > 
> > Or is there a reason you need lockdep working for the cpu_asid_lock?
> 
> I was not aware of this possibility to bypass lockdep tracing, but this seems
> to work and indeed looks like less of a workaround:
> 
> diff --git a/arch/arm/mm/context.c b/arch/arm/mm/context.c
> index 4204ffa2d104..4fc2c559f1b6 100644
> --- a/arch/arm/mm/context.c
> +++ b/arch/arm/mm/context.c
> @@ -254,7 +254,8 @@ void check_and_switch_context(struct mm_struct *mm, struct task_struct *tsk)
>             && atomic64_xchg(&per_cpu(active_asids, cpu), asid))
>                 goto switch_mm_fastpath;
> 
> -       raw_spin_lock_irqsave(&cpu_asid_lock, flags);
> +       local_irq_save(flags);
> +       arch_spin_lock(&cpu_asid_lock.raw_lock);
>         /* Check that our ASID belongs to the current generation. */
>         asid = atomic64_read(&mm->context.id);
>         if ((asid ^ atomic64_read(&asid_generation)) >> ASID_BITS) {
> @@ -269,7 +270,8 @@ void check_and_switch_context(struct mm_struct *mm, struct task_struct *tsk)
> 
>         atomic64_set(&per_cpu(active_asids, cpu), asid);
>         cpumask_set_cpu(cpu, mm_cpumask(mm));
> -       raw_spin_unlock_irqrestore(&cpu_asid_lock, flags);
> +       arch_spin_unlock(&cpu_asid_lock.raw_lock);
> +       local_irq_restore(flags);
> 
>  switch_mm_fastpath:
>         cpu_switch_mm(mm->pgd, mm);
> 
> @Russell, what do you think?

I think this is Will Deacon's code, so we ought to hear from Will...

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

WARNING: multiple messages have this Message-ID (diff)
From: "Russell King (Oracle)" <linux@armlinux.org.uk>
To: Stefan Wiehler <stefan.wiehler@nokia.com>, Will Deacon <will@kernel.org>
Cc: Joel Fernandes <joel@joelfernandes.org>,
	"Paul E. McKenney" <paulmck@kernel.org>,
	Josh Triplett <josh@joshtriplett.org>,
	Boqun Feng <boqun.feng@gmail.com>,
	Steven Rostedt <rostedt@goodmis.org>,
	Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
	Lai Jiangshan <jiangshanlai@gmail.com>,
	Zqiang <qiang.zhang1211@gmail.com>,
	linux-arm-kernel@lists.infradead.org, rcu@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH] arm: smp: Avoid false positive CPU hotplug Lockdep-RCU splat
Date: Sat, 9 Mar 2024 09:57:04 +0000	[thread overview]
Message-ID: <ZewycILled+mZhwe@shell.armlinux.org.uk> (raw)
In-Reply-To: <66fdce3a-c7f6-4ef4-ab56-7c9ece0b00e2@nokia.com>

On Sat, Mar 09, 2024 at 08:45:35AM +0100, Stefan Wiehler wrote:
> > I agree with the problem but disagree with the patch because it feels like a
> > terrible workaround.
> > 
> > Can we just use arch_spin_lock() for the cpu_asid_lock? This might require
> > acquiring the raw_lock within the raw_spinlock_t, but there is precedent:
> > 
> > arch/powerpc/kvm/book3s_hv_rm_mmu.c:245:
> > arch_spin_lock(&kvm->mmu_lock.rlock.raw_lock);
> > 
> > IMO, lockdep tracking of this lock is not necessary or possible considering the
> > hotplug situation.
> > 
> > Or is there a reason you need lockdep working for the cpu_asid_lock?
> 
> I was not aware of this possibility to bypass lockdep tracing, but this seems
> to work and indeed looks like less of a workaround:
> 
> diff --git a/arch/arm/mm/context.c b/arch/arm/mm/context.c
> index 4204ffa2d104..4fc2c559f1b6 100644
> --- a/arch/arm/mm/context.c
> +++ b/arch/arm/mm/context.c
> @@ -254,7 +254,8 @@ void check_and_switch_context(struct mm_struct *mm, struct task_struct *tsk)
>             && atomic64_xchg(&per_cpu(active_asids, cpu), asid))
>                 goto switch_mm_fastpath;
> 
> -       raw_spin_lock_irqsave(&cpu_asid_lock, flags);
> +       local_irq_save(flags);
> +       arch_spin_lock(&cpu_asid_lock.raw_lock);
>         /* Check that our ASID belongs to the current generation. */
>         asid = atomic64_read(&mm->context.id);
>         if ((asid ^ atomic64_read(&asid_generation)) >> ASID_BITS) {
> @@ -269,7 +270,8 @@ void check_and_switch_context(struct mm_struct *mm, struct task_struct *tsk)
> 
>         atomic64_set(&per_cpu(active_asids, cpu), asid);
>         cpumask_set_cpu(cpu, mm_cpumask(mm));
> -       raw_spin_unlock_irqrestore(&cpu_asid_lock, flags);
> +       arch_spin_unlock(&cpu_asid_lock.raw_lock);
> +       local_irq_restore(flags);
> 
>  switch_mm_fastpath:
>         cpu_switch_mm(mm->pgd, mm);
> 
> @Russell, what do you think?

I think this is Will Deacon's code, so we ought to hear from Will...

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  reply	other threads:[~2024-03-09  9:57 UTC|newest]

Thread overview: 37+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-03-07 16:09 [PATCH] arm: smp: Avoid false positive CPU hotplug Lockdep-RCU splat Stefan Wiehler
2024-03-07 16:09 ` Stefan Wiehler
2024-03-07 17:45 ` Paul E. McKenney
2024-03-07 17:45   ` Paul E. McKenney
2024-03-07 17:49   ` Paul E. McKenney
2024-03-07 17:49     ` Paul E. McKenney
2024-03-07 18:54   ` Russell King (Oracle)
2024-03-07 18:54     ` Russell King (Oracle)
2024-03-07 19:08     ` Paul E. McKenney
2024-03-07 19:08       ` Paul E. McKenney
2024-03-07 18:49 ` Boqun Feng
2024-03-07 18:49   ` Boqun Feng
2024-03-08 10:20   ` Stefan Wiehler
2024-03-08 10:20     ` Stefan Wiehler
2024-03-08 14:57 ` Joel Fernandes
2024-03-08 14:57   ` Joel Fernandes
2024-03-09  7:45   ` Stefan Wiehler
2024-03-09  7:45     ` Stefan Wiehler
2024-03-09  9:57     ` Russell King (Oracle) [this message]
2024-03-09  9:57       ` Russell King (Oracle)
2024-03-12 22:14       ` Will Deacon
2024-03-12 22:14         ` Will Deacon
2024-03-12 22:39         ` Russell King (Oracle)
2024-03-12 22:39           ` Russell King (Oracle)
2024-03-13  0:32           ` Will Deacon
2024-03-13  0:32             ` Will Deacon
2024-03-13  9:58             ` Russell King (Oracle)
2024-03-13  9:58               ` Russell King (Oracle)
2024-08-09 13:37               ` Stefan Wiehler
2024-08-09 13:48                 ` Russell King (Oracle)
2024-08-09 14:06                   ` Stefan Wiehler
2024-03-11 16:08 ` Russell King (Oracle)
2024-03-11 16:08   ` Russell King (Oracle)
2024-03-11 16:17   ` Stefan Wiehler
2024-03-11 16:17     ` Stefan Wiehler
2024-03-11 19:01     ` Paul E. McKenney
2024-03-11 19:01       ` Paul E. McKenney

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=ZewycILled+mZhwe@shell.armlinux.org.uk \
    --to=linux@armlinux.org.uk \
    --cc=boqun.feng@gmail.com \
    --cc=jiangshanlai@gmail.com \
    --cc=joel@joelfernandes.org \
    --cc=josh@joshtriplett.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mathieu.desnoyers@efficios.com \
    --cc=paulmck@kernel.org \
    --cc=qiang.zhang1211@gmail.com \
    --cc=rcu@vger.kernel.org \
    --cc=rostedt@goodmis.org \
    --cc=stefan.wiehler@nokia.com \
    --cc=will@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.