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

Fix broken test_udp_recvmsg_multishot_trunc test #284

Merged
merged 2 commits into from
May 9, 2024

Conversation

SUPERCILEX
Copy link
Contributor

@SUPERCILEX SUPERCILEX commented May 4, 2024

The bug is that I wasn't using buf_id to pick the buffers but rather assumed the kernel would return responses in order.

@SUPERCILEX SUPERCILEX force-pushed the truncate-test branch 2 times, most recently from 499ac1c to 5207c43 Compare May 4, 2024 05:06
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
@connortsui20
Copy link
Contributor

Note that I'm running this on a laptop now, but still running the same version of Arch Linux

I now get this error:

test udp_recvmsg_multishot_trunc
thread 'main' panicked at io-uring-test/src/tests/net.rs:1523:5:
assertion `left == right` failed
  left: 4
 right: 5

Looking at the two lines right before this one, I'm pretty sure that this assertion should be equal to 4 and not 5 since I think that submit_and_wait will block until there are "wait_nr completion events" in the completion queue, as opposed to waiting until wait_nr operations complete that were originally in flight when io_uring_submit_and_wait was called.

After changing that to 4 I get a new error:

thread 'main' panicked at io-uring-test/src/tests/net.rs:1542:32:
index out of bounds: the len is 2 but the index is 2

I haven't looked into the code but this might be a logical error given the completion events == 5 thing?

@SUPERCILEX
Copy link
Contributor Author

Nah, the test passes on my machine. I feel like io_uring is just broken on your kernel. The 4 events thing is fine, but that index out of bounds is because the kernel returned a buffer we never gave it. Like what? Maybe that was the old mechanism for signaling no more buffers? Lemme try that real quick.

@SUPERCILEX SUPERCILEX force-pushed the truncate-test branch 4 times, most recently from 2f23531 to 4588ea8 Compare May 6, 2024 05:23
Signed-off-by: Alex Saveau <saveau.alexandre@gmail.com>
@SUPERCILEX
Copy link
Contributor Author

Alright, well I think these changes might be wrong (I added a mod the buffer size), but maybe that's how the kernel works? It just monotonically increases the buf ID? I wish this stuff was documented properly 😭. https://lwn.net/Articles/815491/ helped a little bit.

@connortsui20
Copy link
Contributor

Test now passes on my machine!

@SUPERCILEX
Copy link
Contributor Author

Sweet! I still feel iffy about the mod thing, but hey if it works I guess.

@quininer Do we want to delete the kernel version check code?

@SUPERCILEX SUPERCILEX marked this pull request as ready for review May 6, 2024 17:19
@quininer
Copy link
Member

quininer commented May 7, 2024

You can keep it, feel free.

@quininer quininer merged commit c49e72e into tokio-rs:master May 9, 2024
12 checks passed
@quininer
Copy link
Member

quininer commented May 9, 2024

Thank you!

@SUPERCILEX SUPERCILEX deleted the truncate-test branch May 22, 2024 15:32
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.

3 participants