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

remove frameQ #150

Closed
wants to merge 5 commits into from
Closed

remove frameQ #150

wants to merge 5 commits into from

Conversation

palainp
Copy link
Member

@palainp palainp commented Oct 6, 2022

This update mainly removes code that is no longer needed:

  • frameQ was used to limit the number of promises waiting to be delivered, and somehow it now works without that + it increases performance with tcp as it doesn't drop packets (which will have to be retransmitted)
  • the nat table should no longer be dropped (and is capped at a lower value by default) as it should not grow without control these days (but we still need to call memory_pressure.status to GC from time to time)
  • the firewall seems to have decent performance (according to local iperf tests) with only 32MB

config.ml Outdated Show resolved Hide resolved
@hannesm
Copy link
Member

hannesm commented Oct 6, 2022

In my_nat.ml, should we as well apply the diff:

diff --git a/my_nat.ml b/my_nat.ml
index 2652ff5..94cf790 100644
--- a/my_nat.ml
+++ b/my_nat.ml
@@ -64,15 +64,7 @@ let remove_connections t ports ip =
   ports.nat_icmp := Ports.diff !(ports.nat_icmp) (Ports.of_list freed_ports.Mirage_nat.icmp)
 
 let add_nat_rule_and_translate t ports ~xl_host action packet =
-  let apply_action xl_port =
-    Lwt.catch (fun () ->
-        Nat.add t.table packet (xl_host, xl_port) action
-      )
-      (function
-        | Out_of_memory -> Lwt.return (Error `Out_of_memory)
-        | x -> Lwt.fail x
-      )
-  in
+  let apply_action xl_port = Nat.add t.table packet (xl_host, xl_port) action in
   let rec aux ~retries =
     let nat_ports, dns_ports =
       match packet with
@@ -82,12 +74,6 @@ let add_nat_rule_and_translate t ports ~xl_host action packet =
     in
     let xl_port = pick_free_port ~nat_ports ~dns_ports in
     apply_action xl_port >>= function
-    | Error `Out_of_memory ->
-      (* Because hash tables resize in big steps, this can happen even if we have a fair
-         chunk of free memory. *)
-      Log.warn (fun f -> f "Out_of_memory adding NAT rule. Dropping NAT table...");
-      reset t ports >>= fun () ->
-      aux ~retries:(retries - 1)
     | Error `Overlap when retries < 0 -> Lwt.return (Error "Too many retries")
     | Error `Overlap ->
       if retries = 0 then (

@hannesm
Copy link
Member

hannesm commented Oct 8, 2022

the proposed my_nat changes above are done in #151

@palainp palainp changed the title Update mirleft remove frameQ Oct 9, 2022
@hannesm hannesm mentioned this pull request Oct 11, 2022
@hannesm
Copy link
Member

hannesm commented Oct 12, 2022

thanks, merged via #152

@hannesm hannesm closed this Oct 12, 2022
@palainp palainp deleted the update-mirleft branch October 17, 2022 14:14
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