Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 27 Oct 2021 18:51:16 +1000
From:   Nicholas Piggin <npiggin@gmail.com>
Subject: Re: [PATCH 1/2] powerpc/watchdog: prevent printk and send IPI while
 holding the wd lock
To:     benh@kernel.crashing.org, Laurent Dufour <ldufour@linux.ibm.com>,
        mpe@ellerman.id.au, paulus@samba.org
Cc:     linux-kernel@vger.kernel.org, linuxppc-dev@lists.ozlabs.org
References: <20211026162740.16283-1-ldufour@linux.ibm.com>
        <20211026162740.16283-2-ldufour@linux.ibm.com>
        <1635303699.wgz87uxy4c.astroid@bobo.none>
        <33e15005-d342-5270-9b9d-64750f8794a7@linux.ibm.com>
In-Reply-To: <33e15005-d342-5270-9b9d-64750f8794a7@linux.ibm.com>
MIME-Version: 1.0
Message-Id: <1635324001.1tf9yz448t.astroid@bobo.none>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Excerpts from Laurent Dufour's message of October 27, 2021 6:14 pm:
> Le 27/10/2021 =C3=A0 05:29, Nicholas Piggin a =C3=A9crit=C2=A0:
>> Excerpts from Laurent Dufour's message of October 27, 2021 2:27 am:
>>> When handling the Watchdog interrupt, long processing should not be don=
e
>>> while holding the __wd_smp_lock. This prevents the other CPUs to grab i=
t
>>> and to process Watchdog timer interrupts. Furhtermore, this could lead =
to
>>> the following situation:
>>>
>>> CPU x detect lockup on CPU y and grab the __wd_smp_lock
>>>        in watchdog_smp_panic()
>>> CPU y caught the watchdog interrupt and try to grab the __wd_smp_lock
>>>        in soft_nmi_interrupt()
>>> CPU x wait for CPU y to catch the IPI for 1s in __smp_send_nmi_ipi()
>>=20
>> CPU y should get the IPI here if it's a NMI IPI (which will be true for
>>> =3D POWER9 64s).
>>=20
>> That said, not all platforms support it and the console lock problem
>> seems real, so okay.
>>=20
>>> CPU x will timeout and so has spent 1s waiting while holding the
>>>        __wd_smp_lock.
>>>
>>> A deadlock may also happen between the __wd_smp_lock and the console_ow=
ner
>>> 'lock' this way:
>>> CPU x grab the console_owner
>>> CPU y grab the __wd_smp_lock
>>> CPU x catch the watchdog timer interrupt and needs to grab __wd_smp_loc=
k
>>> CPU y wants to print something and wait for console_owner
>>> -> deadlock
>>>
>>> Doing all the long processing without holding the _wd_smp_lock prevents
>>> these situations.
>>=20
>> The intention was to avoid logs getting garbled e.g., if multiple
>> different CPUs fire at once.
>>=20
>> I wonder if instead we could deal with that by protecting the IPI
>> sending and printing stuff with a trylock, and if you don't get the
>> trylock then just return, and you'll come back with the next timer
>> interrupt.
>=20
> That sounds a bit risky to me, especially on large system when system goe=
s=20
> wrong, all the CPU may try lock here.

That should be okay though, one will get through and the others will=20
skip.

> Furthermore, now operation done under the lock protection are quite fast,=
 there=20
> is no more spinning like the delay loop done when sending an IPI.
>=20
> Protecting the IPI sending is a nightmare, if the target CPU is later pla=
y with=20
> the lock we are taking during the IPI processing, furthermore, if the tar=
get CPU=20
> is not responding the sending CPU is waiting for 1s, which slows all the =
system=20
> due to the lock held.
> Since I do a copy of the pending CPU mask and clear it under the lock=20
> protection, the IPI sending is safe while done without holding the lock.

Protecting IPI sending basically has all the same issues in the NMI
IPI layer.

>=20
> Regarding the interleaved traces, I don't think this has to be managed do=
wn=20
> here, but rather in the printk/console path.

It can't necessarily be because some of the problem is actually that a=20
NMI handler can be interrupted by another NMI IPI because the caller
can return only after handlers start running rather than complete.

I don't think it would be an additional nightmare to trylock.

Thanks,
Nick