Hi, Arnd and Peter,
On Tue, Jul 27, 2021 at 11:08 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
> On Tue, Jul 27, 2021 at 4:10 PM Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> > On Wed, Jul 07, 2021 at 11:56:37PM +1000, Nicholas Piggin wrote:
> > > >> +/*
> > > >> + * Common Vectored Interrupt
> > > >> + * Complete the register saves and invoke the do_vi() handler
> > > >> + */
> > > >> +SYM_FUNC_START(except_vec_vi_handler)
> > > >> + la t1, __arch_cpu_idle
> > > >> + LONG_L t0, sp, PT_EPC
> > > >> + /* 32 byte rollback region */
> > > >> + ori t0, t0, 0x1f
> > > >> + xori t0, t0, 0x1f
> > > >> + bne t0, t1, 1f
> > > >> + LONG_S t0, sp, PT_EPC
> > > >
> > > > Seriously, you're having your interrupt handler recover from the idle
> > > > race? On a *new* architecture?
> > >
> > > It's heavily derived from MIPS (does that make the wholesale replacement
> > > of arch/mips copyright headers a bit questionable?).
> > >
> > > I don't think it's such a bad trick though -- restartable sequences
> > > before they were cool. It can let you save an irq disable in some
> > > cases (depending on the arch of course).
> >
> > In this case you're making *every* interrupt slower. Simply adding a new
> > idle instruction, one that can be called with interrupts disabled and
> > will terminate on a pending interrupt, would've solved the issues much
> > nicer and reclaimed the cycles spend on this restart trick.
>
> Are we actually sure that loongarch /needs/ this version?
>
> The code was clearly copied from the mips default r4k_wait()
> function, but mips also has this one:
>
> /*
> * This variant is preferable as it allows testing need_resched and going to
> * sleep depending on the outcome atomically. Unfortunately the "It is
> * implementation-dependent whether the pipeline restarts when a non-enabled
> * interrupt is requested" restriction in the MIPS32/MIPS64 architecture makes
> * using this version a gamble.
> */
> void __cpuidle r4k_wait_irqoff(void)
> {
> if (!need_resched())
> __asm__(
> " .set push \n"
> " .set arch=r4000 \n"
> " wait \n"
> " .set pop \n");
> raw_local_irq_enable();
> }
>
> case CPU_LOONGSON64:
> if ((c->processor_id & (PRID_IMP_MASK | PRID_REV_MASK)) >=
> (PRID_IMP_LOONGSON_64C |
> PRID_REV_LOONGSON3A_R2_0) ||
> (c->processor_id & PRID_IMP_MASK) ==
> PRID_IMP_LOONGSON_64R)
> cpu_wait = r4k_wait;
> ...
> cpu_wait = r4k_wait;
> if (read_c0_config7() & MIPS_CONF7_WII)
> cpu_wait = r4k_wait_irqoff;
> if (cpu_has_mips_r6) {
> cpu_wait = r4k_wait_irqoff;
>
> So a lot of the newer mips variants already get the fixed
> version. Maybe the hardware developers fixed it without telling
> the kernel people about it?
Very very unfortunately, the idle instruction of LoongArch cannot
executed with irq disabled. In other word, LoongArch should use a
variant like r4k_wait(), not r4k_wait_irqoff().
Huacai
>
> Arnd
