jobserver-rs icon indicating copy to clipboard operation
jobserver-rs copied to clipboard

preadv2 not found during linking

Open NobodyXu opened this issue 4 months ago • 18 comments

Ref: rust-lang/cc-rs#1039

On older glibc version, the preadv2 symbol isn't available.

I think it might makes sense to use syscall directly to avoid this link-time issue.

NobodyXu avatar Apr 20 '24 06:04 NobodyXu

Switching to using syscall would also enable it to be used on musl.

NobodyXu avatar Apr 20 '24 10:04 NobodyXu

From preadv64v2.c and preadv2 from glibc:

ssize_t result = SYSCALL_CANCEL(preadv2, fd, vector, count,
				   LO_HI_LONG(offset), flags);

NobodyXu avatar Apr 20 '24 10:04 NobodyXu

For LO_HI_LONG:

// x86_64
#define LO_HI_LONG(val) (val), 0

// x32
#define LO_HI_LONG(val) (val)

// Other
/* Provide a macro to pass the off{64}_t argument on p{readv,writev}{64}.  */
#define LO_HI_LONG(val) \
 (long) (val), \
 (long) (((uint64_t) (val)) >> 32)

I scrapped it from https://elixir.bootlin.com/glibc/glibc-2.39/source/sysdeps/unix/sysv/linux/sysdep.h#L94 , https://elixir.bootlin.com/glibc/glibc-2.39/source/sysdeps/unix/sysv/linux/x86_64/sysdep.h#L386 , and https://elixir.bootlin.com/glibc/glibc-2.39/source/sysdeps/unix/sysv/linux/x86_64/x32/sysdep.h#L27

NobodyXu avatar Apr 20 '24 11:04 NobodyXu

Thanks for the investigation.

Before implementing this, I would suggest we re-evaluate the value we gain from this Linux specific optimization. If it is pretty beneficial, then we can consider adding it, otherwise this might introduce a bit too many complexity and compatibility issues, and I'd rather remove preadv2.

weihanglo avatar Apr 20 '24 14:04 weihanglo

By re-evaluations, I mean something like listing out in which situation preadv2 would help, and how common they are, as well as looking into the performance side like benchmarking, especially real-world benchmarks.

weihanglo avatar Apr 20 '24 14:04 weihanglo

Can we please start off by reverting so that builds everywhere start passing once again?

ofek avatar Apr 20 '24 14:04 ofek

By re-evaluations, I mean something like listing out in which situation preadv2 would help, and how common they are,

preadv2 would help in situations where procfs isn't present and jobserver is given a pipe instead of fifo.

It's possible that some dev envs block access to procfs due to safety/sandboxing reason.

cc @the8472 since he might have more knowledge of this and how to handle it

Mote that preadv2 is currently disabled on musl due to musl not supporting it yet.

NobodyXu avatar Apr 20 '24 15:04 NobodyXu

It's possible that some dev envs block access to procfs due to safety/sandboxing reason.

Is jobserver completely unusable without this optimization for this situation? That being said, if a simple syscall can resolve the issue without breaking old kernel/glibc, I am pretty okay with it. We already have similar work like: https://github.com/rust-lang/jobserver-rs/blob/7b96b4170e1e7cbeceb95508fb18de8339c9c3dc/src/unix.rs#L70-L72

weihanglo avatar Apr 20 '24 15:04 weihanglo

preadv2 also helps in the sense that the FD does not have to be set to O_NONBLOCK which allows the acquire path continue to use blocking reads which are more efficient on pipes than the poll + non-blocking read dance.

That being said, if a simple syscall can resolve the issue without breaking old kernel/glibc, I am pretty okay with it. We already have similar work like:

https://github.com/rust-lang/jobserver-rs/blob/7b96b4170e1e7cbeceb95508fb18de8339c9c3dc/src/unix.rs#L70-L72

Yes, the other calls should have used the same pattern.

the8472 avatar Apr 20 '24 16:04 the8472

Is there anything I can do to help expedite the release? It looks like others are increasingly affected by this.

ofek avatar Apr 22 '24 02:04 ofek

Is there anything I can do to help expedite the release? It looks like others are increasingly affected by this.

I wonder how many don't even understand what the issue is when it happens. I luckly traced it back through the cc crate, but it was really a stretch to have to note the update 3 hours before and then people stating it is this jobserver-rs. This for me is an application that should build fine with cargo-c as a popular utility used in gstreamer rs. Which currently breaks without a wedge like I inserted forcing a special version of cargo-c downgrading cc temporarily and effectively bypassing this for now. Not a pretty thing to have to do :/ for many it just would result in thinking something is broken with gstreamer or anything based on cargo-c building. It is a "break" that is a huge show stopper if using cargo-c and not knowing how to shim it to work. (maybe I am being too dramatic, yet I was up 3am about done with a huge RPM build that broke suddenly without explanation or logic till I tracked this down taking a 2-3 hours side-track task. Surprised I figured it out and kept going forward with my work, didn't expect such a thing to happen, feels like a flaw to be able to break the stable release through something like this plus having such a convoluted path for testing/confirming staging with such changes that can do this sort of breakage.).

groovybits avatar Apr 22 '24 02:04 groovybits

It's unfortunate, I didn't realise that the preadv2 API is way too new that glibc still don't have it.

NobodyXu avatar Apr 22 '24 03:04 NobodyXu

Is there anything I can do to help expedite the release? It looks like others are increasingly affected by this.

Already pinged petrochenkov on zulip, hopefully a new release will be published soon

NobodyXu avatar Apr 22 '24 03:04 NobodyXu

It's unfortunate, I didn't realise that the preadv2 API is way too new that glibc still don't have it.

The dilemma is that our platform support looks like this, so we support glibc 2.17: https://doc.rust-lang.org/rustc/platform-support.html

In fact, our baseline for Linux kernels definitely don't support this!

workingjubilee avatar Apr 23 '24 01:04 workingjubilee

The preadv2 usage is reverted in [email protected], so everything should be fine now. Thank you for your patience people :)

weihanglo avatar Apr 23 '24 01:04 weihanglo

Thanks -- could jobserver 0.1.30 be yanked so downstream consumers get the right message?

sunshowers avatar Apr 24 '24 21:04 sunshowers

Everything works for me now, thanks so much!

ofek avatar Apr 25 '24 00:04 ofek

I think we should also run CI on musl, on linux musl and gnu have enough differences for them to be considered as two different targets.

NobodyXu avatar May 05 '24 08:05 NobodyXu