Re: [patch 02/14] tmpfs: fix regressions from wider use of ZERO_PAGE

[Borislav Petkov <bp@alien8.de>]

On Tue, Apr 26, 2022 at 06:29:16PM -0700, Linus Torvalds wrote:
> Yeah, yeah, you could also use that _ASM_EXTABLE_TYPE_REG thing for
> the second exception point, and keep %rcx as zero, and keep it in
> %eax, and depend on that whole "%rcx = 8*%rcx+%rax" fixing it up, and
> knowing that if an exception does *not* happen, %rcx will be zero from
> the word-size loop.

Exactly!

> But that really seems much too subtle for me - why not just keep
> things in %rcx, and try to make this look as much as possible like the
> "rep stosq + rep stosb" case?
> 
> And finally: I still think that those fancy exception table things are
> *much* too fancy, and much too subtle, and much too complicated.

This whole confusion goes to show that this really is too subtle. ;-\

I mean, it is probably fine for some simple asm where you want to have
the register fixup out of the way. But for something like that where
I wanted the asm to be optimal but not tricky at the same time, that
tricky "hidden fixup" might turn out to be not such a good idea.

And sure, yeah, we can make this work this way now and all but after
time passes, swapping all that tricky stuff back in is going to be a
pain. That's why I'm leaving breadcrumbs everywhere I can.

> So I'd actually prefer to get rid of them entirely, and make the code
> use the "no register changes" exception, and make the exception
> handler do a proper site-specific fixup. At that point, you can get
> rid of all the "mask bits early" logic, get rid of all the extraneous
> 'test' instructions, and make it all look something like below.

Yap, and the named labels make it even clearer.

> NOTE! I've intentionally kept the %eax thing using 32-bit instructions
> - smaller encoding, and only the low three bits matter, so why
> move/mask full 64 bits?

Some notes below.

> NOTE2! Entirely untested.

I'll poke at it.

> But I tried to make the normal code do minimal work, and then fix
> things up in the exception case more.

Yap.

> So it just keeps the original count in the 32 bits in %eax until it
> wants to test it, and then uses the 'andl' to both mask and test. And
> the exception case knows that, so it masks there too.

Sure.

> I dunno. But I really think that whole new _ASM_EXTABLE_TYPE_REG and
> EX_TYPE_UCOPY_LEN8 was unnecessary.

I've pointed this out to Mr. Z. He likes them as complex as possible.
:-P

> SYM_FUNC_START(clear_user_original)
>         movl %ecx,%eax

I'll add a comment here along the lines of us only needing the 32-bit
quantity of the size as we're dealing with the rest bytes and so we save
a REX byte in the opcode.

>         shrq $3,%rcx             # qwords

Any particular reason for the explicit "q" suffix? The register already
denotes the opsize and the generated opcode is the same.

>         jz .Lrest_bytes
> 
>         # do the qwords first
>         .p2align 4
> .Lqwords:
>         movq $0,(%rdi)
>         lea 8(%rdi),%rdi
>         dec %rcx

Like here, for example.

I guess you wanna be explicit as to the operation width...

>         jnz .Lqwords
> 
> .Lrest_bytes:
>         andl $7,%eax             # rest bytes
>         jz .Lexit
> 
>         # now do the rest bytes
> .Lbytes:
>         movb $0,(%rdi)
>         inc %rdi
>         decl %eax
>         jnz .Lbytes
> 
> .Lexit:
>         xorl %eax,%eax
>         RET
> 
> .Lqwords_exception:
>         # convert qwords back into bytes to return to caller
>         shlq $3, %rcx
>         andl $7, %eax
>         addq %rax,%rcx
>         jmp .Lexit
> 
> .Lbytes_exception:
>         movl %eax,%ecx
>         jmp .Lexit
> 
>         _ASM_EXTABLE_UA(.Lqwords, .Lqwords_exception)
>         _ASM_EXTABLE_UA(.Lbytes, .Lbytes_exception)

Yap, I definitely like this one more. Straightforward.

Thx!

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette