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

My_nat.free_udp_port: avoid looping forever, use last_resort_port earlier #159

Closed
wants to merge 1 commit into from

Conversation

hannesm
Copy link
Member

@hannesm hannesm commented Nov 8, 2022

This avoids using the entire CPU. See #158 for the issue description (especially my comment #158 (comment)).

@hannesm hannesm mentioned this pull request Nov 8, 2022
@palainp
Copy link
Member

palainp commented Nov 8, 2022

Thanks for that PR. After some times setting up correctly my mirage-fw I can confirm that it doesn't burn the CPU anymore.
LGTM (together with the mirage-nat update that also helps in solving this issue)!

@hannesm
Copy link
Member Author

hannesm commented Nov 9, 2022

I would like to verify first that the mirage-nat change (and 3.0.1 release) fixes the issue. This change here is "purely cosmetical" and shouldn't be needed. Though it as well helps to avoid busy loops (since the free_udp_port will be bounded).

While working on this, another issue is how DNS replies are handled with a single global mailbox (Lwt_mvar)

  • a DNS query is sent out
  • whenever a DNS reply is received, this is pushed into the mailbox

This means there can be concurrent requests with unclear ordering, and also the Lwt_mvar.put (in uplink) can block if there's nobody reading the mailbox (which only my_dns is). So that code should be revised that DNS replies are matching the requests, and there's no blocking behaviour if more replies are received than requests that were sent. Especially evil since there's a timeout which cancels the Lwt_mvar.take...

@palainp
Copy link
Member

palainp commented Nov 11, 2022

Thanks @hannesm for your amazing work on that points!
I can confirm that the PR with mirage-nat alone (without this PR) permit to not overload CPU, but I'll feel better to have a bounded loop :)

@hannesm
Copy link
Member Author

hannesm commented Nov 11, 2022

this has been part of #163 and the 0.8.3 release

@hannesm hannesm closed this Nov 11, 2022
@hannesm hannesm deleted the my-nat-fix-free-udp-port branch November 11, 2022 15:53
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.

2 participants