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.133.124]) by passt.top (Postfix) with ESMTP id 60B295A004E for ; Wed, 12 Jun 2024 00:09:46 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1718143785; 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=lGUtpDzJgxw2ZKOzj8pJOoNiehhyn/kv8gmFZh5KV5M=; b=IPjQySaXaNH/o8qEQ/V7o15Op/dn5CH2cOjKtQ6t4futzrB2DvjmfJFvQ8wg/fcvbqhc7+ NNOSYnfcEFmQFxN9qnv6Bpm4wMJ7uiqcOHZecarM7W1/kpiivfUAIeEItmX6KrihdYrAc6 Jh29L4bL8QGE9e4gFmTpXmKueL3JYSk= Received: from mail-oi1-f200.google.com (mail-oi1-f200.google.com [209.85.167.200]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-52-uiUU4PXVNPuE4ZBcXFKOiw-1; Tue, 11 Jun 2024 18:09:43 -0400 X-MC-Unique: uiUU4PXVNPuE4ZBcXFKOiw-1 Received: by mail-oi1-f200.google.com with SMTP id 5614622812f47-3d229ae4cdfso1411398b6e.1 for ; Tue, 11 Jun 2024 15:09:43 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1718143782; x=1718748582; 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=lGUtpDzJgxw2ZKOzj8pJOoNiehhyn/kv8gmFZh5KV5M=; b=F+CpT609Pb7Wn9oeE4c709pBvLGp+jZbeGDikz3apmYWU70fRV+Ervp1jZE9XKwCVH Rd0iOelSWvCyNoiVzwRfFXppRiKm92LOmLnnOe6EPXi1VO2ddEWqLehX+YTlDItIWpF9 g2gE95t9ff2dqX2n5D8WIvZH8GThrgV5ky9IuN1j33IBuNlCwb5ltIstOSktpNVU+zwx BaH5Qd5X2g1OoqbAFy2aWXOOg7YA5TLTEqaNCSlj/tzkbXDqso24yVv4WHcKYK9FepHu g1ADunpzpT8CFWj8iH/3ASRq9rRKqgw3cS/r2o+bO+iiH7huNg7u6+ZDYxcRvTu9q1JB Bv5Q== X-Gm-Message-State: AOJu0YxEOG1YpbQm2NikKY2/iLBXSqwJE0sgcXa66WWTlstqvGxl9/9f jNt8YwLIYUEnA3cupoNbBWbtfsAJkIhzMEOR5P7y4CXCVKCTPpAQg1ehw16dmtawNxd82lr5grL ibY/Y4V/6PaA4mdGZxaxja0VGj+kAS3/FrHw2+RDAP4Y/ormFoUfe1chJ7Pa0sOgjHOewVif1at srojmu2iscwvgz09JVEdtoFbZRhLC3hZkygtg= X-Received: by 2002:a05:6808:f11:b0:3d2:2d1d:3786 with SMTP id 5614622812f47-3d23e03c95amr246325b6e.25.1718143780426; Tue, 11 Jun 2024 15:09:40 -0700 (PDT) X-Google-Smtp-Source: AGHT+IENkmWQBmOFtL2zHxFkBcP1eY4MzKndW+DC03cuBMtuMdvmO3egY+yBwybCrcydsm6tiSSULw== X-Received: by 2002:a05:6808:f11:b0:3d2:2d1d:3786 with SMTP id 5614622812f47-3d23e03c95amr246296b6e.25.1718143779644; Tue, 11 Jun 2024 15:09:39 -0700 (PDT) Received: from maya.cloud.tilaa.com (maya.cloud.tilaa.com. [164.138.29.33]) by smtp.gmail.com with ESMTPSA id d75a77b69052e-440517524aesm39311091cf.76.2024.06.11.15.09.38 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Tue, 11 Jun 2024 15:09:39 -0700 (PDT) Date: Wed, 12 Jun 2024 00:09:04 +0200 From: Stefano Brivio To: Laurent Vivier Subject: Re: [PATCH v5 1/8] tcp: extract buffer management from tcp_send_flag() Message-ID: <20240612000904.59b438c5@elisabeth> In-Reply-To: <20240605152129.1641658-2-lvivier@redhat.com> References: <20240605152129.1641658-1-lvivier@redhat.com> <20240605152129.1641658-2-lvivier@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: S7O4CYX6UOMFENELXNBCWH2ILIUV3F2S X-Message-ID-Hash: S7O4CYX6UOMFENELXNBCWH2ILIUV3F2S 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 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 Wed, 5 Jun 2024 17:21:22 +0200 Laurent Vivier wrote: > This commit isolates the internal data structure management used for storing > data (e.g., tcp4_l2_flags_iov[], tcp6_l2_flags_iov[], tcp4_flags_ip[], > tcp4_flags[], ...) from the tcp_send_flag() function. The extracted > functionality is relocated to a new function named tcp_fill_flag_header(). > > tcp_fill_flag_header() is now a generic function that accepts parameters such > as struct tcphdr and a data pointer. tcp_send_flag() utilizes this parameter to > pass memory pointers from tcp4_l2_flags_iov[] and tcp6_l2_flags_iov[]. > > This separation sets the stage for utilizing tcp_fill_flag_header() to > set the memory provided by the guest via vhost-user in future developments. > > Signed-off-by: Laurent Vivier > --- > tcp.c | 63 ++++++++++++++++++++++++++++++++++++----------------------- > 1 file changed, 39 insertions(+), 24 deletions(-) > > diff --git a/tcp.c b/tcp.c > index 06acb41e4d90..68d4afa05a36 100644 > --- a/tcp.c > +++ b/tcp.c > @@ -1549,24 +1549,25 @@ static void tcp_update_seqack_from_tap(const struct ctx *c, > } > > /** > - * tcp_send_flag() - Send segment with flags to tap (no payload) > + * tcp_fill_flag_header() - Prepare header for flags-only segment (no payload) > * @c: Execution context > * @conn: Connection pointer > * @flags: TCP flags: if not set, send segment only if ACK is due > + * @th: TCP header to update > + * @data: buffer to store TCP option > + * @optlen: size of the TCP option buffer Now, this becomes an output parameter if SYN is set in flags, but it's otherwise an input parameter (and it must be zero, otherwise the data offset field we send will be wrong). I think having it always as output parameter (that is, setting it to zero on non-SYN in this function, not in the caller) would be less fragile and easier to describe in the comment, too. Or, even simpler, pass it as input parameter, and calculate it in the caller. The caller sets 'flags' anyway. > * > - * Return: negative error code on connection reset, 0 otherwise > + * Return: < 0 error code on connection reset, > + * 0 if there is no flag to send As you use one tab to indent "1 otherwise" below, you could use one here as well. > + * 1 otherwise > */ > -static int tcp_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags) > +static int tcp_fill_flag_header(struct ctx *c, struct tcp_tap_conn *conn, > + int flags, struct tcphdr *th, char *data, > + size_t *optlen) > { > - struct tcp_flags_t *payload; > struct tcp_info tinfo = { 0 }; > socklen_t sl = sizeof(tinfo); > int s = conn->sock; > - size_t optlen = 0; > - struct tcphdr *th; > - struct iovec *iov; > - size_t l4len; > - char *data; > > if (SEQ_GE(conn->seq_ack_to_tap, conn->seq_from_tap) && > !flags && conn->wnd_to_tap) > @@ -1588,20 +1589,11 @@ static int tcp_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags) > if (!tcp_update_seqack_wnd(c, conn, flags, &tinfo) && !flags) > return 0; > > - if (CONN_V4(conn)) > - iov = tcp4_l2_flags_iov[tcp4_flags_used++]; > - else > - iov = tcp6_l2_flags_iov[tcp6_flags_used++]; > - > - payload = iov[TCP_IOV_PAYLOAD].iov_base; > - th = &payload->th; > - data = payload->opts; > - > if (flags & SYN) { > int mss; > > /* Options: MSS, NOP and window scale (8 bytes) */ > - optlen = OPT_MSS_LEN + 1 + OPT_WS_LEN; > + *optlen = OPT_MSS_LEN + 1 + OPT_WS_LEN; > > *data++ = OPT_MSS; > *data++ = OPT_MSS_LEN; > @@ -1635,17 +1627,13 @@ static int tcp_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags) > flags |= ACK; > } > > - th->doff = (sizeof(*th) + optlen) / 4; > + th->doff = (sizeof(*th) + *optlen) / 4; > > th->ack = !!(flags & ACK); > th->rst = !!(flags & RST); > th->syn = !!(flags & SYN); > th->fin = !!(flags & FIN); > > - l4len = tcp_l2_buf_fill_headers(c, conn, iov, optlen, NULL, > - conn->seq_to_tap); > - iov[TCP_IOV_PAYLOAD].iov_len = l4len; > - > if (th->ack) { > if (SEQ_GE(conn->seq_ack_to_tap, conn->seq_from_tap)) > conn_flag(c, conn, ~ACK_TO_TAP_DUE); > @@ -1660,6 +1648,33 @@ static int tcp_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags) > if (th->fin || th->syn) > conn->seq_to_tap++; > > + return 1; > +} > + > +static int tcp_send_flag(struct ctx *c, struct tcp_tap_conn *conn, int flags) > +{ > + struct tcp_flags_t *payload; > + size_t optlen = 0; > + struct iovec *iov; > + size_t l4len; > + int ret; > + > + if (CONN_V4(conn)) > + iov = tcp4_l2_flags_iov[tcp4_flags_used++]; > + else > + iov = tcp6_l2_flags_iov[tcp6_flags_used++]; > + > + payload = iov[TCP_IOV_PAYLOAD].iov_base; > + > + ret = tcp_fill_flag_header(c, conn, flags, &payload->th, > + payload->opts, &optlen); > + if (ret <= 0) > + return ret; > + > + l4len = tcp_l2_buf_fill_headers(c, conn, iov, optlen, NULL, > + conn->seq_to_tap); > + iov[TCP_IOV_PAYLOAD].iov_len = l4len; > + > if (flags & DUP_ACK) { > struct iovec *dup_iov; > int i; -- Stefano