Re: [PATCH v4 1/2] rust: add static_key_false

[Alice Ryhl]

On Wed, Jul 31, 2024 at 7:05 PM Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
>
> On Fri, Jun 28, 2024 at 01:23:31PM +0000, Alice Ryhl wrote:
>
> >  rust/kernel/arch/arm64/jump_label.rs     | 34 ++++++++++++++++++++++++++++
> >  rust/kernel/arch/loongarch/jump_label.rs | 35 +++++++++++++++++++++++++++++
> >  rust/kernel/arch/mod.rs                  | 24 ++++++++++++++++++++
> >  rust/kernel/arch/riscv/jump_label.rs     | 38 ++++++++++++++++++++++++++++++++
> >  rust/kernel/arch/x86/jump_label.rs       | 35 +++++++++++++++++++++++++++++
> >  rust/kernel/lib.rs                       |  2 ++
> >  rust/kernel/static_key.rs                | 32 +++++++++++++++++++++++++++
> >  scripts/Makefile.build                   |  2 +-
> >  8 files changed, 201 insertions(+), 1 deletion(-)
>
> So I really find the amount of duplicated asm offensive. Is is far too
> easy for any of this to get out of sync.
>
> > diff --git a/rust/kernel/arch/x86/jump_label.rs b/rust/kernel/arch/x86/jump_label.rs
> > new file mode 100644
> > index 000000000000..383bed273c50
> > --- /dev/null
> > +++ b/rust/kernel/arch/x86/jump_label.rs
> > @@ -0,0 +1,35 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +// Copyright (C) 2024 Google LLC.
> > +
> > +//! X86 Rust implementation of jump_label.h
> > +
> > +/// x86 implementation of arch_static_branch
> > +#[macro_export]
> > +#[cfg(target_arch = "x86_64")]
> > +macro_rules! arch_static_branch {
> > +    ($key:path, $keytyp:ty, $field:ident, $branch:expr) => {'my_label: {
> > +        core::arch::asm!(
> > +            r#"
> > +            1: .byte 0x0f,0x1f,0x44,0x00,0x00
> > +
> > +            .pushsection __jump_table,  "aw"
> > +            .balign 8
> > +            .long 1b - .
> > +            .long {0} - .
> > +            .quad {1} + {2} + {3} - .
> > +            .popsection
> > +            "#,
> > +            label {
> > +                break 'my_label true;
> > +            },
> > +            sym $key,
> > +            const ::core::mem::offset_of!($keytyp, $field),
> > +            const $crate::arch::bool_to_int($branch),
> > +        );
> > +
> > +        break 'my_label false;
> > +    }};
> > +}
>
> Note that this uses the forced 5 byte version, and not the dynamic sized
> one. On top of that it hard-codes the nop5 string :/
>
> Please work harder to not have to duplicate stuff like this.

I really didn't want to duplicate it, but it's very hard to find a
performant alternative. Is there any way we could accept duplication
only in the cases where an 'i' parameter is used? I don't have the
choice of using a Rust helper for 'i' parameters.

Perhaps one option could be to put the Rust code inside jump_label.h
and have the header file evaluate to either C or Rust depending on the
value of some #ifdefs?

#ifndef RUST_ASM
/* existing C code goes here */
#endif
#ifdef RUST_ASM
// rust code goes here
#endif

That way the duplication is all in a single file. It would also avoid
the need for duplicating the nop5 string, as the Rust case is still
going through the C preprocessor and can use the existing #define.

I'm also open to other alternatives. But I don't have infinite
resources to drive major language changes.

Alice