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;
Date:   Fri, 21 Apr 2023 14:36:52 +0200
From:   Frederic Weisbecker <frederic@kernel.org>
To:     Huacai Chen <chenhuacai@kernel.org>,
        WANG Xuerui <kernel@xen0n.name>
Cc:     Peter Zijlstra <peterz@infradead.org>,
        Thomas Gleixner <tglx@linutronix.de>,
        "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>,
        Anna-Maria Behnsen <anna-maria@linutronix.de>,
        LKML <linux-kernel@vger.kernel.org>
Subject: Loongson (and other $ARCHs?) idle VS timer enqueue
Message-ID: <ZEKDZEQmKExv0O7Q@lothringen>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Precedence: bulk

Hi,

I'm looking at the __arch_cpu_idle() implementation in Loongarch
and I'm wondering about the rollback code. I don't understand well
that code but with the help from PeterZ I might have seen something,
so tell me if I'm wrong: when an interrupt happens within
__arch_cpu_idle(), handle_vint() rolls back the return value to the
beginning of __arch_cpu_idle(), so that TIF_NEED_RESCHED is checked
again. Is that correct?

Because if an interrupt fires while in __arch_cpu_idle(), that IRQ
might enqueue a new timer and that new timer needs to be reprogrammed
from the main idle loop and just checking TIF_NEED_RESCHED doesn't
tell about that information.

More generally IRQs must _not_ be re-enabled between cpuidle_select()
(or just tick_nohz_idle_stop_tick() if no cpuidle support) and the
last halting ASM instruction. If that happens there must be
a mechanism to cope with that and make sure we return to the main
idle loop.

If that mechanism has to go through rollback (I wish your arch allows
you to find a simpler and less error prone mechanism through), then the
rollback must actually fast forward to after the halting instruction
so that the main idle loop re-checks the timers. But then
__arch_cpu_idle() alone is not enough to be part of the fastforward
section, it has to start before the raw_local_irq_enable() in
arch_cpu_idle().

Another way to cope with this would be to have:

#define TIF_IDLE_TIMER	 ...
#define TIF_IDLE_EXIT	 (TIF_NEED_RESCHED | TIF_IDLE_TIMER)

And set that from the timer enqueue in idle time and check TIF_IDLE_EXIT
on idle callback. It depends how many architectures are concerned by this.
All I know so far is:

* mwait based mechanism should be fine if called with IRQs disabled (or
sti is called right before) but then we must be sure that IRQs have never
been even temporarily re-enabled between cpuidle_select() and mwait. How
to detect that kind of mistake?

* wfi based mechanism look fine, but again we must make sure IRQs have
never been re-enabled.

* sti;hlt should be fine but again...

* Need to check all other archs

I'm trying to find an automated way to debug this kind of issue but it's not
easy...

Thanks.