Skip to content

AsyncUdpSocket::poll_recv fairness #2981

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

Closed
flub opened this issue Nov 29, 2024 · 0 comments · Fixed by #2996
Closed

AsyncUdpSocket::poll_recv fairness #2981

flub opened this issue Nov 29, 2024 · 0 comments · Fixed by #2996
Labels
bug Something isn't working c-iroh

Comments

@flub
Copy link
Contributor

flub commented Nov 29, 2024

AsyncUdpSocket::poll_recv multiplexes receiving datagrams from 3 different sources:

  • IPv4 UDP socket
  • IPv6 UDP socket
  • Relay channel

Currently they are polled in this specific order: first IPv4, only if that returns Pending poll IPv6 and only if also that returns Pending does it poll the relay channel. If one of the polls returns Ready the remaining ones are not polled.

This unfairly penalises the relay channel.


A possible solution is for the MagicSock to have buffers for datagrams that are ready. Whenever there is something in the buffers return it as Poll::Ready without polling any of the sources. If the buffers are empty always poll all the sources and store any Ready results that are not immediately returned in a buffer.


This is one of the issues found from #2951.

@flub flub added bug Something isn't working c-iroh labels Nov 29, 2024
@flub flub added this to iroh Nov 29, 2024
github-merge-queue bot pushed a commit that referenced this issue Dec 3, 2024
## Description

The existing AsyncUdpSocket::poll_recv polled the UDP IPv4 socket
first, only if that has no datagrams would it poll UDP IPv6 and only
also that had no datagrams to deliver was the relay socket polled.

This highly favoured IPv4 relay traffic, potentially starving the
relay traffic.

This change makes it prefer a different source on each poll.  Meaning
if multiple sockets have data to deliver a fairer delivery happens.

Closes #2981.

## Breaking Changes

<!-- Optional, if there are any breaking changes document them,
including how to migrate older code. -->

## Notes & open questions

- Using Ordering::Relaxed is probably fine.  I believe normally
  this polling would only happen from a single task (the Quinn
  endpoint driver) and even if it was polled concurrently from
  different threads it would not be that bad and still an improvement
  on the current order.
  
- I'm not particularly fond of the implementation, but the macros is
  the best I could come up with.  Maybe there's something cleverer
  which can be done.  

## Change checklist

- [x] Self-review.
- [x] Documentation updates following the [style
guide](https://rust-lang.github.io/rfcs/1574-more-api-documentation-conventions.html#appendix-a-full-conventions-text),
if relevant.
- [x] Tests if relevant.
- [x] All breaking changes documented.

---------

Co-authored-by: Philipp Krüger <[email protected]>
@flub flub closed this as completed in #2996 Dec 3, 2024
@github-project-automation github-project-automation bot moved this to ✅ Done in iroh Dec 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working c-iroh
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant