Re: [PATCH v6 0/6] RISC-V Hardware Probing User Interface

[Conor Dooley]

Hey Evan,

On Fri, Apr 07, 2023 at 04:10:57PM -0700, Evan Green wrote:
> 
> There's been a bunch of off-list discussions about this, including at
> Plumbers.  The original plan was to do something involving providing an
> ISA string to userspace, but ISA strings just aren't sufficient for a
> stable ABI any more: in order to parse an ISA string users need the
> version of the specifications that the string is written to, the version
> of each extension (sometimes at a finer granularity than the RISC-V
> releases/versions encode), and the expected use case for the ISA string
> (ie, is it a U-mode or M-mode string).  That's a lot of complexity to
> try and keep ABI compatible and it's probably going to continue to grow,
> as even if there's no more complexity in the specifications we'll have
> to deal with the various ISA string parsing oddities that end up all
> over userspace.
> 
> Instead this patch set takes a very different approach and provides a set
> of key/value pairs that encode various bits about the system.  The big
> advantage here is that we can clearly define what these mean so we can
> ensure ABI stability, but it also allows us to encode information that's
> unlikely to ever appear in an ISA string (see the misaligned access
> performance, for example).  The resulting interface looks a lot like
> what arm64 and x86 do, and will hopefully fit well into something like
> ACPI in the future.
> 
> The actual user interface is a syscall, with a vDSO function in front of
> it. The vDSO function can answer some queries without a syscall at all,
> and falls back to the syscall for cases it doesn't have answers to.
> Currently we prepopulate it with an array of answers for all keys and
> a CPU set of "all CPUs". This can be adjusted as necessary to provide
> fast answers to the most common queries.
> 
> An example series in glibc exposing this syscall and using it in an
> ifunc selector for memcpy can be found at [1].
> 
> I was asked about the performance delta between this and something like
> sysfs. I created a small test program [2] and ran it on a Nezha D1
> Allwinner board. Doing each operation 100000 times and dividing, these
> operations take the following amount of time:
>  - open()+read()+close() of /sys/kernel/cpu_byteorder: 3.8us
>  - access("/sys/kernel/cpu_byteorder", R_OK): 1.3us
>  - riscv_hwprobe() vDSO and syscall: .0094us
>  - riscv_hwprobe() vDSO with no syscall: 0.0091us
> 
> These numbers get farther apart if we query multiple keys, as sysfs will
> scale linearly with the number of keys, where the dedicated syscall
> stays the same. To frame these numbers, I also did a tight
> fork/exec/wait loop, which I measured as 4.8ms. So doing 4
> open/read/close operations is a delta of about 0.3%, versus a single vDSO
> call is a delta of essentially zero.

Two nits w.r.t. build bot complaints...

On patch 2:
arch/riscv/include/uapi/asm/unistd.h:54:1: warning: initializer overrides prior initialization of this subobject [-Winitializer-overrides]
I think this one is kinda spurious, all of the syscalls complain like
this (and do on arm64 too IIRC). There was a patch from Guo somewhere to
disable -Winitializer-overrides in this case, I should go find out what
happened to it.

On patch 4:
arch/riscv/kernel/cpufeature.c:29:1: warning: symbol '__pcpu_scope_misaligned_access_speed' was not declared. Should it be static?

Probably because cos cpufeature.c doesn't include the header of the same
name... Perhaps Palmer could fix that one up on application?

Cheers,
Conor.

Attachment: signature.asc
Description: PGP signature