CP-32622: avoid using select and instead use epoll #4877

snwoods · 2022-12-16T17:06:42Z

To avoid xapi from running out of file descriptors, replace select with epoll in most cases

ocaml/xapi-idl/lib/posix_channel.ml

robhoes · 2022-12-16T17:34:07Z

ocaml/xapi-idl/lib/posix_channel.ml

+      let epoll = Polly.create () in
+      List.iter (fun fd -> Polly.add epoll fd Polly.Events.inp) (r @ w) ;
+      ignore
+      @@ Polly.wait epoll 4 (-1) (fun _ fd _ ->


I think that the old code can be summarised as: "Wait for any r to be ready to reading or any of the w to be ready for writing. Then read/write to the fd found ready. Start again.". In every round of the loop (every time select returns) one or more fds would be ready, and it may often be just one. So we can't assume that we can write + read in the same round.

The third argument of the function passed in here, currently ignore with _, is set to the kind of event that happened and indicates whether we can read from or write to fd.

snwoods · 2022-12-21T10:19:29Z

ocaml/networkd/lib/jsonrpc_client.ml

-          inner remain_time max_bytes
-    else
-      inner remain_time max_bytes
+    Unix.setsockopt_float fd Unix.SO_RCVTIMEO (Int64.to_float max_time) ;


This line is giving a parse error on make test

lindig · 2023-01-10T16:11:56Z

ocaml/xenopsd/cli/xn.ml

+                     if !i < !buf_remote_end + b then final := true ;
+                     buf_remote_end := !i
+                   ) ;
+                   if fd = file_desc then


An alternative style could use match:

match file_desc with | file_desc when file_desc = Unix.stdin -> .. | file_desc when file_desc = fd -> .. | _otherwise ->

Do we want to handle one or all FDs that are ready? Up to 2 can be ready. This code handles two if they are ready but we have other places where we handle only one. It's not obvious why we have different behavior.

edwintorok · 2023-01-19T13:41:45Z

ocaml/forkexecd/src/child.ml

+    [comms_sock; fd_sock] ;
+  (* Although there are two fds, we set max_fds to 1 here as we only want this
+     function to trigger once so that we get one return value *)
+  Polly.wait_fold epoll 1 (-1) state (fun _ fd _ _ ->


Is 1 correct here though, would that prevent it from even watching the other FD?

My understanding is that one FD would be processed per round - and any other FD that is ready would be reported again in the next round. But does this guarantee fairness?

This was the only way I could think to make it work, do you have any other alternatives?

lindig · 2023-01-24T09:30:43Z

Remember that the timeout in Polly.wait is specified in milliseconds. Maybe I should change the Polly interface to use a labeled argument timeout_ms.

lindig · 2023-02-01T10:31:49Z

ocaml/xsh/xsh.ml

+        ~finally:(fun () -> Polly.close epoll)
+        (fun () ->
+          ignore
+          @@ Polly.wait epoll 4 (-1) (fun _ fd _ ->


Note that the code below matches exactly one condition and not all fds that are ready; could this be written as

if fd = x then do something; if fd = y then do something; ..

Maybe leave a comment why the selected behavior is the right one.

lindig · 2023-02-01T10:35:44Z

ocaml/xenopsd/cli/xn.ml

+                     if !i < !buf_remote_end + b then final := true ;
+                     buf_remote_end := !i
+                   ) ;
+                   if fd = file_desc then


Do we want to handle one or all FDs that are ready? Up to 2 can be ready. This code handles two if they are ready but we have other places where we handle only one. It's not obvious why we have different behavior.

lindig · 2023-02-01T10:38:10Z

ocaml/xapi-idl/lib/posix_channel.ml

+        (fun () ->
+          ignore
+          @@ Polly.wait epoll 4 (-1) (fun _ fd event ->
+                 if event = Polly.Events.inp then


Again, this handles at most one FD. I think the unconditional else case is a bit dangerous and I would explicitly check than any FD is ready before acting on it. This applies to other places as well.

lindig · 2023-02-01T10:45:33Z

ocaml/networkd/lib/jsonrpc_client.ml

+    match Unix.read fd bytes 0 4096 with
+    | 0 ->
+        Buffer.contents buf (* EOF *)
+    | n ->


Could be expressed as

| n when n >= max_bytes -> .. | n (* otherwise *) -> | exception .. ->

edwintorok · 2023-02-01T10:23:21Z

ocaml/xsh/xsh.ml

+      List.iter (fun fd -> Polly.add epoll fd Polly.Events.inp) r ;
+      List.iter (fun fd -> Polly.add epoll fd Polly.Events.out) w ;
+      Fun.protect
+        ~finally:(fun () -> Polly.close epoll)


we use this pattern quite a lot, should we have a with_polly somewhere?

edwintorok · 2023-02-01T10:41:15Z

ocaml/database/block_device_io.ml

+    (fun () ->
+      Unix.setsockopt_float s Unix.SO_RCVTIMEO timeout ;
+      try fst (Unix.accept s)
+      with Unix.Unix_error (Unix.EAGAIN, _, _) -> raise Unixext.Timeout


Does SO_RCVTIMEO work with accept? I can't find a clear statement for or against in the manpage of 'socket(7)', it mentions read/recvmsg/send/sendmsg

It looks like you're right, the only examples online of using a timeout with accept are using select

edwintorok · 2023-02-01T10:59:27Z

ocaml/networkd/lib/jsonrpc_client.ml

-          inner remain_time max_bytes
-    else
-      inner remain_time max_bytes
+    ( try Unix.setsockopt_float fd SO_RCVTIMEO (Int64.to_float max_time)


We do seem to repeat this quite often, perhaps as a future cleanup PR we could define a helper function: 'timed_read' which does the setsockopt+read+fun.protect to set it back, same for write a 'timed_write' might be useful

Yes, this would avoid mistakes, such as around resetting the socket option.

edwintorok · 2023-02-01T11:02:19Z

ocaml/xapi-idl/lib/posix_channel.ml

+                             to_close := fd :: !to_close ;
+                             proxy fd proxy_socket
+                           ) else
+                             assert false (* can never happen *)


Wouldn't be so sure, we could get spurious notifications from 'epoll', and we don't want to crash the application (or thread). Perhaps just log it as error for now?
Especially that you've added 2 file descriptors s_ip and s_unix, this will NOT be just s_unix, it is entirely possible that s_ip will have something available, otherwise why add it to epoll in the first place?

snwoods · 2023-02-06T09:36:45Z

I believe all of the comments have now been addressed.

psafont · 2023-02-06T12:52:12Z

ocaml/database/block_device_io.ml

+  if fds = [] then (* We must have timed out *)
    raise Unixext.Timeout
+  else (* There will only ever be a maximum of one fd *)
+    List.hd fds


While List.hd is guarded here, it's preferrable to destructure the list to obtain values from it:

Suggested change

if fds = [] then (* We must have timed out *)

raise Unixext.Timeout

else (* There will only ever be a maximum of one fd *)

List.hd fds

match fds with

| [] -> (* We must have timed out *)

raise Unixext.Timeout

| fd :: _ -> (* There will only ever be a maximum of one fd *)

fd

snwoods · 2023-02-09T10:36:58Z

All comments addressed and suiterun 179915 passing.

robhoes · 2023-02-09T13:49:59Z

ocaml/libs/stunnel/stunnel.ml

@@ -394,7 +394,7 @@ let rec retry f = function
    try f ()
    with Stunnel_initialisation_failed ->
      (* Leave a few seconds between each attempt *)
-      ignore (Unix.select [] [] [] 3.) ;
+      Unix.sleep 3 ;


Use Thread.delay

Signed-off-by: Steven Woods <[email protected]>

of pipes Signed-off-by: Steven Woods <[email protected]>

Signed-off-by: Steven Woods <[email protected]>

loop as its contents has been removed in a previous commit. Signed-off-by: Steven Woods <[email protected]>

Signed-off-by: Steven Woods <[email protected]>

robhoes · 2023-03-13T13:52:59Z

ocaml/libs/http-svr/buf_io.ml

+        n
+      with Unix.Unix_error (Unix.EAGAIN, _, _) -> -1
+    in
+    Unix.setsockopt_float ic.fd Unix.SO_RCVTIMEO 0. ;


I think we'd better use finally to ensure that this timeout is reset in case of any exception (only one kind is caught above.

Every read on ic.fd seems to go through here, and each read is preceded by setting a timeout, so I don't think we even need to reset the timeout, just drop this line.

There are more reads in http.ml but they also set a socket timeout. Will need to check the other callers too.

robhoes · 2023-03-13T13:55:09Z

ocaml/message-switch/unix/protocol_unix_scheduler.ml

-          r = []
+          try
+            ignore (Unix.read sock_out (Bytes.create 1) 0 1) ;
+            Unix.setsockopt_float sock_out Unix.SO_RCVTIMEO 0. ;


Here we'd miss resetting the socket option in case of an exception.

edwintorok · 2023-11-17T11:30:48Z

Testing whether 'select' is completely gone or not is tricky due to a lot of dead code in the linked binary.
I hacked up a script to find at least the actual OCaml functions that call 'Unix.select' directly:

#!/bin/sh
set -eu

EXE="$1"
ADDR="${EXE}.unix_select.address"
DISASM="${EXE}.disasm"

# Find address of 'unix_select' symbol
addr2line -pae "${EXE}" unix_select \
  | sed -e "s/^0x0*\\([0-9a-fA-F]*\\):.*/\\1/" \
  | tr -d '\n' >"${ADDR}"

# Find all calls to 'unix_select', which is done by
#  mov $0x...,%rax
#  call <caml_c_call>

# disassemble, but without sources
# (source lookup doesn't work for all dependencies, and is very slow on a large binary)
# To make debugging easier the disassembly is saved to a file instead of piping
objdump -d --no-show-raw-insn --no-addresses "${EXE}" >"${DISASM}"

SYMADDR=$(cat "${ADDR}")
# Prints a list of functions that call unix_select
# But we won't know whether it is dead code or not
gawk -f unix_select.gawk "${SYMADDR}" <"${DISASM}"

unix_select.gawk:

BEGIN { SYMADDR=ARGV[1]; ARGV[1]=""; }

/^<.*/ { SYMBOL=$1 }
match($0, /mov.*0x([0-9a-fA-F]*),%rax/, addr) { RAX=addr[1]; }
/call.*<caml_c_call>/ { if (RAX == SYMADDR) { print SYMBOL }  }

It shows these for XAPI and xenopsd:

./no-unix-select.sh _build/default/ocaml/xapi/xapi_main.exe
<camlXapi_stdext_threads__Threadext__fun_812>:
<camlXapi_stdext_unix__Unixext__kill_and_wait_inner_2120>:
<camlXapi_stdext_unix__Unixext__proxy_1371>:
<camlXapi_stdext_unix__Unixext__time_limited_write_internal_1424>:
<camlXapi_stdext_unix__Unixext__time_limited_read_1450>:
<camlThread__wait_timed_read_777>:
<camlThread__wait_timed_write_781>:
<camlUnix__fun_2441>:

And for xenopsd:

./no-unix-select.sh _build/default/ocaml/xenopsd/xc/xenops_xc_main.exe
<camlXapi_stdext_unix__Unixext__kill_and_wait_inner_2120>:
<camlXapi_stdext_unix__Unixext__proxy_1371>:
<camlXapi_stdext_unix__Unixext__time_limited_write_internal_1424>:
<camlXapi_stdext_unix__Unixext__time_limited_read_1450>:
<camlXapi_stdext_threads__Threadext__fun_812>:
<camlWatch__fun_884>:
<camlThread__wait_timed_read_777>:
<camlThread__wait_timed_write_781>:
<camlUnix__fun_2441>:

edwintorok · 2023-11-17T11:33:51Z

The 'select' in watch.ml can be replaced similarly as elsewhere by changing pipe->socket and using socket timeouts.

edwintorok · 2023-11-21T17:38:16Z

Removed some Unix.select and Thread.wait* usages in xapi-project/stdext#80.

And a few more from XAPI and xenopsd as well: https://github.com/edwintorok/xen-api/commits/private/edvint/CP-32622

We'll probably need a CA ticket and backport the watch.ml fix since it affects xenopsd which already claims to support >1024 fds.

I think these now remove all the obvious Unix.select usages, but we should implement what has been suggested here and add some CI checks in xs-opam with various methods to check that we really don't call it anymore (and then add some unit tests to all these functions that we modified to check that they still work correctly):
https://discuss.ocaml.org/t/ensuring-that-an-ocaml-binary-never-calls-unix-select/13465/7?u=edwin

edwintorok · 2023-11-23T15:15:15Z

I've updated the no-unix-select.sh to be recursive up to a certain depth, and it didn't find more calls.
Going to try the .cmt iterator suggested in the forums just to have another way to validate this.

snwoods · 2024-03-11T13:23:51Z

I'm closing this and have opened a PR against Edwin's fork so that he can still keep track of it edwintorok#4

robhoes reviewed Dec 16, 2022

View reviewed changes

ocaml/xapi-idl/lib/posix_channel.ml Outdated Show resolved Hide resolved

robhoes reviewed Dec 16, 2022

View reviewed changes

snwoods commented Dec 21, 2022

View reviewed changes

snwoods force-pushed the private/stevenwo/CP-32622 branch 3 times, most recently from 4a79673 to a562d49 Compare January 10, 2023 15:57

lindig reviewed Jan 10, 2023

View reviewed changes

snwoods force-pushed the private/stevenwo/CP-32622 branch 2 times, most recently from 823ef93 to ea9cb03 Compare January 13, 2023 14:31

edwintorok reviewed Jan 19, 2023

View reviewed changes

snwoods force-pushed the private/stevenwo/CP-32622 branch 3 times, most recently from 535388b to 596a6dc Compare January 23, 2023 12:31

snwoods marked this pull request as ready for review January 23, 2023 12:31

snwoods force-pushed the private/stevenwo/CP-32622 branch from 596a6dc to 33c4bb6 Compare January 23, 2023 12:34

psafont changed the title ~~Private/stevenwo/cp 32622~~ CP-32622: avoid using select and instead use epoll Jan 23, 2023

snwoods force-pushed the private/stevenwo/CP-32622 branch from 33c4bb6 to d4d6c10 Compare January 24, 2023 10:07

lindig approved these changes Feb 1, 2023

View reviewed changes

edwintorok reviewed Feb 1, 2023

View reviewed changes

snwoods force-pushed the private/stevenwo/CP-32622 branch 5 times, most recently from 8f13617 to 89bb393 Compare February 6, 2023 09:36

snwoods force-pushed the private/stevenwo/CP-32622 branch 2 times, most recently from d4b4878 to e124de2 Compare February 6, 2023 12:39

psafont reviewed Feb 6, 2023

View reviewed changes

snwoods force-pushed the private/stevenwo/CP-32622 branch from e124de2 to da3f2d9 Compare February 6, 2023 13:07

snwoods force-pushed the private/stevenwo/CP-32622 branch 2 times, most recently from eed05d5 to f259cae Compare February 8, 2023 11:30

robhoes reviewed Feb 9, 2023

View reviewed changes

snwoods added 8 commits February 10, 2023 10:47

CP-32622: Use Unix.sleepf for sleeps instead of select

aa166fb

Signed-off-by: Steven Woods <[email protected]>

CP-32622: Replace select in buf_io

2877609

Signed-off-by: Steven Woods <[email protected]>

CP-32622: Replace select in protocol_unix_scheduler. Use sockets instead

416bf66

of pipes Signed-off-by: Steven Woods <[email protected]>

CP-32622: Replace select in jsonrpc_client

aa9fb63

Signed-off-by: Steven Woods <[email protected]>

CP-32622: Replace select with polly in posix_channel

16508f1

Signed-off-by: Steven Woods <[email protected]>

CP-32622: Replace select with polly in xe-cli. Remove redundant while

f6819e0

loop as its contents has been removed in a previous commit. Signed-off-by: Steven Woods <[email protected]>

CP-32622: Replace select with polly in xn

d86f1b8

Signed-off-by: Steven Woods <[email protected]>

CP-32622: Replace select with polly in xsh

f4c2087

Signed-off-by: Steven Woods <[email protected]>

snwoods force-pushed the private/stevenwo/CP-32622 branch from f259cae to f4c2087 Compare February 10, 2023 10:48

robhoes reviewed Mar 13, 2023

View reviewed changes

snwoods mentioned this pull request Mar 11, 2024

CP-32622: avoid using select and instead use epoll edwintorok/xen-api#4

Draft

snwoods closed this Mar 11, 2024

edwintorok mentioned this pull request Jun 19, 2024

[epoll] Unix.select conversion: replace with stdext modules/calls or polly calls #5705

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CP-32622: avoid using select and instead use epoll #4877

CP-32622: avoid using select and instead use epoll #4877

snwoods commented Dec 16, 2022 •

edited by psafont

Loading

robhoes Dec 16, 2022

snwoods Dec 21, 2022

lindig Jan 10, 2023

lindig Feb 1, 2023

edwintorok Jan 19, 2023

lindig Jan 19, 2023

snwoods Jan 23, 2023

lindig commented Jan 24, 2023

lindig Feb 1, 2023

lindig Feb 1, 2023

lindig Feb 1, 2023

lindig Feb 1, 2023

edwintorok Feb 1, 2023

edwintorok Feb 1, 2023

snwoods Feb 3, 2023

edwintorok Feb 1, 2023

robhoes Mar 13, 2023

edwintorok Feb 1, 2023

snwoods commented Feb 6, 2023

psafont Feb 6, 2023

snwoods commented Feb 9, 2023

robhoes Feb 9, 2023

robhoes Mar 13, 2023

edwintorok Nov 16, 2023

edwintorok Nov 16, 2023

robhoes Mar 13, 2023

edwintorok commented Nov 17, 2023

edwintorok commented Nov 17, 2023

edwintorok commented Nov 21, 2023

edwintorok commented Nov 23, 2023

snwoods commented Mar 11, 2024

CP-32622: avoid using select and instead use epoll #4877

CP-32622: avoid using select and instead use epoll #4877

Conversation

snwoods commented Dec 16, 2022 • edited by psafont Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lindig commented Jan 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

snwoods commented Feb 6, 2023

Choose a reason for hiding this comment

snwoods commented Feb 9, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

edwintorok commented Nov 17, 2023

edwintorok commented Nov 17, 2023

edwintorok commented Nov 21, 2023

edwintorok commented Nov 23, 2023

snwoods commented Mar 11, 2024

snwoods commented Dec 16, 2022 •

edited by psafont

Loading