Re: [PR] Support splice in http blind tunnel [trafficserver]

["moonchen (via GitHub)" <gi...@apache.org>]

moonchen commented on code in PR #11890:
URL: https://github.com/apache/trafficserver/pull/11890#discussion_r1925533339


##########
src/iocore/eventsystem/P_IOBuffer.h:
##########
@@ -656,6 +656,15 @@ new_MIOBuffer_internal(const char *location, int64_t size_index)
 TS_INLINE void
 free_MIOBuffer(MIOBuffer *mio)
 {
+#if TS_USE_LINUX_SPLICE
+  // check if mio is PipeIOBuffer using dynamic_cast
+  PipeIOBuffer *pipe_mio = dynamic_cast<PipeIOBuffer *>(mio);

Review Comment:
   Relying on a runtime type check with dynamic_cast to select the proper deallocation mechanism is generally considered an anti-pattern.  Instead, ClassAllocator has a `Destruct_on_free_` parameter that allows you to use a virtual destructor to clean up properly depending on the underlying type.



##########
src/proxy/http/HttpSM.cc:
##########
@@ -7274,6 +7274,32 @@ HttpSM::setup_blind_tunnel(bool send_response_hdr, IOBufferReader *initial)
   //  header buffer into new buffer
   client_request_body_bytes += from_ua_buf->write(_ua.get_txn()->get_remote_reader());
 
+#if TS_USE_LINUX_SPLICE
+  MIOBuffer *from_ua_pipe_buf = new_PipeIOBuffer(BUFFER_SIZE_INDEX_32K);

Review Comment:
   `BUFFER_SIZE_INDEX_32K` is equal to 8.  Is this the capacity of the pipe that we're requesting from the kernel?



##########
src/iocore/net/UnixNetVConnection.cc:
##########
@@ -508,6 +508,81 @@ UnixNetVConnection::net_read_io(NetHandler *nh)
     read_disable(nh, this);
     return;
   }
+#if TS_USE_LINUX_SPLICE
+  // Check if the buffer is a PipeIOBuffer
+  PipeIOBuffer *pipe_buffer = dynamic_cast<PipeIOBuffer *>(buf.writer());
+  if (pipe_buffer) {
+    // Use splice_to to transfer data from socket directly to pipe with SPLICE_F_MORE hint
+    int64_t to_splice = std::min(ntodo, pipe_buffer->write_avail());
+    if (to_splice > 0) {
+      r = con.sock.splice_to(pipe_buffer->fd[1], to_splice, SPLICE_F_MOVE | SPLICE_F_NONBLOCK);
+
+      if (r <= 0) {
+        // Temporary Unavailable, Non-Blocking I/O
+        if (r == -EAGAIN || r == -ENOTCONN) {

Review Comment:
   Is it possible that we get EAGAIN here because the pipe is at capacity?  How do we handle that case?



##########
include/iocore/eventsystem/IOBuffer.h:
##########
@@ -1508,3 +1512,116 @@ IOBufferChain::iterator::operator->() const
 {
   return _b;
 }
+
+#if TS_USE_LINUX_SPLICE
+
+class PipeIOBufferReader : public IOBufferReader

Review Comment:
   Managing header dependency in ATS (specifically iocore) has been an ongoing challenge.  One of the reasons is that we often combine multiple classes into a single header file.  Please move the new classes to their own header and source files.



##########
src/iocore/net/UnixNetVConnection.cc:
##########
@@ -508,6 +508,81 @@ UnixNetVConnection::net_read_io(NetHandler *nh)
     read_disable(nh, this);
     return;
   }
+#if TS_USE_LINUX_SPLICE

Review Comment:
   This function is getting quite long, with two independent code paths to read from socket to buffer.  I would prefer using polymorphism to handle the different socket-to-buffer copies, but at least we should split this function up so that it's more readable. 



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org