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

tests for Unix.select and introduce select-as-epoll #5910

Merged
merged 13 commits into from
Aug 27, 2024

Conversation

edwintorok
Copy link
Contributor

@edwintorok edwintorok commented Aug 5, 2024

First write tests for the semantics of Unix.select.
Then introduce a Unixext.select implemented using epoll, and switch the tests to test this function: now we know that Unixext.select semantics matches Unix.select.
(we don't test error semantics, because XAPI only calls Unix.select with an empty list as the error list).

This doesn't yet switch the code to use Unixext.select, that will be done on feature/perf as it needs more testing.

Draft, needs testing in koji and some ticket numbers.
Also I don't know why the BLK tests haven't failed before on master, maybe they are not being run correctly, they were supposed to run as part of the Quicktests...

@edwintorok
Copy link
Contributor Author

Ah Unixext_test is not part of quicktest.ml, let me fix that.

@edwintorok edwintorok force-pushed the private/edvint/epoll-tests branch 2 times, most recently from 6913a42 to 500f633 Compare August 5, 2024 09:51
@edwintorok
Copy link
Contributor Author

Added, and it passes now:

ulimit -n 2048
./quicktest.exe -skip-xapi -- test unixext
qcheck random seed: 724901538
Testing `Quicktests'.
This run has ID `8CKKL4GO'.

  [SKIP]        Quicktest_example                         0   example test.
  [SKIP]        xenstore                                  0   basic.
  [SKIP]        xenstore                                  1   watch_within_timeout.
  [SKIP]        xenstore                                  2   watch_exceed_timeout.
  [SKIP]        event                                     0   event_next_unblocking_test.
  [SKIP]        event                                     1   event_next_test.
  [SKIP]        event                                     2   event_from_test.
  [SKIP]        event                                     3   event_from_parallel_test.
  [SKIP]        event                                     4   object_level_event_test.
  [SKIP]        event                                     5   event_message_test.
  [SKIP]        event                                     6   event_inject_test.
  [SKIP]        event                                     7   event_from_number_test.
  [SKIP]        import_raw_vdi                            0   import_raw_vdi.
  [SKIP]        Quicktest_date                            0   host.get_server_localtime.
  [SKIP]        Quicktest_date                            1   message.get_since.
  [OK]          unixext                                   0   Unixext_test.test_select.
  [OK]          unixext                                   1   Unixext_test.test_proxy.
  [OK]          unixext                                   2   Unixext_test.test_time_limited_write.
  [OK]          unixext                                   3   Unixext_test.test_time_limited_read.
  [SKIP]        bufio                                     0   Bufio_test.test_buf_io.
  [SKIP]        Timer                                     0   Timer.remaining.

@edwintorok edwintorok mentioned this pull request Aug 5, 2024
@edwintorok edwintorok force-pushed the private/edvint/epoll-tests branch 2 times, most recently from 6354eca to ef8864f Compare August 5, 2024 15:07
@edwintorok edwintorok marked this pull request as ready for review August 5, 2024 15:07
@edwintorok
Copy link
Contributor Author

The tests are now run by 'quicktest' in Dom0 too:

[...] 
  [OK]          unixext                                   0   Unixext_test.te...
  [OK]          unixext                                   1   Unixext_test.te...
  [OK]          unixext                                   2   Unixext_test.te...
  [OK]          unixext                                   3   Unixext_test.te...
  [OK]          bufio                                     0   Bufio_test.test...
  [OK]          Timer                                     0   Timer.remaining.

Also included #5912 to fix the CI

Copy link
Contributor

@Vincent-lau Vincent-lau 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. Most of these are unit tests so should be relatively low risk

Parenthesis were in the wrong place, so when you ran just the non-XAPI tests by hand it worked,
but when running all tests automatically it skipped the qcheck tests.

I'm fairly sure I fixed this bug previously, but must've gotten lost during a rebase.

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
The version we have in Dom0 is too old and doesn't support these flags.
We can only test 512 block size.

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
These only run when root.

However certain combinations are not valid: we cannot insert delays when reading/writing,
because the other side won't get any completion notifications, so we cannot check the usual
way whether time_limited_{read,write} has completed.

Skip BLK tests with delays.

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
The file descriptor gets closed when the channel is closed, so we need to use Unix.dup here
to avoid a double close and avoid an EBADF.

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
And increase ulimit when running quicktest, to cope with open-1024 on startup.

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Like file_kind, but exclude file kinds that the current program cannot generate,
e.g. exclude block devices when not root.

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Generate a list of file descriptors, with nested try/finally closing.

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Generate 3 sets of file descriptors and a timeout.

Try to generate interesting combinations:
* empty lists
* lists with the same file kinds
* lists with mixed file kinds
* lists with common elements
* bias towards generating very short lists

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
Test Unix.select.
This can be easily switched to test Unixext.select based on epoll.

We disable the 'open 1024' fds on startup because we are testing regular select here.

Signed-off-by: Edwin Török <edwin.torok@cloud.com>
A drop-in replacement for select that doesn't fail when a file descriptor number is >1024.

This is bad for performance (it makes a lot more syscalls than select),
and performance sensitive callers should instead use `SO_RCVTIMEO/SO_SNDTIMEO` on sockets,
or move the polly instance creation out of loops.

This will also use one additional file descriptor compared to 'select', so it will reach EMFILE sooner.
When replacing `Unix.select` with `Unixext.select` you must also increase resource limits!

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>
@edwintorok
Copy link
Contributor Author

I've added 2 comments:

@@ -684,6 +684,8 @@ let pollerr_set = Polly.Events.pri

 let to_milliseconds ms = ms *. 1e3 |> ceil |> int_of_float

+(* we could change lists to proper Sets once the Unix.select to Unixext.select conversion is done *)
+
 let readable fd (rd, wr, ex) = (fd :: rd, wr, ex)

 let writable fd (rd, wr, ex) = (rd, fd :: wr, ex)
@@ -723,6 +723,7 @@ let select ins outs errs timeout =
       Unix.sleepf timeout ; no_events
   | _ -> (
       with_polly @@ fun polly ->
+      (* file descriptors that cannot be watched by epoll *)
       let immediate =
         no_events
         |> polly_fold polly pollin_set ins readable

@edwintorok edwintorok merged commit 61ca8fb into xapi-project:master Aug 27, 2024
15 checks passed
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.

4 participants