On Wed, Jan 22, 2025 at 4:33 PM Dave Hansen <dave.hansen@xxxxxxxxx> wrote:
>
> On 1/22/25 16:06, Kevin Loughlin wrote:
> >> BTW, I don't think you should be compelled to use alternative() as
> >> opposed to a good old:
> >>
> >> if (cpu_feature_enabled(X86_FEATURE_WBNOINVD))
> >> ...
> > Agreed, though I'm leaving as alternative() for now (both because it
> > results in fewer checks and because that's what is used in the rest of
> > the file); please holler if you prefer otherwise. If so, my slight
> > preference in that case would be to update the whole file
> > stylistically in a separate commit.
>
> alternative() can make a _lot_ of sense. It's extremely compact in the
> code it generates. It messes with compiler optimization, of course, just
> like any assembly. But, overall, it's great.
>
> In this case, though, we don't care one bit about code generation or
> performance. We're running the world's slowest instruction from an IPI.
>
> As for consistency, special_insns.h is gloriously inconsistent. But only
> two instructions use alternatives, and they *need* the asm syntax
> because they're passing registers and meaningful constraints in.
>
> The wbinvds don't get passed registers and their constraints are
> trivial. This conditional:
>
> alternative_io(".byte 0x3e; clflush %0",
> ".byte 0x66; clflush %0",
> X86_FEATURE_CLFLUSHOPT,
> "+m" (*(volatile char __force *)__p));
>
> could be written like this:
>
> if (cpu_feature_enabled(X86_FEATURE_CLFLUSHOPT))
> asm volatile(".byte 0x3e; clflush %0",
> "+m" (*(volatile char __force *)__p));
> else
> asm volatile(".byte 0x66; clflush %0",
> "+m" (*(volatile char __force *)__p));
>
> But that's _actively_ ugly. alternative() syntax there makes sense.
> Here, it's not ugly at all:
>
> if (cpu_feature_enabled(X86_FEATURE_WBNOINVD))
> asm volatile(".byte 0xf3,0x0f,0x09\n\t": : :"memory");
> else
> wbinvd();
>
> and it's actually more readable with alternative() syntax.
>
> So, please just do what makes the code look most readable. Performance
> and consistency aren't important. I see absolutely nothing wrong with:
>
> static __always_inline void raw_wbnoinvd(void)
> {
> asm volatile(".byte 0xf3,0x0f,0x09\n\t": : :"memory");
> }
>
> void wbnoinvd(void)
> {
> if (cpu_feature_enabled(X86_FEATURE_WBNOINVD))
> raw_wbnoinvd();
> else
> wbinvd();
> }
>
> ... except the fact that cpu_feature_enabled() kinda sucks and needs
> some work, but that's a whole other can of worms we can leave closed today.
Thanks for the detailed explanation; you've convinced me. v6 coming up
shortly (using native_wbnoinvd() instead of raw_wbnoinvd(), as you
named the proposed wrapper in your reply to v5).
