Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

implement fadvise for linux #8301

Merged
merged 2 commits into from
Jun 9, 2021
Merged

implement fadvise for linux #8301

merged 2 commits into from
Jun 9, 2021

Conversation

vrischmann
Copy link
Contributor

Continuation of #8095 but I forced-push and GitHub doesn't allow me to reopen the PR.

I think I took care of all special cases for the architectures that work in zig today.

lib/std/os/linux.zig Outdated Show resolved Hide resolved
lib/std/os/linux.zig Outdated Show resolved Hide resolved
lib/std/os/linux.zig Outdated Show resolved Hide resolved
lib/std/os/linux.zig Outdated Show resolved Hide resolved
lib/std/os/linux/test.zig Outdated Show resolved Hide resolved
@vrischmann vrischmann force-pushed the fadvise branch 2 times, most recently from 9c24722 to 07bd194 Compare March 20, 2021 18:39
@vrischmann
Copy link
Contributor Author

sparcv9 has both fadvise64 and fadvise64_64 but for some reason fadvise64_64 is broken:

$ zig build -Dtarget=x86_64-linux
$ strace -e fadvise64_64,fadvise64 ./zig-cache/bin/zig-fadvise
fadvise64(3, 0, 28, POSIX_FADV_SEQUENTIAL) = 0
$ zig build -Dtarget=sparcv9-linux
$ strace -e fadvise64_64,fadvise64 ./zig-cache/bin/zig-fadvise
fadvise64(3, 0, 0, POSIX_FADV_NORMAL)   = 0

I can't test on a real sparc machine but I'm assuming it's broken given the strace output.

If I make it use fadvise64 for the sparc architectures it's fine. Should I special case sparc too ?

@LemonBoy
Copy link
Contributor

sparcv9 has both fadvise64 and fadvise64_64 but for some reason fadvise64_64 is broken:

Hmm, it works for me. I tried running the attached test case under qemu and the parameters look ok.

@vrischmann
Copy link
Contributor Author

sparcv9 has both fadvise64 and fadvise64_64 but for some reason fadvise64_64 is broken:

Hmm, it works for me. I tried running the attached test case under qemu and the parameters look ok.

Weird. I'm running the test binary with strace -e fadvise64,fadvise64_64 qemu-sparc64 and I get this:

fadvise64(3, 0, 0, POSIX_FADV_NORMAL)   = 0

the advice should be POSIX_FADV_SEQUENTIAL.

@LemonBoy
Copy link
Contributor

Well fuck me (and SPARC as a whole), that's the only 64bit platform where fadvise64_64 is defined and behaves as a fadvise64.
Changing the check to @hasField(SYS, "fadvise64_64") and usize_bits != 64 should solve the problem, please add a brief comment explaining why the extra check is needed.

@vrischmann
Copy link
Contributor Author

I added the workaround and a comment in the latest commit.

For some reason one check is still shown as pending, but it finished successfully.

@vrischmann
Copy link
Contributor Author

I noticed the offset/len types changed with 31f1cc9 so I changed it here too. Also rebased on latest master.

@Vexu Vexu merged commit c71e8a3 into ziglang:master Jun 9, 2021
pub const POSIX_FADV_DONTNEED = 6;
pub const POSIX_FADV_NOREUSE = 7;
}
else
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is going on with indentation here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

well that's what zig fmt produces.

@vrischmann vrischmann deleted the fadvise branch June 9, 2021 15:25
@alexrp
Copy link
Member

alexrp commented Jun 23, 2024

@vrischmann @LemonBoy I'm unfortunately having to touch the fadvise code in #20389. I don't suppose you could shed some light on the sparc64 thing? I don't see any special-casing of sparc64 in glibc or musl. The kernel syscalls also look how I'd expect. So I'm genuinely puzzled at the need for the special case here. Is it possible you were observing a QEMU bug?

@alexrp alexrp mentioned this pull request Jun 23, 2024
4 tasks
@vrischmann
Copy link
Contributor Author

