From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by passt.top (Postfix) with ESMTP id 6299B5A004F for ; Fri, 12 Jul 2024 16:20:22 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1720794021; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=3oao+xWy49P1HAJvwrIe94qcac/SF5aTZVqMi0zz278=; b=WxevKKdIPvAdccXlGiB0XsBkYc1QIMDCbzcyf2uQQCfhc4YJD+dDfUB02Gp7V2kVzRrQrT kNzgeTqHgaeefvLYOv2e1+DBpGYoeP8yBLzyjWRHYGu+0lT9VCLS19H9RR0GWYnM9N0bMR TwLKT99TpbMpytr72yDbsb2XASIZ6H0= Received: from mail-qt1-f197.google.com (mail-qt1-f197.google.com [209.85.160.197]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-627-9jyO5FlWOISA4oU0Ccw_hQ-1; Fri, 12 Jul 2024 10:20:20 -0400 X-MC-Unique: 9jyO5FlWOISA4oU0Ccw_hQ-1 Received: by mail-qt1-f197.google.com with SMTP id d75a77b69052e-44dbd6ef5edso21531391cf.0 for ; Fri, 12 Jul 2024 07:20:20 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1720794018; x=1721398818; h=content-transfer-encoding:mime-version:organization:references :in-reply-to:message-id:subject:cc:to:from:date:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=3oao+xWy49P1HAJvwrIe94qcac/SF5aTZVqMi0zz278=; b=YimSWcwto2HOVjl97ILZuxarQa7xfsdzmLwaR4ginBJBFQb/fi8fwxDFSxelfFABsB ei6jht+tI+1vTI3ZbYjFOa+pI6XK3wbbPQNyhxSkM0UB5jF1rBIJwOEVWOT130aazYML Wpt/J4AXKQLFsgOapOx9dXY23uirEyUpAaGtrfLjAY0+qpf3Omibfff73BzvvTlLq6g7 /mZhmXg8kM8WAGJ8V23JfivER+ge3G0nKx14b/CQp9+Otq5D1kYfAXg4V2Byp4BEJIzQ e5IqIb5DlkiEJZ5nATG4J7/yPQrzvF04FGcxpWHMhT8mliQumDkzIyOK2qQ7eJajpkm/ 7vcw== X-Gm-Message-State: AOJu0Yz0uArOtDS0O+zP4tLuVY5SLIVh8J6ml+MUKg/6Hc8kSH61TK/1 w8xj98dGm4mcpdCfe1XDD3bxpLZG7oPBT0WBpZ9IDohhrDatb+pviOJayn42JCsmQxSTvaTlr9K qtTlJ69MgMzfMqIjdGMakOkfyOMwGvtBWdP2hnXwX18novpYvRwSQbFwJyRJoVQvS8OkT4sUZ52 fgF58Z806H2zCdj7u+NxBnwcyfjNMDRP98DVc= X-Received: by 2002:a05:622a:2ca:b0:446:51f3:1867 with SMTP id d75a77b69052e-447fa9083d3mr126863691cf.47.1720794018483; Fri, 12 Jul 2024 07:20:18 -0700 (PDT) X-Google-Smtp-Source: AGHT+IG/mF7fejlf0nb9ezYOmRiM6/vfEYH563ZbKjLeCvykQ+G+NtAz3YRo4R5h4utr9wYM14UkQg== X-Received: by 2002:a05:622a:2ca:b0:446:51f3:1867 with SMTP id d75a77b69052e-447fa9083d3mr126863371cf.47.1720794017986; Fri, 12 Jul 2024 07:20:17 -0700 (PDT) Received: from maya.cloud.tilaa.com (maya.cloud.tilaa.com. [164.138.29.33]) by smtp.gmail.com with ESMTPSA id d75a77b69052e-447fef6c996sm37302701cf.46.2024.07.12.07.20.17 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Fri, 12 Jul 2024 07:20:17 -0700 (PDT) Date: Fri, 12 Jul 2024 16:19:42 +0200 From: Stefano Brivio To: Jon Maloy Subject: Re: [PATCH v8 1/2] tcp: leverage support of SO_PEEK_OFF socket option when available Message-ID: <20240712161942.291c9efe@elisabeth> In-Reply-To: <20240711222631.1073408-2-jmaloy@redhat.com> References: <20240711222631.1073408-1-jmaloy@redhat.com> <20240711222631.1073408-2-jmaloy@redhat.com> Organization: Red Hat X-Mailer: Claws Mail 4.2.0 (GTK 3.24.41; x86_64-pc-linux-gnu) MIME-Version: 1.0 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Message-ID-Hash: RF4EQX2EBZ2MVU5WN4DTBLWMDO4LYDYW X-Message-ID-Hash: RF4EQX2EBZ2MVU5WN4DTBLWMDO4LYDYW X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, lvivier@redhat.com, dgibson@redhat.com X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Thu, 11 Jul 2024 18:26:30 -0400 Jon Maloy wrote: > From linux-6.9.0 the kernel will contain > commit 05ea491641d3 ("tcp: add support for SO_PEEK_OFF socket option"). > > This new feature makes is possible to call recv_msg(MSG_PEEK) and make > it start reading data from a given offset set by the SO_PEEK_OFF socket > option. This way, we can avoid repeated reading of already read bytes of > a received message, hence saving read cycles when forwarding TCP > messages in the host->name space direction. > > In this commit, we add functionality to leverage this feature when > available, while we fall back to the previous behavior when not. > > Measurements with iperf3 shows that throughput increases with 15-20 > percent in the host->namespace direction when this feature is used. > > Reviewed-by: David Gibson > Signed-off-by: Jon Maloy > > --- > v2: - Some smaller changes as suggested by David Gibson and Stefano Brivio. > - Moved initial set_peek_offset(0) to only the locations where the socket is set > to ESTABLISHED. > - Removed the per-packet synchronization between sk_peek_off and > already_sent. Instead only doing it in retransmit situations. > - The problem I found when trouble shooting the occasionally occurring > out of synch values between 'already_sent' and 'sk_peek_offset' may > have deeper implications that we may need to be investigate. > > v3: - Rebased to most recent version of tcp.c, plus the previous > patch in this series. > - Some changes based on feedback from PASST team > > v4: - Some small changes based on feedback from Stefan/David. > > v5: - Re-added accidentally dropped set_peek_offset() line. > Thank you, David. > > v6: - Adapted to new file structure. No code changes. > --- > tcp.c | 35 ++++++++++++++++++++++++++++++++++- > tcp.h | 3 +++ > tcp_buf.c | 22 ++++++++++++++++------ > 3 files changed, 53 insertions(+), 7 deletions(-) > > diff --git a/tcp.c b/tcp.c > index 698e7ec..1a8a8df 100644 > --- a/tcp.c > +++ b/tcp.c > @@ -373,6 +373,9 @@ static union inany_addr low_rtt_dst[LOW_RTT_TABLE_SIZE]; > > char tcp_buf_discard [MAX_WINDOW]; > > +/* Does the kernel support TCP_PEEK_OFF? */ > +bool peek_offset_cap; > + > /* sendmsg() to socket */ > static struct iovec tcp_iov [UIO_MAXIOV]; > > @@ -388,6 +391,20 @@ static_assert(ARRAY_SIZE(tc_hash) >= FLOW_MAX, > int init_sock_pool4 [TCP_SOCK_POOL_SIZE]; > int init_sock_pool6 [TCP_SOCK_POOL_SIZE]; > > +/** > + * tcp_set_peek_offset() - Set SO_PEEK_OFF offset on a socket if supported > + * @s: Socket to update > + * @offset: Offset in bytes > + */ > +void tcp_set_peek_offset(int s, int offset) > +{ > + if (!peek_offset_cap) > + return; > + > + if (setsockopt(s, SOL_SOCKET, SO_PEEK_OFF, &offset, sizeof(offset))) > + err("Failed to set SO_PEEK_OFF to %i in socket %i", offset, s); The whole patch looks good to me (minus one nit below), but note that David was suggesting to reset the connection in case of an error here (which makes sense to me as well): https://archives.passt.top/passt-dev/Zlkt3tXNId5A-xs1@zatzit/ ...you could either pass 'c' and 'conn' to this function (see for example tcp_connect_finish()), or return error and call tcp_rst() in the caller. Calling tcp_rst() directly from here might save some lines, but on the other hand you can't keep using the connection in the caller after you reset it, so you would need some if clauses in callers anyway... so at that point you can just take care of the whole thing in callers I guess. > +} > + > /** > * tcp_conn_epoll_events() - epoll events mask for given connection state > * @events: Current connection events > @@ -1948,6 +1965,7 @@ static int tcp_data_from_tap(struct ctx *c, struct tcp_tap_conn *conn, > "fast re-transmit, ACK: %u, previous sequence: %u", > max_ack_seq, conn->seq_to_tap); > conn->seq_to_tap = max_ack_seq; > + tcp_set_peek_offset(conn->sock, 0); > tcp_data_from_sock(c, conn); > } > > @@ -2040,6 +2058,7 @@ static void tcp_conn_from_sock_finish(struct ctx *c, struct tcp_tap_conn *conn, > conn->seq_ack_to_tap = conn->seq_from_tap; > > conn_event(c, conn, ESTABLISHED); > + tcp_set_peek_offset(conn->sock, 0); > > /* The client might have sent data already, which we didn't > * dequeue waiting for SYN,ACK from tap -- check now. > @@ -2120,6 +2139,7 @@ int tcp_tap_handler(struct ctx *c, uint8_t pif, sa_family_t af, > goto reset; > > conn_event(c, conn, ESTABLISHED); > + tcp_set_peek_offset(conn->sock, 0); > > if (th->fin) { > conn->seq_from_tap++; > @@ -2368,6 +2388,7 @@ void tcp_timer_handler(struct ctx *c, union epoll_ref ref) > flow_dbg(conn, "ACK timeout, retry"); > conn->retrans++; > conn->seq_to_tap = conn->seq_ack_from_tap; > + tcp_set_peek_offset(conn->sock, 0); > tcp_data_from_sock(c, conn); > tcp_timer_ctl(c, conn); > } > @@ -2660,7 +2681,8 @@ static void tcp_sock_refill_init(const struct ctx *c) > */ > int tcp_init(struct ctx *c) > { > - unsigned b; > + unsigned int b, optv = 0; > + int s; > > for (b = 0; b < TCP_HASH_TABLE_SIZE; b++) > tc_hash[b] = FLOW_SIDX_NONE; > @@ -2684,6 +2706,17 @@ int tcp_init(struct ctx *c) > NS_CALL(tcp_ns_socks_init, c); > } > > + /* Probe for SO_PEEK_OFF support */ > + s = socket(AF_INET, SOCK_STREAM | SOCK_CLOEXEC, IPPROTO_TCP); > + if (s < 0) { > + warn("Temporary TCP socket creation failed"); I recently added _perror variants that would print perror-like messages, so you can effortlessly do: warn_perror("Temporary TCP socket creation failed"); and the reason why it failed (strerror(errno)) is also printed. > + } else { > + if (!setsockopt(s, SOL_SOCKET, SO_PEEK_OFF, &optv, sizeof(int))) > + peek_offset_cap = true; > + close(s); > + } > + info("SO_PEEK_OFF%ssupported", peek_offset_cap ? " " : " not "); > + > return 0; > } > > diff --git a/tcp.h b/tcp.h > index a9b8bf8..13c108d 100644 > --- a/tcp.h > +++ b/tcp.h > @@ -24,6 +24,9 @@ void tcp_timer(struct ctx *c, const struct timespec *now); > void tcp_defer_handler(struct ctx *c); > > void tcp_update_l2_buf(const unsigned char *eth_d, const unsigned char *eth_s); > +void tcp_set_peek_offset(int s, int offset); > + > +extern bool peek_offset_cap; > > /** > * union tcp_epoll_ref - epoll reference portion for TCP connections > diff --git a/tcp_buf.c b/tcp_buf.c > index ff9f797..efb761a 100644 > --- a/tcp_buf.c > +++ b/tcp_buf.c > @@ -408,6 +408,7 @@ int tcp_buf_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn) > uint32_t already_sent, seq; > struct iovec *iov; > > + /* How much have we read/sent since last received ack ? */ > already_sent = conn->seq_to_tap - conn->seq_ack_from_tap; > > if (SEQ_LT(already_sent, 0)) { > @@ -416,6 +417,7 @@ int tcp_buf_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn) > conn->seq_ack_from_tap, conn->seq_to_tap); > conn->seq_to_tap = conn->seq_ack_from_tap; > already_sent = 0; > + tcp_set_peek_offset(s, 0); > } > > if (!wnd_scaled || already_sent >= wnd_scaled) { > @@ -433,11 +435,16 @@ int tcp_buf_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn) > iov_rem = (wnd_scaled - already_sent) % mss; > } > > - mh_sock.msg_iov = iov_sock; > - mh_sock.msg_iovlen = fill_bufs + 1; > - > - iov_sock[0].iov_base = tcp_buf_discard; > - iov_sock[0].iov_len = already_sent; > + /* Prepare iov according to kernel capability */ > + if (!peek_offset_cap) { > + mh_sock.msg_iov = iov_sock; > + iov_sock[0].iov_base = tcp_buf_discard; > + iov_sock[0].iov_len = already_sent; > + mh_sock.msg_iovlen = fill_bufs + 1; > + } else { > + mh_sock.msg_iov = &iov_sock[1]; > + mh_sock.msg_iovlen = fill_bufs; > + } > > if (( v4 && tcp4_payload_used + fill_bufs > TCP_FRAMES_MEM) || > (!v4 && tcp6_payload_used + fill_bufs > TCP_FRAMES_MEM)) { > @@ -478,7 +485,10 @@ int tcp_buf_data_from_sock(struct ctx *c, struct tcp_tap_conn *conn) > return 0; > } > > - sendlen = len - already_sent; > + sendlen = len; > + if (!peek_offset_cap) > + sendlen -= already_sent; > + > if (sendlen <= 0) { > conn_flag(c, conn, STALLED); > return 0; -- Stefano