Add kqueue context #22

pandazxx · 2025-04-21T00:55:29Z

A draft implementation of kqueue context. Not fully tested yet and with a lot of duplicated codes. Compare to poll context:

The read-write event will create seperate events.
One event could, in theory, has multiple outstanding operations.
Canceling one operation should also deregister the coresponding read/write event in case of the operation listen to read-write event.

dietmarkuehl

I had a brief look (I haven't looked at the actual kqueue logic).

include/beman/net/detail/basic_socket_acceptor.hpp

include/beman/net/detail/io_context.hpp

src/beman/net/net.cpp

include/beman/net/detail/kqueue_context.hpp

pandazxx · 2025-04-24T12:03:23Z

Thank you so much for your quick response and guidance. Will update and work on condiction compilation this weekend. Best Regards, Xiangxin Zheng Dietmar Kühl ***@***.***> 于2025年4月24日周四 07:58写道：

…

***@***.**** commented on this pull request. I had a brief look (I haven't looked at the actual kqueue logic). ------------------------------ In include/beman/net/detail/basic_socket_acceptor.hpp <#22 (comment)>: > @@ -4,6 +4,7 @@ #ifndef INCLUDED_BEMAN_NET_DETAIL_BASIC_SOCKET_ACCEPTOR #define INCLUDED_BEMAN_NET_DETAIL_BASIC_SOCKET_ACCEPTOR +#include "beman/net/detail/socket_base.hpp" I think the #include directives should consistently use <file>. ⬇️ Suggested change -#include "beman/net/detail/socket_base.hpp" +#include <beman/net/detail/socket_base.hpp> ------------------------------ In include/beman/net/detail/io_context.hpp <#22 (comment)>: > @@ -6,6 +6,7 @@ // ---------------------------------------------------------------------------- +#include "beman/net/detail/kqueue_context.hpp" ⬇️ Suggested change -#include "beman/net/detail/kqueue_context.hpp" +#include <beman/net/detail/kqueue_context.hpp> ------------------------------ In include/beman/net/detail/io_context.hpp <#22 (comment)>: > @@ -30,7 +31,8 @@ class io_context; class beman::net::io_context { private: ::std::unique_ptr<::beman::net::detail::context_base> d_owned{new ::beman::net::detail::poll_context()}; - ::beman::net::detail::context_base& d_context{*this->d_owned}; + // ::std::unique_ptr<::beman::net::detail::context_base> d_owned{new ::beman::net::detail::kqueue_context()}; I'd think this would ask for conditional compilation: when compiled on a system supporting kqueue the default implementation should use just that. The interesting question may be how to determine if kqueue is support but I think this can be done by detecting this feature in the CMakeLists.txt. I do something like that here <https://github.com/dietmarkuehl/kuhllib/blob/482ddc2b910870398a9a2bcaa0a77a145e081f78/src/toy/CMakeLists.txt#L74> for my "toy" implementation. ------------------------------ In src/beman/net/net.cpp <#22 (comment)>: > @@ -2,6 +2,7 @@ // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception // ---------------------------------------------------------------------------- +#include "beman/net/net.hpp" ⬇️ Suggested change -#include "beman/net/net.hpp" +#include <beman/net/net.hpp> ------------------------------ In include/beman/net/detail/kqueue_context.hpp <#22 (comment)>: > +#include "beman/net/detail/event_type.hpp" +#include "beman/net/detail/io_base.hpp" ⬇️ Suggested change -#include "beman/net/detail/event_type.hpp" -#include "beman/net/detail/io_base.hpp" +#include <beman/net/detail/event_type.hpp> +#include <beman/net/detail/io_base.hpp> ------------------------------ In include/beman/net/detail/kqueue_context.hpp <#22 (comment)>: > +struct kqueue_record; +struct kqueue_context; +} // namespace beman::net::detail + +// ---------------------------------------------------------------------------- + +struct beman::net::detail::kqueue_record final { + kqueue_record(::beman::net::detail::native_handle_type h) : handle(h) {} + ::beman::net::detail::native_handle_type handle; + bool blocking{true}; +}; + +// ---------------------------------------------------------------------------- + +struct beman::net::detail::kqueue_context final : ::beman::net::detail::context_base { + static constexpr size_t event_buffer_size = 10; There isn't a guarantee that these names are made available in the global namespace when including the respective C++ version of the C headers: ⬇️ Suggested change - static constexpr size_t event_buffer_size = 10; + static constexpr ::std::size_t event_buffer_size = 10; ------------------------------ In include/beman/net/detail/kqueue_context.hpp <#22 (comment)>: > + +// ---------------------------------------------------------------------------- + +struct beman::net::detail::kqueue_record final { + kqueue_record(::beman::net::detail::native_handle_type h) : handle(h) {} + ::beman::net::detail::native_handle_type handle; + bool blocking{true}; +}; + +// ---------------------------------------------------------------------------- + +struct beman::net::detail::kqueue_context final : ::beman::net::detail::context_base { + static constexpr size_t event_buffer_size = 10; + using time_t = ::std::chrono::system_clock::time_point; + using timer_node_t = ::beman::net::detail::context_base::resume_at_operation; + using event_key_t = ::std::tuple<uintptr_t, int16_t>; ⬇️ Suggested change - using event_key_t = ::std::tuple<uintptr_t, int16_t>; + using event_key_t = ::std::tuple<::std::uintptr_t, ::std::int16_t>; ------------------------------ In include/beman/net/detail/kqueue_context.hpp <#22 (comment)>: > + constexpr auto to_native_filter(::beman::net::event_type event_type) -> std::span<const int16_t> { + static constexpr std::array<int16_t, 1> read_filter = {EVFILT_READ}; + static constexpr std::array<int16_t, 1> write_filter = {EVFILT_WRITE}; + static constexpr std::array<int16_t, 2> readwrite_filter = {EVFILT_READ, EVFILT_WRITE}; ⬇️ Suggested change - constexpr auto to_native_filter(::beman::net::event_type event_type) -> std::span<const int16_t> { - static constexpr std::array<int16_t, 1> read_filter = {EVFILT_READ}; - static constexpr std::array<int16_t, 1> write_filter = {EVFILT_WRITE}; - static constexpr std::array<int16_t, 2> readwrite_filter = {EVFILT_READ, EVFILT_WRITE}; + constexpr auto to_native_filter(::beman::net::event_type event_type) -> std::span<const ::std::int16_t> { + static constexpr std::array<::std::int16_t, 1> read_filter = {EVFILT_READ}; + static constexpr std::array<::std::int16_t, 1> write_filter = {EVFILT_WRITE}; + static constexpr std::array<::std::int16_t, 2> readwrite_filter = {EVFILT_READ, EVFILT_WRITE}; ------------------------------ In include/beman/net/detail/kqueue_context.hpp <#22 (comment)>: > + return {read_filter}; + case ::beman::net::event_type::out: + return {write_filter}; + case ::beman::net::event_type::in_out: + return {readwrite_filter}; + case ::beman::net::event_type::none: + return {}; + } + } + + ::beman::net::detail::container<::beman::net::detail::kqueue_record> d_sockets; + ::std::map<event_key_t, std::vector<::beman::net::detail::socket_id>> d_event; + ::beman::net::detail::container<::beman::net::detail::io_base*> d_outstanding; + timer_priority_t d_timeouts; + ::beman::net::detail::context_base::task* d_tasks{}; + const int d_queue = kqueue(); // TODO: is this a good practise to put it here? I like to initialise members upon definition. It is more important for class with multiple constructors but I think this is fine. ⬇️ Suggested change - const int d_queue = kqueue(); // TODO: is this a good practise to put it here? + const int d_queue = ::kqueue(); — Reply to this email directly, view it on GitHub <#22 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJDJAZWKQO5YHXJ6INFM5L23ASJLAVCNFSM6AAAAAB3QJKFYOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDOOBYHEYDMNRVG4> . You are receiving this because you authored the thread.Message ID: ***@***.***>

Co-authored-by: Dietmar Kühl <[email protected]>

dietmarkuehl

Sorry for the late response: I was traveling the last two weeks and I didn't manage to look at the review before or during traveling. Also, I haven't, yet, reviewed everything.

The primary issue with the current code is that it uses a map for each operation: while there may be a few allocations needed for the overall operation, e.g., to manage a vector of open file descriptors, these should be really few (I'd think reallocating a vector managed by the context to accommodate more file descriptors should be sufficient). I believe that the kqueue interface actually helps with that by providing a std::uintptr_t which can be populated with user-selected data. Storing a pointer to an operation state into that is the key idea which should make extra storage and various search unnecessary!

It is a while since I implemented my "toy" kqueue interface but I'm pretty sure it gets away without any allocators. I should also refactor the poll implementation over here to factor out the common POSIX interface (as some of your code is roughly another version instead of sharing the logic of processing once work is reported as ready by poll, kqueue, or epoll functions).

dietmarkuehl · 2025-05-05T06:43:10Z

CMakeLists.txt

+    #include <stdint.h>
+    int main() {
+        struct kevent ev{::uintptr_t(), ::int16_t(), EV_DELETE, ::uint32_t(), ::intptr_t(), nullptr, {}};
+        (void)ev;


I wonder if that should use [[maybe_unused]] kevent ev{ … }; instead. The cast to (void) does work but nothing in the standard says that it needs to suppress an unused variable warning. Also, C style casts are somewhat frowned upon.

dietmarkuehl · 2025-05-05T06:43:37Z

CMakeLists.txt

+    #include <sys/event.h>
+    #include <stdint.h>
+    int main() {
+        struct kevent ev{::uintptr_t(), ::int16_t(), EV_DELETE, ::uint32_t(), ::intptr_t(), nullptr, {}};


In C++ the struct keyword isn’t needed.

dietmarkuehl · 2025-05-05T06:46:34Z

CMakeLists.txt

+  check_cxx_source_compiles(
+    "
+    #include <sys/event.h>
+    #include <stdint.h>


Using nullptr the test is strictly C++: I’d use <cstdint> and ::std::-qualify the respective names.

dietmarkuehl · 2025-05-05T06:56:20Z

include/beman/net/detail/container.hpp

 }

+template <typename Record>
+inline auto ::beman::net::detail::container<Record>::find(const Record& r)


This member function can be const.

include/beman/net/detail/io_context.hpp

dietmarkuehl · 2025-05-05T21:37:48Z

include/beman/net/detail/kqueue_context.hpp

+        }
+        return 0u;
+    }
+    auto remove_outstanding(::beman::net::detail::socket_id outstanding_id) {


This code is too complex for me to easily follow (although I will openly admit that I may write similar code - I shouldn't really and where I did it should probably be fixed). It looks like a fairly linear search for a socket which seems odd: the entire point of kqueue is to remove the linear work (in the kernel) needed when using poll(). With the kqueue interface they should also not be needed in user-space.

dietmarkuehl · 2025-05-05T21:38:48Z

include/beman/net/detail/kqueue_context.hpp

+            }
+        }
+    }
+    auto to_milliseconds(auto duration) -> int {


This function can be static (and can possibly also be constexpr and/or noexcept although I don't, yet, care much for the latter).

dietmarkuehl · 2025-05-05T21:43:52Z

include/beman/net/detail/kqueue_context.hpp

+        while (0 < n) {
+            --n;


I think using while (0 < n--) or while (n--) is the idiomatic way to phrase counting down loops. For the last iteration the n-- wraps around which is well-defined behaviour of unsigned integers.

dietmarkuehl · 2025-05-05T21:51:21Z

include/beman/net/detail/kqueue_context.hpp

+            completion->work(*this, completion);
+            ++ncompleted;


This code seems to complete multiple operations where the run_one should really just complete one operation. The way to set this up is probably to have an array of already completed worked which is processed one at the time
for each call at the start of the function (it seems that is set up with process_task/process_timeout calls). Once all known to be ready work is exhausted kevent(...) is called to populate the array with new work (or time out).

dietmarkuehl · 2025-05-05T22:41:00Z

include/beman/net/detail/kqueue_context.hpp

+            auto       filters        = to_native_filter(completion->event);
+            auto       outstanding_id = d_outstanding.insert(completion);
+            for (const auto f : filters) {
+                const event_key_t key{native_handle, f};


I think this is the place where we can do better! First off, there is an important aspect of the work being scheduled: I'm fairly certain that all work is either input or output and never both! Thus, there isn't a case where the completion is used for two potential completions at the same time. There may be outstanding read and write operations for the same file descriptor but they'd have different operation states/completion records.

With that in mind, we can store the a pointer to a io_base in the event_key_t! All relevant information should be accessible vai the io_base:

The file descriptor is accessible via the socket_id.

The direction is accessible via the event_type.

With that I think there isn't any map needed. Also, the key can be reconstructed from the operation state when it is necessary to cancel the work.

Hi Diemar, my understanding is there could be more than one completion listen to the same socket. If so, I think the pointer in kevent struct should point to a linked list. Please correct me if I am wrong.

dietmarkuehl · 2025-05-14T21:19:46Z

I have tried the milano example with kqueue and it seems it terminates the I/O loop while the poll version doesn't. I suspect there is something not quite right (although I'm not, yet, sure in which version).

pandazxx added 2 commits April 6, 2025 12:20

Finish coding of kqueue_context

9609861

Cleaning up

ff45cad

dietmarkuehl reviewed Apr 23, 2025

View reviewed changes

pandazxx and others added 7 commits April 24, 2025 20:03

Update include/beman/net/detail/basic_socket_acceptor.hpp

e47bd38

Co-authored-by: Dietmar Kühl <[email protected]>

Add conditional compilation

830c966

Fix CI error, replace CheckIncludeFile with CheckIncludeFileCXX

f88f85e

Update execution library to latest version

bd2bb46

Fix Clang 15 compile error

9b3bbe2

Move static filters to member variables

2a2bb54

Fix problems from review feedback

448da9e

pandazxx marked this pull request as ready for review May 4, 2025 04:09

pandazxx requested a review from camio as a code owner May 4, 2025 04:09

pandazxx requested review from a team, DeveloperPaul123 and JeffGarland May 4, 2025 04:09

dietmarkuehl requested changes May 5, 2025

View reviewed changes

Add kqueue context #22

Are you sure you want to change the base?

Add kqueue context #22

Uh oh!

Conversation

pandazxx commented Apr 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dietmarkuehl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pandazxx commented Apr 24, 2025 via email

Uh oh!

dietmarkuehl 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!

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!

dietmarkuehl commented May 14, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

pandazxx commented Apr 21, 2025 •

edited

Loading