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?
Arnd