Hi @alexrp. Sorry, I can't remember the details but I tested this code again right now and it looks like it works ?

$ zig build -Dtarget=x86_64-linux
$ strace -e fadvise64_64,fadvise64 ./zig-out/bin/zig-fadvise
fadvise64(3, 0, 0, POSIX_FADV_SEQUENTIAL) = 0
fadvise ret: 0+++ exited with 0 +++
$ zig build -Dtarget=sparc64-linux
$ strace -e fadvise64_64,fadvise64 qemu-sparc64 ./zig-out/bin/zig-fadvise
fadvise64(3, 0, 0, POSIX_FADV_SEQUENTIAL) = 0
fadvise ret: 0+++ exited with 0 +++

maybe you're right and it was a qemu bug ?

@alexrp
Copy link
Member

alexrp commented Jun 23, 2024

@vrischmann just to clarify, is that actually testing with a call to fadvise64_64 or fadvise64 in the code?

Also, did you actually observe it having visibility broken behavior? Now I'm wondering if it could just have been an earlier version of strace handling it wrong. 🤔

@vrischmann
Copy link
Contributor Author

vrischmann commented Jun 23, 2024

not sure I understand the question, the code calls os.linux.fadvise which calls fadvise64 or fadvise64_64. But my test doesn't make any sense because I left the special case for sparc64.
To make sure I understand, you're saying that this special case should be unnecessary, right ?

Also, did you actually observe it having visibility broken behavior?

Sadly I can't remember if there was a broken test case or something.

@alexrp
Copy link
Member

alexrp commented Jun 23, 2024

not sure I understand the question, the code calls os.linux.fadvise which calls fadvise64 or fadvise64_64. But my test doesn't make any sense because I left the special case for sparc64.

That's exactly what I was getting at. 🙂 For testing purposes, you'd need to either undo the special case (or do a raw syscall I guess).

To make sure I understand, you're saying that this special case should be unnecessary, right ?

I think so, yes. I can't find any evidence in Linux, glibc, or musl sources that indicate that it should be necessary. The two libcs have no special-casing for SPARC for this system call, and the two system calls end up in the same core function in the kernel.

@vrischmann
Copy link
Contributor Author

I don't know what changed but when built for sparc64-linux the special case isn't even hit: it's using the else case.
In my commit from 2021 I used sparcv9-linux instead but this architecture doesn't seem to exist anymore, I'm confused.

It looks like we could just disregard that special case now.

@alexrp
Copy link
Member

alexrp commented Jun 23, 2024

Hm, ok, I'll go ahead and remove that case then. Thanks for the assistance!

alexrp added a commit to alexrp/zig that referenced this pull request Jun 23, 2024
This does not seem to be needed anymore, and it's unclear if it was ever truly
needed or if it was just there to deal with a QEMU/strace bug.

See: ziglang#8301 (comment)
alexrp added a commit to alexrp/zig that referenced this pull request Jun 26, 2024
This does not seem to be needed anymore, and it's unclear if it was ever truly
needed or if it was just there to deal with a QEMU/strace bug.

See: ziglang#8301 (comment)
alexrp added a commit to alexrp/zig that referenced this pull request Jul 3, 2024
This does not seem to be needed anymore, and it's unclear if it was ever truly
needed or if it was just there to deal with a QEMU/strace bug.

See: ziglang#8301 (comment)
alexrp added a commit to alexrp/zig that referenced this pull request Jul 7, 2024
This does not seem to be needed anymore, and it's unclear if it was ever truly
needed or if it was just there to deal with a QEMU/strace bug.

See: ziglang#8301 (comment)
alexrp added a commit to alexrp/zig that referenced this pull request Jul 9, 2024
This does not seem to be needed anymore, and it's unclear if it was ever truly
needed or if it was just there to deal with a QEMU/strace bug.

