Re: [PATCH v6 5/5] riscv: Add and use alignment-ignorant memcpy

[Evan Green <evan@rivosinc.com>]

On Thu, Aug 3, 2023 at 10:50 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 8/3/23 00:25, Florian Weimer via Libc-alpha wrote:
> > * Evan Green:
> >
> >> +static inline __typeof (__redirect_memcpy) *
> >> +select_memcpy_ifunc (uint64_t dl_hwcap, __riscv_hwprobe_t hwprobe_func)
> >> +{
> >> +  INIT_ARCH ();
> >> +
> >> +  struct riscv_hwprobe pair;
> >> +
> >> +  pair.key = RISCV_HWPROBE_KEY_CPUPERF_0;
> >> +  if (!hwprobe_func || hwprobe_func(&pair, 1, 0, NULL, 0) != 0)
> >> +    return __memcpy_generic;
> >> +
> >> +  if ((pair.key > 0) &&
> >> +      (pair.value & RISCV_HWPROBE_MISALIGNED_MASK) ==
> >> +       RISCV_HWPROBE_MISALIGNED_FAST)
> >> +    return __memcpy_noalignment;
> >> +
> >> +  return __memcpy_generic;
> >> +}
> >
> > In libc, you could call __riscv_hwprobe directly, so the additional
> > argument isn't needed after all.

So you think I should drop the libc-symbols.h change and call
__riscv_hwprobe directly here? Sure, I can do that.

>
> Outside libc something is required.
>
> An extra parameter to ifunc is surprising though, and clearly not ideal per the extra
> hoops above.  I would hope for something with hidden visibility in libc_nonshared.a that
> could always be called directly.

My previous spin took that approach, defining a
__riscv_hwprobe_early() in libc_nonshared that could route to the real
function if available, or make the syscall directly if not. But that
approach had the drawback that ifunc users couldn't take advantage of
the vDSO, and then all users had to comprehend the difference between
__riscv_hwprobe() and __riscv_hwprobe_early().

In contrast, IMO this approach is much nicer. Ifunc writers are
already used to getting hwcap info via a parameter. Adding this second
parameter, which also provides hwcap-like things, seems like a natural
extension. I didn't quite follow what you meant by the "extra hoops
above". If you meant the previous patch to libc-symbols.h, that's all
glibc-internal-isms, and shouldn't affect external callers. As per
Florian's comment above I can drop that patch for now since I don't
strictly need it.

-Evan