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

CP-32622: Remove 'select' from stdext #80

Closed

Conversation

edwintorok
Copy link
Contributor

Draft, because I need to add some unit tests here.

I'd also like to add a separate function that works only on sockets which is much simpler and uses just 'setsockopt_float'.

@edwintorok edwintorok changed the title Remove 'select' from stdext CP-32622: Remove 'select' from stdext Nov 21, 2023
let proxy (a: Unix.file_descr) (b: Unix.file_descr) =
let size = 64 * 1024 in
(* [a'] is read from [a] and will be written to [b] *)
(* [b'] is read from [b] and will be written to [a] *)
let a' = CBuf.empty size and b' = CBuf.empty size in
Unix.set_nonblock a;
Unix.set_nonblock b;
with_polly @@ fun polly ->

try
while true do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could this be a recursive function with any as a parameter which is used for termination?

lib/xapi-stdext-unix/unixext.ml Outdated Show resolved Hide resolved
@@ -0,0 +1,2 @@
depexts: ["linux-headers"] {os-distribution = "alpine"}
available: [ os = "macos" | os = "linux" ]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

epoll is not available on macOS; not sure if this is relevant here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could use iomux if needed, it supports more OSes. If we want to make xe available on more platforms then we should either remove its dependency on stdext, or avoid hard dependencies on epoll.
For now I've updated the template to drop macos.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although contrary to its claim in the package description 'epoll' support isn't actually implement in iomux yet, it is just poll. Although for most purposes that is enough, epoll is useful if you want to watch a large number of FDs in one go (e.g. XAPI webserver)

@edwintorok
Copy link
Contributor Author

edwintorok commented Nov 28, 2023

I hadn't noticed that there is already #71 which also has one unit test.
I cherry picked the unit test to this PR and it found a bug: time_limited_read takes an absolute time not relative time as timeout (which means that it always timed out instantly because it interpreted the relative timeout as an absolute time in the distant past).

That is very error prone, so I introduced a time_limited_single_read which takes a relative time as parameter, and this time it is labeled so we know what it is.

Also fixed up the unit test so that it prints the expected and actually measured time instead of just printing 'false', which is very useful for debugging what went wrong.

@edwintorok edwintorok mentioned this pull request Nov 28, 2023
@edwintorok edwintorok force-pushed the private/edvint/noselect branch 3 times, most recently from b7902c6 to ff5b1b6 Compare November 28, 2023 17:58
edwintorok and others added 8 commits November 28, 2023 18:02
'make format' with ocamlformat 0.22.4

No functional change.

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Signed-off-by: Steven Woods <steven.woods@citrix.com>
Signed-off-by: Edwin Török <edwin.torok@cloud.com>
`Unix.write` would internally loop until the desired number of bytes are sent,
or `EAGAIN`/`EWOULDBLOCK`/another error is reached.
It cannot check for timeouts because it is not aware that we'd want one.

For pipes and sockets in non-blocking mode this wouldn't be a problem, but this function
is often called with block devices too.
However according to `pselect(3p)` it is a no-op on regular files:
"File descriptors associated with regular files shall always select true for ready to read, ready to write, and error conditions."
And the behaviour on block devices is left unspecified by POSIX, and `select(2)` on Linux doesn't document the behaviour either:
"The pselect() and select() functions shall support regular files, terminal and pseudo‐terminal devices, STREAMS‐based files, FIFOs, pipes, and sockets. The behavior of pselect() and select() on file descriptors that refer to other types of file is unspecified"

Although timeouts for a completely stuck block device cannot be implemented, we can still implement timeouts for a *slow* block device.
Use `Unix.single_{write,read}` instead which gives full control of the iteration to the caller.

The only way to interrupt stuck IO on a block device or regular file would be to use a separate process and `SIGKILL` it after a timeout (this is what `block_device_io` in XAPI does).

These approaches do not work:
* `alarm` or `setitimer` would only interrupt one thread in a multi-threaded program.
* `pthread_kill` can be used to send a signal to a particular thread, but that requires `EINTR` behaviour on syscalls to be enabled
* XAPI expects `SA_RESTART` semantics from syscalls, and would fail an assertion if it ever sees `EINTR` in some paths,
so although the syscall *would* get interrupted, it'd also immediately resume without giving the caller a chance to check for timeouts
* even if it'd work there are no bindings to `pthread_kill` in OCaml

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
'select' has a hardcoded limit of 1024 file descriptors.

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Signed-off-by: Edwin Török <edwin.torok@cloud.com>
It is too easy to misuse Unixext.time_limited_read because that one takesan absolute timestamp
as parameter, not a relative one.

Introduce a new function that takes a relative time as parameter, and doesn't loop.

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Signed-off-by: Edwin Török <edwin.torok@cloud.com>
@psafont
Copy link
Member

psafont commented Nov 29, 2023

I cherry picked the unit test to this PR and it found a bug: time_limited_read takes an absolute time not relative time as timeout (which means that it always timed out instantly because it interpreted the relative timeout as an absolute time in the distant past).

That is very error prone, so I introduced a time_limited_single_read which takes a relative time as parameter, and this time it is labeled so we know what it is.

I'll try to find out some time to add a timer module I've been developing lately to module in the date / time package that can be used for timed functions:

module Timer : sig
  (** This module is useful for knowing that a set amount of time has passed
    after some event happened. For example, to know when pasta is cooked al
    dente. *)
  type t

  type remaining = Spare of Mtime.Span.t | Excess of Mtime.Span.t

  val start : timeout:Mtime.Span.t -> t
  (** [start ~timeout] starts a timer that expires after [timeout] has passed.
      The wait is done in monotonic time, not in POSIX time. *)

  val timeout : t -> Mtime.Span.t
  (** [timeout timer] returns the amount of time after which the timer expires,
      from the moment it was started. *)

  val expired : t -> bool
  (** [expired timer] returns whether [timer] has reached the timeout it was
      set to. *)

  val elapsed : t -> Mtime.Span.t
  (** [elapsed timer] returns the amount of time elapsed since [timer] was
      started. *)

  val remaining : t -> remaining
  (** [remaining timer] returns the amount of spare time is left until it
      expires, or the amount of excess time since it expired. *)

  val deadline_of : t -> float
  (** [deadline_of timer] returns the posix timestamp when the timer expires.
      This is an approximation as the timer doesn't take leap seconds into
      account when waiting. The use of this function is discouraged and it's
      only provided for backwards-compatible reasons. *)
end

The implementation can be found in this branch: psafont/xen-api@c9ebc65

@edwintorok
Copy link
Contributor Author

We'd also need to convert that into whatever form the underlying POSIX API expects, and they may use different clocks.
E.g. 'epoll_wait' (and thus polly) will use monotonic clocks, so that'll match what mtime uses, but XAPI computes a deadline using 'gettimeofday'.

Shouldn't we just merge stdext into XAPI itself? it looks like we might want to refactor some of the APIs, and having them in separate repos like this makes that more difficult (I'd prefer if XAPI used mtime too, but we cannot do that easily without breaking changes, whereas in a monorepo it'd be a lot simpler).

@edwintorok
Copy link
Contributor Author

Continued in xapi-project/xen-api#5402

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.

None yet

4 participants