See: ziglang#8301 (comment)
alexrp added a commit to alexrp/zig that referenced this pull request Jul 20, 2024
This does not seem to be needed anymore, and it's unclear if it was ever truly
needed or if it was just there to deal with a QEMU/strace bug.

See: ziglang#8301 (comment)
alexrp added a commit to alexrp/zig that referenced this pull request Jul 20, 2024
This does not seem to be needed anymore, and it's unclear if it was ever truly
needed or if it was just there to deal with a QEMU/strace bug.

See: ziglang#8301 (comment)
alexrp added a commit to alexrp/zig that referenced this pull request Jul 20, 2024
This does not seem to be needed anymore, and it's unclear if it was ever truly
needed or if it was just there to deal with a QEMU/strace bug.

See: ziglang#8301 (comment)
alexrp added a commit to alexrp/zig that referenced this pull request Jul 21, 2024
This does not seem to be needed anymore, and it's unclear if it was ever truly
needed or if it was just there to deal with a QEMU/strace bug.

See: ziglang#8301 (comment)
alexrp added a commit to alexrp/zig that referenced this pull request Jul 21, 2024
This does not seem to be needed anymore, and it's unclear if it was ever truly
needed or if it was just there to deal with a QEMU/strace bug.

See: ziglang#8301 (comment)
alexrp added a commit to alexrp/zig that referenced this pull request Jul 21, 2024
This does not seem to be needed anymore, and it's unclear if it was ever truly
needed or if it was just there to deal with a QEMU/strace bug.

See: ziglang#8301 (comment)
alexrp added a commit to alexrp/zig that referenced this pull request Jul 22, 2024
This does not seem to be needed anymore, and it's unclear if it was ever truly
needed or if it was just there to deal with a QEMU/strace bug.

See: ziglang#8301 (comment)
alexrp added a commit to alexrp/zig that referenced this pull request Jul 22, 2024
This does not seem to be needed anymore, and it's unclear if it was ever truly
needed or if it was just there to deal with a QEMU/strace bug.

See: ziglang#8301 (comment)
alexrp added a commit to alexrp/zig that referenced this pull request Jul 22, 2024
This does not seem to be needed anymore, and it's unclear if it was ever truly
needed or if it was just there to deal with a QEMU/strace bug.

See: ziglang#8301 (comment)
alexrp added a commit to alexrp/zig that referenced this pull request Jul 23, 2024
This does not seem to be needed anymore, and it's unclear if it was ever truly
needed or if it was just there to deal with a QEMU/strace bug.

See: ziglang#8301 (comment)
alexrp added a commit to alexrp/zig that referenced this pull request Jul 23, 2024
This does not seem to be needed anymore, and it's unclear if it was ever truly
needed or if it was just there to deal with a QEMU/strace bug.

See: ziglang#8301 (comment)
alexrp added a commit to alexrp/zig that referenced this pull request Jul 24, 2024
This does not seem to be needed anymore, and it's unclear if it was ever truly
needed or if it was just there to deal with a QEMU/strace bug.

See: ziglang#8301 (comment)
alexrp added a commit to alexrp/zig that referenced this pull request Jul 24, 2024
This does not seem to be needed anymore, and it's unclear if it was ever truly
needed or if it was just there to deal with a QEMU/strace bug.

See: ziglang#8301 (comment)
alexrp added a commit to alexrp/zig that referenced this pull request Jul 29, 2024
This does not seem to be needed anymore, and it's unclear if it was ever truly
needed or if it was just there to deal with a QEMU/strace bug.

See: ziglang#8301 (comment)
SammyJames pushed a commit to SammyJames/zig that referenced this pull request Aug 7, 2024
This does not seem to be needed anymore, and it's unclear if it was ever truly
needed or if it was just there to deal with a QEMU/strace bug.

See: ziglang#8301 (comment)
igor84 pushed a commit to igor84/zig that referenced this pull request Aug 11, 2024
This does not seem to be needed anymore, and it's unclear if it was ever truly
needed or if it was just there to deal with a QEMU/strace bug.

See: ziglang#8301 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants