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;
MIME-Version: 1.0
References: <20220714013131.12648-1-zhouzhouyi@gmail.com> <87tu7k6o7n.fsf@jogness.linutronix.de>
In-Reply-To: <87tu7k6o7n.fsf@jogness.linutronix.de>
From:   Zhouyi Zhou <zhouzhouyi@gmail.com>
Date:   Thu, 14 Jul 2022 18:01:41 +0800
Message-ID: <CAABZP2x-OiwX7-nRvdPz0NU1D5kgUoL4MQOgTqLK1KgLcrZ5DA@mail.gmail.com>
Subject: Re: [PATCH linux-next] powerpc: use raw_smp_processor_id in arch_touch_nmi_watchdog
To:     John Ogness <john.ogness@linutronix.de>
Cc:     Michael Ellerman <mpe@ellerman.id.au>,
        Benjamin Herrenschmidt <benh@kernel.crashing.org>,
        paulus@samba.org, Nicholas Piggin <npiggin@gmail.com>,
        ldufour@linux.ibm.com, pmladek@suse.com, Julia.Lawall@inria.fr,
        linuxppc-dev <linuxppc-dev@lists.ozlabs.org>,
        linux-kernel <linux-kernel@vger.kernel.org>, lance@osuosl.org,
        "Paul E. McKenney" <paulmck@kernel.org>, rcu <rcu@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Thank John for correcting me ;-)

On Thu, Jul 14, 2022 at 5:25 PM John Ogness <john.ogness@linutronix.de> wrote:
>
> On 2022-07-14, Zhouyi Zhou <zhouzhouyi@gmail.com> wrote:
> > use raw_smp_processor_id() in arch_touch_nmi_watchdog
> > because when called from watchdog, the cpu is preemptible.
>
> I would expect the correct solution is to make it a non-migration
> section. Something like the below (untested) patch.
I applied your patch (I have made a tiny modification by removing the
return statement after "goto out;") and
passed the test in the ppc VM of Open Source Lab of Oregon State University.

Tested-by: Zhouyi Zhou <zhouzhouyi@gmail.com>

Many Thanks
Kindly Regards
Zhouyi
>
> John Ogness
>
> diff --git a/arch/powerpc/kernel/watchdog.c b/arch/powerpc/kernel/watchdog.c
> index bfc27496fe7e..9d34aa809241 100644
> --- a/arch/powerpc/kernel/watchdog.c
> +++ b/arch/powerpc/kernel/watchdog.c
> @@ -450,17 +450,23 @@ static enum hrtimer_restart watchdog_timer_fn(struct hrtimer *hrtimer)
>  void arch_touch_nmi_watchdog(void)
>  {
>         unsigned long ticks = tb_ticks_per_usec * wd_timer_period_ms * 1000;
> -       int cpu = smp_processor_id();
> +       int cpu;
>         u64 tb;
>
> -       if (!cpumask_test_cpu(cpu, &watchdog_cpumask))
> +       cpu = get_cpu();
> +
> +       if (!cpumask_test_cpu(cpu, &watchdog_cpumask)) {
> +               goto out;
>                 return;
I think we should remove the return statement here.
> +       }
>
>         tb = get_tb();
>         if (tb - per_cpu(wd_timer_tb, cpu) >= ticks) {
>                 per_cpu(wd_timer_tb, cpu) = tb;
>                 wd_smp_clear_cpu_pending(cpu);
>         }
> +out:
> +       put_cpu();
>  }
>  EXPORT_SYMBOL(arch_touch_nmi_watchdog);