Re: [PATCH v12 0/7] RISC-V: ifunced memcpy using new kernel hwprobe interface

[Palmer Dabbelt <palmer@rivosinc.com>]

On Thu, 15 Feb 2024 10:45:10 PST (-0800), enh@google.com wrote:
> On Thu, Feb 15, 2024 at 9:22 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>>
>> On Thu, 15 Feb 2024 09:00:03 PST (-0800), enh@google.com wrote:
>> > On Thu, Feb 15, 2024 at 8:50 AM Palmer Dabbelt <palmer@rivosinc.com> wrote:
>> >>
>> >> On Thu, 15 Feb 2024 07:48:03 PST (-0800), Evan Green wrote:
>> >> > On Wed, Feb 14, 2024 at 10:16 AM Adhemerval Zanella Netto
>> >> > <adhemerval.zanella@linaro.org> wrote:
>> >> >>
>> >> >>
>> >> >>
>> >> >> On 14/02/24 11:31, Evan Green wrote:
>> >> >> >
>> >> >> > This series illustrates the use of a recently accepted Linux syscall that
>> >> >> > enumerates architectural information about the RISC-V cores the system
>> >> >> > is running on. In this series we expose a small wrapper function around
>> >> >> > the syscall. An ifunc selector for memcpy queries it to see if unaligned
>> >> >> > access is "fast" on this hardware. If it is, it selects a newly provided
>> >> >> > implementation of memcpy that doesn't work hard at aligning the src and
>> >> >> > destination buffers.
>> >> >> >
>> >> >> > For applications and libraries outside of glibc that want to use
>> >> >> > __riscv_hwprobe() in ifunc selectors, this series also sends a pointer
>> >> >> > to the riscv_hwprobe() function in as the second argument to ifunc
>> >> >> > selectors. A new inline convenience function can help application and
>> >> >> > library callers to check for validity and quickly probe a single key.
>> >> >>
>> >> >> I still think we should address Jessica Clarke remarks for the ifunc ABI [1].
>> >> >> I recall that Florian has tried to address the ifunc ordering and that
>> >> >> Jessica proposed solutions was not fully sufficient to address all the
>> >> >> ifunc corner cases.
>> >> >>
>> >> >> [1] https://sourceware.org/pipermail/libc-alpha/2024-January/154082.html
>> >> >
>> >> > I haven't invested the time yet in studying the resolver to understand
>> >> > how feasible Jessica's suggestion is. I was sort of hoping Florian
>> >> > would chime in with an "oh yeah let's do that" or "no, it doesn't
>> >> > work". I suppose I still am :)
>> >>
>> >> The discussion's over here:
>> >> https://inbox.sourceware.org/libc-alpha/20231017044641.pw2ccr6exvhtmhkk@google.com/
>> >>
>> >> I was inclined to just ignore it: Florian explained what's going on with
>> >> the multiple libraries constraint, so we're kind of just going around in
>> >> circles at that point.
>> >>
>> >> I'm also not sure the argument makes a whole lot of sense in the first
>> >> place.  The core of the argument seems to be around binary compatibility
>> >> with other kernels/libcs, but there's a ton of reasons why binaries
>> >> aren't compatible between those systems.  So if glibc just has a more
>> >> complex IFUNC resolving scheme and that ends up requiring the extra
>> >> agrument, then users of glibc have to go deal with that -- other
>> >> systems/libcs might make different decisions, but that's just how these
>> >> things go.
>> >
>> > yeah, i struggle with the portability premise too. not least because
>> > macOS/iOS (which is obviously the most common "other platform" for me
>> > with my Android hat on) doesn't have ifuncs at all. that's quite a big
>> > blocker to any dream of portability.
>> >
>> > as i've said before, in a survey of all the open source libraries that
>> > go into Android, there are none that don't support macOS/iOS, and so
>> > there are none that actually rely on ifuncs. ifunc usage is limited to
>> > libc (which is why we libc maintainers get bikeshedded by stuff like
>> > this) and compiler-generated FMV stuff. i'd argue that "real people"
>> > (app developers) should probably be looking at the latter anyway, and
>> > it's our job to make that work (which happens once^Wtwice, in llvm and
>> > gcc).
>> >
>> > also, the fact that Android is doing what's proposed here (with the
>> > extra argument) means there'd be _some_ incompatibility even if glibc
>> > and FreeBSD didn't do that.
>>
>> So you're already committed to that ABI?
>
> "pretty much" given glibc release cycles and Android release cycles...

Ya, cool -- I was just curious, I figured it'd be something along those 
lines (we also talked a bit off list).

> the current API addresses a real problem with current dynamic linkers,
> and -- though it would have been great to make a different decision 16
> years ago -- between the already stated reasons and the potential for
> app compat issues (and a distaste for having the riscv64 linker
> behavior differ from the already-shipped architectures), i think we'll
> want something like that...

Ya, I'm generally in favor of making RISC-V as unsurprising as possible.  
Given how complex the IFUNC stuff is it seems like a recipe for disaster 
to have different rules around linker ordering or cross-object symbol 
resoltion for RISC-V.  Maybe folks want to change that for all the 
ports, but that seems like a fairly complex project and I'm worried 
we'll miss another release waiting on it.

As far as I can tell the downside here is just that we have an extra 
argument pointing to a hwprobe function, that doesn't seem so painful to 
just support forever.  It's not like this possible change to the 
resolution rules would make calling via the pre-resolved function 
argument invalid, it'd just be unnecessary.

Thus I'm generally in favor of merging this, so

Acked-by: Palmer Dabbelt <palmer@rivosinc.com>

though I'm cool holding off a bit on merging it, in case anyone thinks 
that's a bad idea.

>> > (Android's only source incompatibility with glibc at the moment is the
>> > lack of the single-probe helper inline, which i fear will encourage
>> > _worse_ code, but suspect will actually be unused in practice anyway
>> > for the same "ifuncs are 'assembler' for libc/toolchain, and
>> > library/app developers doing stuff themselves will continue to use
>> > regular function pointers like they do today" reasons.)
>> >
>> >> Looks like Maskray is proposing making glibc's IFUNC resolving.  That's
>> >> a generic glibc decision and I don't really understand this stuff well
>> >> enough to have much of an opinion -- sure maybe the multi-library IFUNC
>> >> resolving is overkill, but aside from doing some similar grep of all the
>> >> sources (or trying some builds and such) I'm not sure how to tell if
>> >> we're safe breaking the ABI there.
>> >>
>> >> That said, we're not comitting to ABI stability until the release.  So
>> >> if we decide to make a generic glibc change we can always go drop the
>> >> argument, that should be easy.  Maybe we even just throw out a RFC patch
>> >> to do it, unless I'm missing something the code is the easy part here
>> >> (we're essentially just going back to one of the early versions, from
>> >> before we knew about this resolving order complexity).
>> >>
>> >> > Alternatively, patches 1-3 of this series stand on their own. If the
>> >> > ifunc aspect of this is gated on me doing a bunch of research, it
>> >> > might at least make sense to land the first half now, to get Linux
>> >> > users easy access to the __riscv_hwprobe() syscall and vDSO.
>> >>
>> >> I'm OK with that, too.
>> >>
>> >> >
>> >> > -Evan