io: add iohandle backed with io_uring by rojkov · Pull Request #19082 · envoyproxy/envoy

rojkov · 2021-11-23T08:51:38Z

Commit Message: io: add iohandle backed with io_uring

Additional Description:
This commit adds a new iohandle following the so called "completeness" approach to IO: it requests IO operations first and reacts upon their completion whereas the default iohandle waits for readiness events and when ready performs IO syscalls. The former approach allows grouping IO operations in batches and making only one syscall per batch. The performance gain is about 10% bandwidth- and latency-wise.

Risk Level: low (new extension)
Testing: added integration test
Docs Changes: added an .rst page
Release Notes: n/a
Platform Specific Features: io_uring-backed iohandle is available for Linux only
Fixes #17922

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

repokitteh-read-only · 2021-11-23T08:51:41Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #19082 was opened by rojkov.

see: more, trace.

repokitteh-read-only · 2021-11-23T08:51:48Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).
CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).
envoyproxy/dependency-shepherds assignee is @moderation

🐱

Caused by: #19082 was opened by rojkov.

see: more, trace.

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

moderation · 2021-11-23T12:57:55Z

bazel/repository_locations.bzl

+    com_github_axboe_liburing = dict(
+        project_name = "liburing",
+        project_desc = "C helpers to set up and tear down io_uring instances",
+        project_url = "https://github.com/axboe/liburing",
+        version = "2.1",
+        sha256 = "f1e0500cb3934b0b61c5020c3999a973c9c93b618faff1eba75aadc95bb03e07",
+        strip_prefix = "liburing-liburing-{version}",
+        urls = ["https://github.com/axboe/liburing/archive/liburing-{version}.tar.gz"],
+        use_category = ["dataplane_ext", "controlplane"],
+        extensions = [
+            "envoy.io_socket.io_uring",
+        ],
+        release_date = "2021-09-09",
+        cpe = "N/A",
+    ),


Fuzzing and a security policy would help the score
scorecard --repo=https://github.com/axboe/liburing

RESULTS ------- Aggregate score: 5.7 / 10 Check scores: |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------| | SCORE | NAME | REASON | DOCUMENTATION/REMEDIATION | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------| | 10 / 10 | Binary-Artifacts | no binaries found in the repo | https://github.com/ossf/scorecard/blob/main/docs/checks.md#binary-artifacts | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------| | 10 / 10 | Branch-Protection | branch protection is fully | https://github.com/ossf/scorecard/blob/main/docs/checks.md#branch-protection | | | | enabled on development and all | | | | | release branches | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------| | ? | CI-Tests | internal error: cannot list | https://github.com/ossf/scorecard/blob/main/docs/checks.md#ci-tests | | | | check runs by ref | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------| | 0 / 10 | CII-Best-Practices | no badge detected | https://github.com/ossf/scorecard/blob/main/docs/checks.md#cii-best-practices | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------| | 10 / 10 | Code-Review | GitHub code reviews found for | https://github.com/ossf/scorecard/blob/main/docs/checks.md#code-review | | | | 30 commits out of the last 30 | | | | | -- score normalized to 10 | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------| | 10 / 10 | Contributors | 24 different companies found | https://github.com/ossf/scorecard/blob/main/docs/checks.md#contributors | | | | -- score normalized to 10 | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------| | 0 / 10 | Dependency-Update-Tool | no update tool detected | https://github.com/ossf/scorecard/blob/main/docs/checks.md#dependency-update-tool | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------| | 0 / 10 | Fuzzing | project is not fuzzed | https://github.com/ossf/scorecard/blob/main/docs/checks.md#fuzzing | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------| | 10 / 10 | Maintained | 30 commit(s) out of 30 and 30 | https://github.com/ossf/scorecard/blob/main/docs/checks.md#maintained | | | | issue activity out of 30 found | | | | | in the last 90 days -- score | | | | | normalized to 10 | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------| | ? | Packaging | no published package detected | https://github.com/ossf/scorecard/blob/main/docs/checks.md#packaging | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------| | 8 / 10 | Pinned-Dependencies | dependency not pinned by hash | https://github.com/ossf/scorecard/blob/main/docs/checks.md#pinned-dependencies | | | | detected -- score normalized | | | | | to 8 | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------| | 0 / 10 | SAST | no SAST tool detected | https://github.com/ossf/scorecard/blob/main/docs/checks.md#sast | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------| | 0 / 10 | Security-Policy | security policy file not | https://github.com/ossf/scorecard/blob/main/docs/checks.md#security-policy | | | | detected | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------| | ? | Signed-Releases | no releases found | https://github.com/ossf/scorecard/blob/main/docs/checks.md#signed-releases | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------| | 0 / 10 | Token-Permissions | non read-only tokens detected | https://github.com/ossf/scorecard/blob/main/docs/checks.md#token-permissions | | | | in GitHub workflows | | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------| | 10 / 10 | Vulnerabilities | no vulnerabilities detected | https://github.com/ossf/scorecard/blob/main/docs/checks.md#vulnerabilities | |---------|------------------------|--------------------------------|-----------------------------------------------------------------------------------|

