From 1723f3f2d683d3d7121e02d47f67d36e3edff56b Mon Sep 17 00:00:00 2001 From: Xie Zhihao Date: Wed, 19 Oct 2022 14:28:18 +0000 Subject: [PATCH 1/4] network: cancel before close Signed-off-by: Xie Zhihao --- source/common/io/io_uring.h | 7 ++++ source/common/io/io_uring_impl.cc | 11 ++++++ source/common/io/io_uring_impl.h | 1 + .../network/io_uring_socket_handle_impl.cc | 34 +++++++++++++------ .../network/io_uring_socket_handle_impl.h | 4 +-- 5 files changed, 45 insertions(+), 12 deletions(-) diff --git a/source/common/io/io_uring.h b/source/common/io/io_uring.h index 26ef78ea9173f..348f99db76f25 100644 --- a/source/common/io/io_uring.h +++ b/source/common/io/io_uring.h @@ -87,6 +87,13 @@ class IoUring { */ virtual IoUringResult prepareClose(os_fd_t fd, void* user_data) PURE; + /** + * Prepares a cancellation and puts it into the submission queue. + * Returns IoUringResult::Failed in case the submission queue is full already + * and IoUringResult::Ok otherwise. + */ + virtual IoUringResult prepareCancel(void* cancelling_user_data, void* user_data) PURE; + /** * Submits the entries in the submission queue to the kernel using the * `io_uring_enter()` system call. diff --git a/source/common/io/io_uring_impl.cc b/source/common/io/io_uring_impl.cc index 97258e524cf4f..14e3122dcc362 100644 --- a/source/common/io/io_uring_impl.cc +++ b/source/common/io/io_uring_impl.cc @@ -143,6 +143,17 @@ IoUringResult IoUringImpl::prepareClose(os_fd_t fd, void* user_data) { return IoUringResult::Ok; } +IoUringResult IoUringImpl::prepareCancel(void* cancelling_user_data, void* user_data) { + struct io_uring_sqe* sqe = io_uring_get_sqe(&ring_); + if (sqe == nullptr) { + return IoUringResult::Failed; + } + + io_uring_prep_cancel(sqe, cancelling_user_data, 0); + io_uring_sqe_set_data(sqe, user_data); + return IoUringResult::Ok; +} + IoUringResult IoUringImpl::submit() { int res = io_uring_submit(&ring_); RELEASE_ASSERT(res >= 0 || res == -EBUSY, "unable to submit io_uring queue entries"); diff --git a/source/common/io/io_uring_impl.h b/source/common/io/io_uring_impl.h index d97b5031e8aa3..176080d551cfe 100644 --- a/source/common/io/io_uring_impl.h +++ b/source/common/io/io_uring_impl.h @@ -29,6 +29,7 @@ class IoUringImpl : public IoUring, public ThreadLocal::ThreadLocalObject { IoUringResult prepareWritev(os_fd_t fd, const struct iovec* iovecs, unsigned nr_vecs, off_t offset, void* user_data) override; IoUringResult prepareClose(os_fd_t fd, void* user_data) override; + IoUringResult prepareCancel(void* cancelling_user_data, void* user_data) override; IoUringResult submit() override; private: diff --git a/source/common/network/io_uring_socket_handle_impl.cc b/source/common/network/io_uring_socket_handle_impl.cc index 38d945cedbdec..fe307a3b8731a 100644 --- a/source/common/network/io_uring_socket_handle_impl.cc +++ b/source/common/network/io_uring_socket_handle_impl.cc @@ -38,15 +38,28 @@ IoUringSocketHandleImpl::~IoUringSocketHandleImpl() { Api::IoCallUint64Result IoUringSocketHandleImpl::close() { ASSERT(SOCKET_VALID(fd_)); + auto& uring = io_uring_factory_.get().ref(); + if (read_req_) { + auto req = new Request{*this, RequestType::Cancel}; + auto res = uring.prepareCancel(read_req_, req); + if (res == Io::IoUringResult::Failed) { + // TODO(rojkov): handle `EBUSY` in case the completion queue is never reaped. + uring.submit(); + res = uring.prepareCancel(read_req_, req); + RELEASE_ASSERT(res == Io::IoUringResult::Ok, "unable to prepare cancel"); + } + uring.submit(); + } + auto req = new Request{absl::nullopt, RequestType::Close}; - Io::IoUringResult res = io_uring_factory_.get().ref().prepareClose(fd_, req); + auto res = uring.prepareClose(fd_, req); if (res == Io::IoUringResult::Failed) { // Fall back to posix system call. ::close(fd_); } if (isLeader()) { - if (io_uring_factory_.get().ref().isEventfdRegistered()) { - io_uring_factory_.get().ref().unregisterEventfd(); + if (uring.isEventfdRegistered()) { + uring.unregisterEventfd(); } file_event_adapter_.reset(); } @@ -73,7 +86,7 @@ Api::IoCallUint64Result IoUringSocketHandleImpl::readv(uint64_t /* max_length */ num_bytes_to_read += slice_length; } ASSERT(num_bytes_to_read <= static_cast(bytes_to_read_)); - is_read_added_ = false; + read_req_ = nullptr; uint64_t len = bytes_to_read_; bytes_to_read_ = 0; @@ -103,7 +116,7 @@ Api::IoCallUint64Result IoUringSocketHandleImpl::read(Buffer::Instance& buffer, delete this_fragment; }); buffer.addBufferFragment(*fragment); - is_read_added_ = false; + read_req_ = nullptr; uint64_t len = bytes_to_read_; bytes_to_read_ = 0; @@ -312,22 +325,21 @@ void IoUringSocketHandleImpl::resetFileEvents() { file_event_adapter_.reset(); } Api::SysCallIntResult IoUringSocketHandleImpl::shutdown(int /*how*/) { PANIC("not implemented"); } void IoUringSocketHandleImpl::addReadRequest() { - if (!is_read_enabled_ || !SOCKET_VALID(fd_) || is_read_added_) { + if (!is_read_enabled_ || !SOCKET_VALID(fd_) || read_req_) { return; } ASSERT(read_buf_ == nullptr); - is_read_added_ = true; // don't add READ if it's been already added. read_buf_ = std::unique_ptr(new uint8_t[read_buffer_size_]); iov_.iov_base = read_buf_.get(); iov_.iov_len = read_buffer_size_; auto& uring = io_uring_factory_.get().ref(); - auto req = new Request{*this, RequestType::Read}; - auto res = uring.prepareReadv(fd_, &iov_, 1, 0, req); + read_req_ = new Request{*this, RequestType::Read}; + auto res = uring.prepareReadv(fd_, &iov_, 1, 0, read_req_); if (res == Io::IoUringResult::Failed) { // TODO(rojkov): handle `EBUSY` in case the completion queue is never reaped. uring.submit(); - res = uring.prepareReadv(fd_, &iov_, 1, 0, req); + res = uring.prepareReadv(fd_, &iov_, 1, 0, read_req_); RELEASE_ASSERT(res == Io::IoUringResult::Ok, "unable to prepare readv"); } } @@ -524,6 +536,8 @@ void IoUringSocketHandleImpl::FileEventAdapter::onRequestCompletion(const Reques } case RequestType::Close: break; + case RequestType::Cancel: + break; default: PANIC("not implemented"); } diff --git a/source/common/network/io_uring_socket_handle_impl.h b/source/common/network/io_uring_socket_handle_impl.h index 71e6a57e434c8..fb3995fe81f7a 100644 --- a/source/common/network/io_uring_socket_handle_impl.h +++ b/source/common/network/io_uring_socket_handle_impl.h @@ -12,7 +12,7 @@ namespace Network { class IoUringSocketHandleImpl; -enum class RequestType { Accept, Connect, Read, Write, Close, Unknown }; +enum class RequestType { Accept, Connect, Read, Write, Close, Cancel, Unknown }; using IoUringSocketHandleImplOptRef = absl::optional>; @@ -126,7 +126,7 @@ class IoUringSocketHandleImpl final : public IoHandle, protected Logger::Loggabl struct iovec iov_; std::unique_ptr read_buf_{nullptr}; int32_t bytes_to_read_{0}; - bool is_read_added_{false}; + Request* read_req_{nullptr}; bool is_read_enabled_{true}; std::list write_buf_{}; uint32_t vecs_to_write_{0}; From f7045b17c770074716c0adc0fba946506942208b Mon Sep 17 00:00:00 2001 From: Xie Zhihao Date: Wed, 19 Oct 2022 16:15:06 +0000 Subject: [PATCH 2/4] network: fix missing submitting close request Signed-off-by: Xie Zhihao --- source/common/network/io_uring_socket_handle_impl.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/common/network/io_uring_socket_handle_impl.cc b/source/common/network/io_uring_socket_handle_impl.cc index fe307a3b8731a..efbea2f8e7974 100644 --- a/source/common/network/io_uring_socket_handle_impl.cc +++ b/source/common/network/io_uring_socket_handle_impl.cc @@ -48,7 +48,6 @@ Api::IoCallUint64Result IoUringSocketHandleImpl::close() { res = uring.prepareCancel(read_req_, req); RELEASE_ASSERT(res == Io::IoUringResult::Ok, "unable to prepare cancel"); } - uring.submit(); } auto req = new Request{absl::nullopt, RequestType::Close}; @@ -57,6 +56,7 @@ Api::IoCallUint64Result IoUringSocketHandleImpl::close() { // Fall back to posix system call. ::close(fd_); } + uring.submit(); if (isLeader()) { if (uring.isEventfdRegistered()) { uring.unregisterEventfd(); From 0d1772398210ee3842fa312593a2a8d7e88207fb Mon Sep 17 00:00:00 2001 From: Xie Zhihao Date: Thu, 20 Oct 2022 09:34:15 +0000 Subject: [PATCH 3/4] network: ignore callback on canceled requests Signed-off-by: Xie Zhihao --- source/common/network/io_uring_socket_handle_impl.cc | 3 +++ 1 file changed, 3 insertions(+) diff --git a/source/common/network/io_uring_socket_handle_impl.cc b/source/common/network/io_uring_socket_handle_impl.cc index efbea2f8e7974..31bc873210fa7 100644 --- a/source/common/network/io_uring_socket_handle_impl.cc +++ b/source/common/network/io_uring_socket_handle_impl.cc @@ -490,6 +490,9 @@ IoHandlePtr IoUringSocketHandleImpl::FileEventAdapter::accept(struct sockaddr* a void IoUringSocketHandleImpl::FileEventAdapter::onRequestCompletion(const Request& req, int32_t result) { if (result < 0) { + if (result == -ECANCELED) { + return; + } ENVOY_LOG(debug, "async request failed: {}", errorDetails(-result)); } From c6e398ada4ab0049575a235e014de6b72b386cb6 Mon Sep 17 00:00:00 2001 From: Xie Zhihao Date: Thu, 20 Oct 2022 10:34:25 +0000 Subject: [PATCH 4/4] network: minor optimize Signed-off-by: Xie Zhihao --- source/common/network/io_uring_socket_handle_impl.cc | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/source/common/network/io_uring_socket_handle_impl.cc b/source/common/network/io_uring_socket_handle_impl.cc index 31bc873210fa7..3543ab0712dff 100644 --- a/source/common/network/io_uring_socket_handle_impl.cc +++ b/source/common/network/io_uring_socket_handle_impl.cc @@ -490,9 +490,6 @@ IoHandlePtr IoUringSocketHandleImpl::FileEventAdapter::accept(struct sockaddr* a void IoUringSocketHandleImpl::FileEventAdapter::onRequestCompletion(const Request& req, int32_t result) { if (result < 0) { - if (result == -ECANCELED) { - return; - } ENVOY_LOG(debug, "async request failed: {}", errorDetails(-result)); } @@ -507,6 +504,10 @@ void IoUringSocketHandleImpl::FileEventAdapter::onRequestCompletion(const Reques break; case RequestType::Read: { ASSERT(req.iohandle_.has_value()); + // Read is cancellable. + if (result == -ECANCELED) { + return; + } auto& iohandle = req.iohandle_->get(); iohandle.bytes_to_read_ = result; // This is hacky fix, we should check the req is valid or not.