Re: Simplify load_unaligned_zeropad() (was Re: [GIT PULL] Ceph updates for 5.20-rc1)

[Linus Torvalds]

On Mon, Aug 15, 2022 at 12:17 AM Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>
> So obviously we could use _ASM_EXTABLE_TYPE_REG together with something
> like: "mov (%[reg]), %[reg]" to not depend on these fixed registers, but
> yeah, that doesn't seem needed. Code-gen is fine as is.

I looked at using TYPE_REG, but it got *much* more complicated for no
obvious gain.

The biggest downside of the fixed registers is actually the address,
which in the fs/dcache.c case doesn't matter (the address is just in a
register anyway, and the compiler can pick the register it wants to
and seems to happily pick %rdx).

But in fs/namei.c, the code *used* to be able to use register indexed
addressing, and now needs to use a 'lea' to generate the address into
a single register.

Using _ASM_EXTABLE_TYPE_REG wouldn't help that case - we'd have to
actually disassemble the instruction that faulted, and figure out the
address that way. Because while the fault itself gives us an address,
it gives us the address of the fault, which will be the first byte of
the next page, not the beginning address for the access that we want.

And no, disassembling the instruction wouldn't kill us either (we know
it's a "mov" instruction, so it's just the modrm bytes), but again it
really didn't seem worth the pain. The generated code with the fixed
registers wasn't optimal, but it was close enough that it really
doesn't seem to matter.

> > +     regs->ip = ex_fixup_addr(e);
> > +     return true;
>
> I think the convention here is to do:
>
>         return ex_handler_default(e, regs);

Ahh, yes, except for the FP case where we restore it first (because
the code seems to want to print out the restored address for some
reason).

I had just started with the ex_handler_default() implementation as the
boiler plate, which is why I did that thing by hand.

Will fix.

The other question I had was actually the "return false" above - I
decided that if the address of the fault does *not* match the expected
"round %rdx up" address, we shouldn't do any fixup at all, and treat
it as a regular kernel oops - as if the exception table hadn't been
found at all.

That seems to be a good policy, but no other exception handler does
anything like that, so maybe somebody has comments about that thing?
All the other exception handler fixup functions always return true
unconditionally, but the fixup_exception() code is clearly written to
be able to return 0 for "no fixup".

I may have written that code originally, but it's _soo_ long ago that ....

              Linus