Re: [PATCH] x86: add 'runtime constant' infrastructure

[Linus Torvalds]

On Mon, 10 Jun 2024 at 03:43, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>
> --- linux-2.6.orig/arch/Kconfig
> +++ linux-2.6/arch/Kconfig
> @@ -1492,6 +1492,9 @@ config HAVE_SPARSE_SYSCALL_NR
>  config ARCH_HAS_VDSO_DATA
>         bool
>
> +config HAVE_RUNTIME_CONST
> +       bool

No. We're not adding a stupid config variable, when nothing actually wants it.

> +#define __runtime_const(sym, op, type)                 \
> +({                                                     \
> +       typeof(sym) __ret;                              \
> +       asm(op " %1, %0\n1:\n"                          \
> +           ".pushsection __runtime_const, \"aw\"\n\t"  \
> +           ".long %c3 - .      # sym \n\t"             \
> +           ".long %c2          # size \n\t"            \
> +           ".long 1b - %c2 - . # addr \n\t"            \
> +           ".popsection\n\t"                           \
> +           : "=r" (__ret)                              \
> +           : "i" ((type)0x0123456789abcdefull),        \
> +             "i" (sizeof(type)),                       \
> +             "i" ((void *)&sym));                      \
> +       __ret;                                          \
> +})
> +
> +#define runtime_const(sym)                                             \
> +({                                                                     \
> +       typeof(sym) __ret;                                              \
> +       switch(sizeof(sym)) {                                           \
> +       case 1: __ret = __runtime_const(sym, "movb", u8); break;        \
> +       case 2: __ret = __runtime_const(sym, "movs", u16); break;       \
> +       case 4: __ret = __runtime_const(sym, "movl", u32); break;       \
> +       case 8: __ret = __runtime_const(sym, "movq", u64); break;       \
> +       default: BUG();                                                 \
> +       }                                                               \
> +       __ret;                                                          \
> +})

And no. We're not adding magic "generic" helpers that have zero use
and just make the code harder to read, and don't even work on 32-bit
x86 anyway.

Because I didn't test, but I am pretty sure that clang will not
compile the above on x86-32, because clang verifies the inline asm,
and "movq" isn't a valid instruction.

We had exactly that for the uaccess macros, and needed to special-case
the 64-bit case for that reason.

And we don't *need* to. All of the above is garbage waiting for a use
that shouldn't exist.

> +++ linux-2.6/kernel/runtime_const.c
> @@ -0,0 +1,119 @@

And here you basically tripled the size of the patch in order to have
just one section, when I had per-symbol sections.

So no.

I envision a *couple* of runtime constants. The absolutely only reason
to use a runtime constant is that it is *so* hot that the difference
between "load a variable" and "write code with a constant" is
noticeable.

I can point to exactly one such case in the kernel right now.

I'm sure there are others, but I'd expect that "others" to be mainly a handful.

This needs to be simple, obvious, and literally designed for very targeted use.

This is a *very* special case for a *very* special code. Not for generic use.

I do not ever expect to see this used by modules, for example. There
is no way in hell I will expose the instruction rewriting to a module.
The *use* of a constant is a _maybe_, but it's questionable too. By
definition, module code cannot be all that core.

             Linus