Have we considered some way for extensions to declare dependencies?

davinci26 · 2021-11-23T17:20:20Z

looks awesome, adding myself as a reviewer to fully digest the change

davinci26

nice to see this feature implemented, I added some questions and nits

davinci26 · 2021-11-23T19:37:08Z

source/extensions/io_socket/io_uring/config.cc

+
+namespace {
+
+const uint32_t DefaultReadBufferSize = 8192;


nit: this could be constexpr

Updated.

What's the rationale behind constexpr here? Usually I ask to use const for simple constants of primitive types for better readability.

source/extensions/io_socket/io_uring/io_handle_impl.cc

davinci26 · 2021-11-23T19:53:15Z

source/extensions/io_socket/io_uring/io_handle_impl.cc

+
+Api::IoCallUint64Result IoUringSocketHandleImpl::read(Buffer::Instance& buffer,
+                                                      absl::optional<uint64_t> max_length_opt) {
+  const uint64_t max_length = max_length_opt.value_or(UINT64_MAX);


So the idea is that read doesn't call io_uring_factory_.getOrCreateUring().prepareRead() but it just extends the read_buf_ with a new fragment and then when ReadDisable(false) we add all the read requests at once with a single addReadRequest?

By the time read() is called the data has been already read into read_buf_. In this function we just move the data to buffer.

But now I see I need to test that big files are downloaded correctly.

davinci26 · 2021-11-23T20:04:04Z

source/extensions/io_socket/io_uring/io_handle_impl.cc

+                                                  Event::FileReadyCb cb,
+                                                  Event::FileTriggerType trigger, uint32_t events) {
+  // Check if this is a server socket accepting new connections.
+  if (isLeader()) {


under which scenario is isLeader() true in initializeFileEvent?

Is there a case file_event_adapter_ is not a nullptr and the caller wants to initializeFileEvent?

under which scenario is isLeader() true in initializeFileEvent?
Is there a case file_event_adapter_ is not a nullptr and the caller wants to initializeFileEvent?

Only in case of a listener socket. In this case file_event_adapter_ is set in listen() which is called before initializeFileEvent(). For client sockets listen() is never called.

davinci26 · 2021-11-23T20:08:13Z

source/extensions/io_socket/io_uring/io_handle_impl.h

+/**
+ * IoHandle derivative for sockets.
+ */
+class IoUringSocketHandleImpl final : public Network::IoHandle,


nit: you can inherit from the concrete impl of IoSocketHandleImpl if you want to reduce the code

I wanted to avoid inheritance as the type of the relationship doesn't feel like "is-a" to me.

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

rojkov · 2021-11-25T15:27:34Z

Thank you @davinci26 for your help! I'm still struggling with the design.

rojkov · 2021-11-25T15:54:13Z

/retest

repokitteh-read-only · 2021-11-25T15:54:17Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19082 (comment) was created by @rojkov.

see: more, trace.

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

rojkov · 2021-11-26T13:07:16Z

@antoniovicente would you mind having a look? I put your name to CODEOWNERS for now to pass the CI checks.

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

rojkov · 2021-11-29T16:45:05Z

/retest

repokitteh-read-only · 2021-11-29T16:45:11Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19082 (comment) was created by @rojkov.

see: more, trace.

mattklein123 · 2021-11-29T20:10:27Z

Nice this is super cool. A very high level comment: should this be an extension or just be built in? If the kernel supports io_uring and it's faster, I don't know why we wouldn't use it? I'm wondering if instead of an extension this should just be a platform check and Envoy will use it if available? (Obviously we would runtime guard it's use and default to off for initial testing.) cc @envoyproxy/envoy-maintainers for thoughts on this.

SaveTheRbtz · 2021-11-30T03:11:30Z

side note: it would be nice if io_uring interface would be generic enough so that we reuse it for file IO (instead of relying on AIO or a threadpool). For example: disk buffering, caching, or potentially even serving static.

phlax · 2021-11-30T11:48:38Z

/retest

repokitteh-read-only · 2021-11-30T11:48:42Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #19082 (comment) was created by @phlax.

see: more, trace.

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

Envoy runs OK. But... The integration test is broken now, because the client connection created in the test_thread wants to use IoUring too, but its TLS is empty as test_thread is not registered with TLS. Can it be registered? Wouldn't it be too clumsy? Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

wrowe · 2022-02-24T21:07:55Z

Note current discussion of dependency at #19339 (comment)

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

github-actions · 2022-04-23T16:01:16Z

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

github-actions · 2022-05-02T16:01:51Z

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

soulxu · 2022-05-11T09:48:21Z

Me @soulxu and @zhxie are trying to take this over since @rojkov can't work on this anymore.

io: add iohandle backed with io_uring

a9ca9f5

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

repokitteh-read-only bot added api deps Approval required for changes to Envoy's external dependencies labels Nov 23, 2021

repokitteh-read-only bot assigned mattklein123 and moderation Nov 23, 2021

update dependency

5ccddde

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

rojkov mentioned this pull request Nov 23, 2021

Migrate event loops from file events to buffer events #17922

Open

add metadata to the extension

bf57019

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

moderation reviewed Nov 23, 2021

View reviewed changes

davinci26 self-assigned this Nov 23, 2021

davinci26 reviewed Nov 23, 2021

View reviewed changes

rojkov added 3 commits November 25, 2021 13:41

close valid fd in destructor

aa6158d

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

use constexpr

4d1d16b

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

move Linux specific code to io_uring_impl.cc

bb8cce1

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

Merge remote-tracking branch 'upstream/main' into io-uring

ff5494e

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

Merge remote-tracking branch 'upstream/main' into io-uring

a4e7d33

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

rojkov mentioned this pull request Nov 30, 2021

All PRs with dependency updates fail in CI #19136

Closed

rojkov added 6 commits February 11, 2022 10:46

use Envoy::ThreadLocal API

9d365c7

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

sync with io-uring-wrapper branch

25f1924

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

adapt iohandle to the new IoUring

5ffae83

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

fix address sanitizer CI check

7dfad5b

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

Merge remote-tracking branch 'upstream/main' into io-uring-v2

0ffeeee

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

repokitteh-read-only bot removed the waiting:any label Feb 11, 2022

mattklein123 added the waiting label Feb 11, 2022

wrowe requested a review from htuch February 16, 2022 15:07

rojkov added 2 commits March 23, 2022 11:16

drop bootstrap extension

0aabb0e

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

adapt to new IoUringFactory which isn't extension

2e363f7

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

repokitteh-read-only bot removed the waiting label Mar 23, 2022

rojkov added the waiting label Mar 23, 2022

rojkov added 3 commits March 23, 2022 14:06

use ThreadLocal::TypedSlot

e6cea15

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

Merge remote-tracking branch 'upstream/main' into io-uring-v2

e0a1ee0

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

fix build

4aef5c9

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

repokitteh-read-only bot removed the waiting label Mar 24, 2022

rojkov added the waiting label Mar 24, 2022

rojkov mentioned this pull request Mar 24, 2022

lazy read disable for the http1 codec #20148

Merged

Merge remote-tracking branch 'upstream/main' into io-uring

e86974a

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label Apr 23, 2022

Merge remote-tracking branch 'upstream/main' into io-uring

c8233aa

Signed-off-by: Dmitry Rozhkov <dmitry.rozhkov@intel.com>

repokitteh-read-only bot removed the waiting label Apr 25, 2022

rojkov added the waiting label Apr 25, 2022

github-actions bot closed this May 2, 2022

soulxu mentioned this pull request Jul 26, 2022

[PoC] io: add iouring support #22392

Closed

Conversation

rojkov commented Nov 23, 2021

Uh oh!

repokitteh-read-only bot commented Nov 23, 2021

Uh oh!

repokitteh-read-only bot commented Nov 23, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davinci26 commented Nov 23, 2021

Uh oh!

davinci26 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rojkov commented Nov 25, 2021

Uh oh!

rojkov commented Nov 25, 2021

Uh oh!

repokitteh-read-only bot commented Nov 25, 2021

Uh oh!

rojkov commented Nov 26, 2021

Uh oh!

rojkov commented Nov 29, 2021

Uh oh!

repokitteh-read-only bot commented Nov 29, 2021

Uh oh!

mattklein123 commented Nov 29, 2021

Uh oh!

SaveTheRbtz commented Nov 30, 2021

Uh oh!

phlax commented Nov 30, 2021

Uh oh!

repokitteh-read-only bot commented Nov 30, 2021

Uh oh!

wrowe commented Feb 24, 2022

Uh oh!

github-actions bot commented Apr 23, 2022

Uh oh!

github-actions bot commented May 2, 2022

Uh oh!

soulxu commented May 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants