Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Valentin Schneider <vschneid@redhat.com>
To:     Petr Mladek <pmladek@suse.com>
Cc:     Tao Zhou <tao.zhou@linux.dev>, linux-kernel@vger.kernel.org,
        kexec@lists.infradead.org, linux-rt-users@vger.kernel.org,
        Eric Biederman <ebiederm@xmission.com>,
        Arnd Bergmann <arnd@arndb.de>,
        Thomas Gleixner <tglx@linutronix.de>,
        Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
        Juri Lelli <jlelli@redhat.com>,
        "Luis Claudio R. Goncalves" <lgoncalv@redhat.com>
Subject: Re: [PATCH] panic, kexec: Don't mutex_trylock() in __crash_kexec()
In-Reply-To: <YqyHGxPCgiXuep3/@alley>
References: <20220616123709.347053-1-vschneid@redhat.com>
 <Yqxaf6V+hvCSXQSM@geo.homenetwork>
 <xhsmhh74j7biy.mognet@vschneid.remote.csb> <YqyHGxPCgiXuep3/@alley>
Date:   Fri, 17 Jun 2022 15:46:23 +0100
Message-ID: <xhsmhczf773gg.mognet@vschneid.remote.csb>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On 17/06/22 15:52, Petr Mladek wrote:
> On Fri 2022-06-17 12:52:05, Valentin Schneider wrote:
>> If you look at __crash_kexec() in isolation yes, but if you look at panic()
>> and nmi_panic() only a single NMI can get in there. I think that is also
>> true for invocations via crash_kexec().
>
> It is true that panic() could be called only once, see this code
> in panic():
>
> 	 * Only one CPU is allowed to execute the panic code from here. For
> 	this_cpu = raw_smp_processor_id();
> 	old_cpu  = atomic_cmpxchg(&panic_cpu, PANIC_CPU_INVALID, this_cpu);
>
> 	if (old_cpu != PANIC_CPU_INVALID && old_cpu != this_cpu)
> 		panic_smp_self_stop();
>
>
> One the other hand, the aproach with two variables is strage
> and brings exactly these questions.
>
> If a trylock is enough that the mutex can be replaced by two
> simple atomic operations. The mutex would be needed only
> when a user really would need to wait for another one.
>
>
> 	atomic_t crash_kexec_lock;
>
> 	/* trylock part */
> 	if (atomic_cmpxchg_acquire(&crash_kexec_lock, 0, 1) != 0)
> 		return -EBUSY;
>
> 	/* do anything guarded by crash_kexec_lock */
>
> 	/* release lock */
> 	atomic_set_release(&crash_kexec_lock, 0);
>
> The _acquire, _release variants will do the barriers correctly.
>

Looks saner! I can't think of anything wrong with it so I'll shove that in
v2. Thanks!

> Best Regards,
> Petr