nit: comment

added

What is the threading model assumed by IoUring? Which methods can be called from any thread and which can only be called from the IoRing thread?

io_uring docs don't state any requirement regarding thread-safety though it probably should. In case of Envoy every IoUring is coupled with one libevent loop (it doesn't run its own) which is rarely posted from threads other than the one running it. So, I assumed that making IoUring thread local and calling its methods only from the thread it lives in simplifies everything. But perhaps it makes more sense to extend Dispatcher to own IoUrings.

It may make sense for the dispatcher to own the io_urings eventually instead of providing a getOrCreate factory method. We can get to that as development continues.

From examples I believe that uring is thread safe. However conceptually there should only one thread emptying the queue. But adding more I/O operations (preparing) into the ring can be done from any thread.

Use of void* in the interface seems a bit too low level. What is the ultimate purpose of this interface? Implement an alternate dispatcher.h implementation? I wonder if use of an user_data interface would improve the usability of the resulting class as it would allow for the removal of the CompletionCb argument to forEveryCompletion

Particularly this interface hides the io_uring syscalls and makes unit testing of the new iohandle (not in this PR) easier a bit. Originally I used a custom type containing the network iohandle instead of *void, but in case of Block IO a different type might be needed. Probably this *void in the interface could be avoided with a template.

I haven't thought of the possibility to implement an alternative Dispatcher, because its interface implies the file readiness model (i.e. Dispatcher::createFileEvent()), whereas io_uring is all about completeness of system operations. CompletionCb is an analog of FileReadyCb, but the former isn't called for a known file descriptor. A system operation and its context (including the arguments) are parts of a completeness event.

I think it's ok to use void* for now, we may revisit this later as our understanding of this module improves.

I'ld like to better understand how io_uring and dispatcher should interact. I could see some threads being exclusively one or the other. I'm not sure about how they would coexist in a thread while avoiding starvation.

I see no problem of multiple io_urings coexisting in one dispatcher: all updates in their completion queues are reflected in their respective event notification file descriptors (man eventfd). So, different callback instances are called for different eventfds, only one registered callback per io_uring. The dispatcher is supposed to call these callbacks in the same manner as in the case of connection callbacks.

I have rewritten the tests to run the callbacks through Dispatcher::run().

Though I fail to come up with a simple enough API for Dispatcher to accommodate multiple io_urings if go this way. If we assume only one io_uring per Dispatcher then extending Dispatcher with something like void Dispatcher::registerCompletionEvent(CompletionCb cb) and IoUring& Dispatcher::getIoUring() would be enough, IoUring::registerEventfd() could be dropped, since all manipulations with eventfd would be incapsulated in Dispatcher.

For the case of multiple io_urings Dispatcher::getIoUring() and Dispatcher::registerCompletionEvent() should distinguish io_urings somehow. Maybe something like the following could work:

using IoUringId = uint32_t;

class DispatcherImpl {
public:
  ...
  IoUringId addIoUring(const IoUringFactory& factory);
  void registerCompletionEvent(IoUringId id, CompletionCb cb);
  IoUring& getIoUring(IoUringId id);
  ...

private:
  std::vector<IoUring> io_urings_;
};

How should the application do that?

added a clarification

I'm not sure I follow this TODO.

The problem is this wrapper doesn't get linked in Envoy's binary. It's not an extension and is not used by the new iohandle currently. If dropped it results in

Invalid build graph structure. deps(//source/...) != deps(//source/exe:envoy_main_common_with_core_extensions_lib) union deps(//source/extensions/...)

in ci/run_envoy_docker.sh 'ci/do_ci.sh deps'. The original PR didn't need this dependency added.

Does IoUringFactoryImpl need to be thread local? It seems to me that it doesn't need to be given the thread local behavior of its getOrCreate accessor.

Thanks! This was a mistake. Fixed.

Use of epoll_wait to decide when to drive the IoUring seems like generic code that most uring users will needed. Should this use of epoll_wait be moved to IoUringImpl ?

Users don't need epoll_wait if they rely on libevent for event handling (via Dispatcher).

If the intended use is together with the dispatcher, perhaps this test should use the dispatcher for event handling and thus serve as an example of how to use this class.

Good point. The test doesn't include sys/epoll.h now, event handlers are run by dispatcher instances.

Please look at the test failures in CI:

[2021-12-28 13:52:10.547][15][critical][assert] [source/common/io/io_uring_impl.cc:34] assert failure: ret == 0. Details: unable to initialize io_uring: Operation not permitted

This is complicated.

The test passes perfectly when run with Azure's managed workers: https://dev.azure.com/dmitryrojkov/meta-helloworld/_build/results?buildId=13&view=logs&j=8c169225-0ae8-53bd-947f-07cb81846cb5&t=6b0ace90-28b2-51d9-2951-b1fd35e67dec
It fails if RBE is active. I suspect that io_uring syscalls are not allow-listed on the workers used as RBEs. Do you happen to know how to reach out to people running those builders?

cc @yanavlasov who may have some thoughts.

From examples I believe that uring is thread safe. However conceptually there should only one thread emptying the queue. But adding more I/O operations (preparing) into the ring can be done from any thread.

Why isn't this in the bootstrap extension hierarchy? Looking below, I think this is what it is. This hierarchy is introduced in https://github.com/envoyproxy/envoy/pull/19467/files, it seems out bootstrap extensions have been adhoc scattered so far :(

This is a bit strange, this is both in core but also an extension?

Uh, right. Originally it was not an extension. It was turned into an extension in a later commit to avoid static thread_local and to use the TLS API instead as suggested.

Now I'm confused. If I keep the code to be an extension io_uring won't be usable in core. Otherwise the TLS API seems to be unavailable. I'd rather drop TLS and bootstrap extension as it was originally.

Now I'm confused, so you're not the only one :) Why can't we use TLS in extensions? Why does this need to be used directly by core?

Why does this need to be used directly by core?

This comment is the main reason.

Why can't we use TLS in extensions?

That was about (un)availability of TLS in core, not extensions. This concern is moot probably. Still I'm not sure why using TLS is more beneficial than static thread_local in this case, because I bumped into other problems with integration testing.

OK, then let's make it core and eliminate the use of the extension stuff. We can use TLS from non-extensions, it's used all over core, e.g. see

I think one of the nice things about using Envoy TLS is you get TLS lifecycle management, which should in theory actually work better for testing etc.

Thanks!

So far my problem with Envoy TLS in testing has been that the thread controlling integration tests doesn't initialize its TLS slot hence lacks io_uring which is needed to open a client connection. I'll experiment with enabling TLS in the testing thread.

The bootstrap extension is dropped now.