Skip to content

Conversation

bikallem
Copy link
Contributor

@bikallem bikallem commented Jul 27, 2022

This PR allows specifying clocks for timeout (as timeout is relative to a clock) when waiting for cqe events; specifically Uring.wait now accepts clock argument along with the timeout value in nanoseconds.

Summary:

  • Changes timeout from seconds to nanoseconds (float -> int64)
  • Add Clock_mono, Clock_sys and Clock_default to Uring.wait

Required by ocaml-multicore/eio#249

Copy link
Collaborator

@talex5 talex5 left a comment

Choose a reason for hiding this comment

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

I think the wait API should match the C library. You should be able to add io_uring_prep_timeout as a separate operation, like the other jobs.

@bikallem
Copy link
Contributor Author

bikallem commented Jul 27, 2022

I think the wait API should match the C library. You should be able to add io_uring_prep_timeout as a separate operation, like the other jobs.

Does that mean we need another version of wait_cqe_timeout - for eg. wait_cqe_timeout2. Something like this:

let wait ?clock ?timeout t =
  match timeout with
  | None -> fn_on_ring Uring.wait_cqe t
  | Some timeout -> begin
    assert(timeout >= 0L);
    match clock with 
    | Some c -> fn_on_ring (Uring.wait_cqe_timeout2 c timeout) t
    | None ->  fn_on_ring (Uring.wait_cqe_timeout timeout) t
 end

AFAIK there is no corresponding liburing API which matches what I am attempting to do here, i.e wait cqe with clock source specified.

@talex5
Copy link
Collaborator

talex5 commented Jul 28, 2022

There shouldn't be any need to change anything about wait to support different clocks. Just add a new function for timeouts. Something like:

val timeout : 'a t -> [`Clock_mono |`Clock_sys | `Clock_default] -> int64 -> 'a -> 'a job option

Then users can submit (and cancel) timeout operations just like any other job.

@bikallem
Copy link
Contributor Author

bikallem commented Aug 3, 2022

@talex5 created a new function timeout as suggested.

Copy link
Collaborator

@talex5 talex5 left a comment

Choose a reason for hiding this comment

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

Needs a test-case added to tests/main.md.

@bikallem
Copy link
Contributor Author

bikallem commented Aug 16, 2022

@talex5 Uring.submit on Uring.timeout job returns -62. Is this optimal/ergonomic? Should it perhaps return [ETIME | Int of int]? Or perhaps a helper function val is_timeout : int -> boolto decode the return value fromUring.submit`?

EDIT: Aargh, just realized changing Uring.submit would incur huge backwards incompatibility change on eio, so disregard my above comment on changing Uring.submit signature.

Copy link
Collaborator

@talex5 talex5 left a comment

Choose a reason for hiding this comment

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

Looks good to me, once the flag(s) are added.

@talex5 talex5 merged commit d57bc33 into ocaml-multicore:main Aug 22, 2022
@bikallem bikallem deleted the io_uring_wait_cqe_timeout branch August 22, 2022 15:17
talex5 added a commit to talex5/opam-repository that referenced this pull request Aug 26, 2022
CHANGES:

New features:

- Add `Uring.timeout` (@bikallem ocaml-multicore/ocaml-uring#59).

- Add `Uring.read` and `Uring.write` (@haesbaert ocaml-multicore/ocaml-uring#62).
  These are simple wrappers for read(2) and write(2).

- Add `Uring.unlink` (@talex5 ocaml-multicore/ocaml-uring#65).
  This uses unlinkat(2), and so can also be used to remove directories.

- Add support for uring probes (@talex5 ocaml-multicore/ocaml-uring#70).
  Allows checking whether a feature is supported by the kernel at runtime.

- Rename `peek` to `get_cqe_nonblocking` (@talex5 ocaml-multicore/ocaml-uring#67).
  The old name was confusing because it does remove the item from the ring.

- Update to liburing 2.2 (@talex5 ocaml-multicore/ocaml-uring#56).

- Add `Uring.active_ops` (@talex5 ocaml-multicore/ocaml-uring#68).
  Avoids needing to track the value returned by `submit`, which is important as it is sometimes called automatically.

- Add `Uring.iov_max` constant (@talex5 ocaml-multicore/ocaml-uring#76).

- Add `Uring.get_debug_stats` (@talex5 ocaml-multicore/ocaml-uring#64).
  This should make it easier to check that the uring is behaving as expected.

Performance:

- Introduce a Sketch buffer per Uring (@haesbaert ocaml-multicore/ocaml-uring#63).
  The main motivation of this change is to avoid having one malloc per packet in readv(2), writev(2) and friends.

- Use `submit_and_wait` where appropriate (@haesbaert ocaml-multicore/ocaml-uring#69).

- Add a `readv` benchmark (@talex5 ocaml-multicore/ocaml-uring#64).

- Avoid unnecessary use of `CAMLparam` in the C stubs (@haesbaert ocaml-multicore/ocaml-uring#61).

Bug fixes:

- Prevent ring from being used after exit (@talex5 ocaml-multicore/ocaml-uring#78).

Build changes:

- Remove use of notty for formatting benchmark results (@talex5 ocaml-multicore/ocaml-uring#71).
  It prevented uring from being tested on OCaml 5.

- Use MDX for README (@talex5 ocaml-multicore/ocaml-uring#57).

- Convert tests to MDX (@talex5 ocaml-multicore/ocaml-uring#58 ocaml-multicore/ocaml-uring#73).

- Use opam-repository syntax for license (@kit-ty-kate ocaml-multicore/ocaml-uring#72).

- Remove internal `is_dirty` flag (@talex5 ocaml-multicore/ocaml-uring#77).
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.

3 participants