Re: [PATCH v6 4/4] tcp_vu: Support multibuffer frames in tcp_vu_send_flag()

On Thu, Apr 16, 2026 at 06:16:18PM +0200, Laurent Vivier wrote:

Build the Ethernet, IP, and TCP headers on the stack instead of directly in the buffer via pointer casts, then write them into the iovec with IOV_PUSH_HEADER(). This mirrors the approach already used in tcp_vu_prepare() and udp_vu_prepare().

Remove the vu_eth(), vu_ip(), vu_payloadv4() and vu_payloadv6() helpers from vu_common.h, as they are no longer used anywhere.

Introduce tcp_vu_send_dup() to handle DUP_ACK duplication using vu_collect() and iov_memcopy() instead of a plain memcpy(), so that

s/iov_memcopy/iov_memcpy/

the duplicated frame is also properly scattered across multiple iovecs.

Signed-off-by: Laurent Vivier --- iov.c | 1 - tcp_vu.c | 153 ++++++++++++++++++++++++++++++---------------------- vu_common.h | 20 ------- 3 files changed, 90 insertions(+), 84 deletions(-)

diff --git a/iov.c b/iov.c index c0d9c6d21322..31fc3e479572 100644 --- a/iov.c +++ b/iov.c @@ -208,7 +208,6 @@ void iov_memset(const struct iovec *iov, size_t iov_cnt, size_t offset, int c, * * Return: total number of bytes copied */ -/* cppcheck-suppress unusedFunction */ size_t iov_memcpy(struct iovec *dst_iov, size_t dst_iov_cnt, size_t dst_offset, const struct iovec *src_iov, size_t src_iov_cnt, size_t src_offset, size_t length) diff --git a/tcp_vu.c b/tcp_vu.c index 96b16007701d..7f7e43860b10 100644 --- a/tcp_vu.c +++ b/tcp_vu.c @@ -74,6 +74,42 @@ static size_t tcp_vu_hdrlen(bool v6) return hdrlen; }

+/** + * tcp_vu_send_dup() - Duplicate a frame into a new virtqueue element + * @c: Execution context + * @vq: Receive virtqueue + * @dest_elem: Destination virtqueue element to collect + * @dest_iov: Destination iovec array for collected buffers + * @max_dest_iov: Maximum number of entries in @dest_iov + * @src_iov: Source iovec array containing the frame to duplicate + * @src_cnt: Number of entries in @src_iov

As Jon noted, @vnlen is missing.

+ * + * Return: number of virtqueue elements collected (0 if none available) + */ +static int tcp_vu_send_dup(const struct ctx *c, struct vu_virtq *vq, + struct vu_virtq_element *dest_elem, + struct iovec *dest_iov, size_t max_dest_iov, + const struct iovec *src_iov, size_t src_cnt, + size_t vnlen) +{ + const struct vu_dev *vdev = c->vdev; + size_t dest_cnt; + int elem_cnt; + + elem_cnt = vu_collect(vdev, vq, dest_elem, 1, dest_iov, max_dest_iov, + &dest_cnt, vnlen, NULL); + if (elem_cnt == 0) + return 0; + + iov_memcpy(dest_iov, dest_cnt, 0, src_iov, src_cnt, 0, + MAX(VNET_HLEN + ETH_ZLEN, vnlen)); + + if (*c->pcap) + pcap_iov(dest_iov, dest_cnt, VNET_HLEN, vnlen - VNET_HLEN); + + return elem_cnt; +} + /** * tcp_vu_send_flag() - Send segment with flags to vhost-user (no payload) * @c: Execution context @@ -86,97 +122,88 @@ int tcp_vu_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags) { struct vu_dev *vdev = c->vdev; struct vu_virtq *vq = &vdev->vq[VHOST_USER_RX_QUEUE]; + size_t optlen, hdrlen, iov_cnt, iov_used; struct vu_virtq_element flags_elem[2];

Does the rationale for this having only 2 elements still apply?

- size_t optlen, hdrlen, l2len; - struct ipv6hdr *ip6h = NULL; - struct iphdr *ip4h = NULL; - struct iovec flags_iov[2]; - struct tcp_syn_opts *opts; + struct iovec flags_iov[64]; + int elem_cnt, dup_elem_cnt = 0; + struct tcp_syn_opts opts; struct iov_tail payload; - struct tcphdr *th; - struct ethhdr *eh; + struct ipv6hdr ip6h; + struct iphdr ip4h; + struct tcphdr th; + struct ethhdr eh; uint32_t seq; - int elem_cnt; int ret;

hdrlen = tcp_vu_hdrlen(CONN_V6(conn));

elem_cnt = vu_collect(vdev, vq, &flags_elem[0], 1, - &flags_iov[0], 1, NULL, - hdrlen + sizeof(*opts), NULL); - if (elem_cnt != 1) + flags_iov, ARRAY_SIZE(flags_iov), &iov_cnt, + hdrlen + sizeof(opts), NULL); + if (elem_cnt == 0) return -1;

- assert(flags_elem[0].in_num == 1); - assert(flags_elem[0].in_sg[0].iov_len >= - MAX(hdrlen + sizeof(*opts), ETH_ZLEN + VNET_HLEN)); - - eh = vu_eth(flags_elem[0].in_sg[0].iov_base); - - memcpy(eh->h_dest, c->guest_mac, sizeof(eh->h_dest)); - memcpy(eh->h_source, c->our_tap_mac, sizeof(eh->h_source)); - - if (CONN_V4(conn)) { - eh->h_proto = htons(ETH_P_IP); - - ip4h = vu_ip(flags_elem[0].in_sg[0].iov_base); - *ip4h = (struct iphdr)L2_BUF_IP4_INIT(IPPROTO_TCP); - - th = vu_payloadv4(flags_elem[0].in_sg[0].iov_base); - } else { - eh->h_proto = htons(ETH_P_IPV6); + memcpy(eh.h_dest, c->guest_mac, sizeof(eh.h_dest));

- ip6h = vu_ip(flags_elem[0].in_sg[0].iov_base); - *ip6h = (struct ipv6hdr)L2_BUF_IP6_INIT(IPPROTO_TCP); - th = vu_payloadv6(flags_elem[0].in_sg[0].iov_base); - } + if (CONN_V4(conn)) + ip4h = (struct iphdr)L2_BUF_IP4_INIT(IPPROTO_TCP); + else + ip6h = (struct ipv6hdr)L2_BUF_IP6_INIT(IPPROTO_TCP);

- memset(th, 0, sizeof(*th)); - th->doff = sizeof(*th) / 4; - th->ack = 1;

Where are doff and ack now initialised? Or were these already unneeded?

+ memset(&th, 0, sizeof(th));

This could be an initialiser now.

seq = conn->seq_to_tap; - opts = (struct tcp_syn_opts *)(th + 1); - ret = tcp_prepare_flags(c, conn, flags, th, opts, &optlen); + ret = tcp_prepare_flags(c, conn, flags, &th, &opts, &optlen); if (ret <= 0) { - vu_queue_rewind(vq, 1); + vu_queue_rewind(vq, elem_cnt); return ret; }

- payload = IOV_TAIL(flags_elem[0].in_sg, 1, hdrlen); - if (flags & KEEPALIVE) seq--;

- tcp_fill_headers(c, conn, eh, ip4h, ip6h, th, &payload, + iov_used = iov_skip_bytes(flags_iov, iov_cnt, + MAX(optlen + hdrlen, VNET_HLEN + ETH_ZLEN), + NULL); + if (iov_used < iov_cnt) + iov_used++; + iov_cnt = iov_used; + + payload = IOV_TAIL(flags_elem[0].in_sg, iov_cnt, hdrlen);

@payload here contains the TCP payload...

+ iov_from_buf(payload.iov, payload.cnt, payload.off, &opts, optlen); + tcp_fill_headers(c, conn, &eh, CONN_V4(conn) ? &ip4h : NULL, + CONN_V6(conn) ? &ip6h : NULL, &th, &payload, optlen, IP4_CSUM | (*c->pcap ? TCP_CSUM : 0), seq);

- vu_pad(flags_elem[0].in_sg, 1, hdrlen + optlen); - vu_flush(vdev, vq, flags_elem, 1, hdrlen + optlen); + vu_pad(flags_elem[0].in_sg, iov_cnt, hdrlen + optlen); + + /* write headers */ + payload = IOV_TAIL(flags_elem[0].in_sg, iov_cnt, VNET_HLEN);

...but here contains the whole L2 frame, which seems confusing. Maybe name the variable differently? Or even a second variable?

+ + IOV_PUSH_HEADER(&payload, eh); + if (CONN_V4(conn)) + IOV_PUSH_HEADER(&payload, ip4h); + else + IOV_PUSH_HEADER(&payload, ip6h); + IOV_PUSH_HEADER(&payload, th);

- l2len = hdrlen + optlen - VNET_HLEN; if (*c->pcap) - pcap_iov(&flags_elem[0].in_sg[0], 1, VNET_HLEN, l2len); + pcap_iov(flags_elem[0].in_sg, iov_cnt, VNET_HLEN, + hdrlen + optlen - VNET_HLEN);

if (flags & DUP_ACK) { - elem_cnt = vu_collect(vdev, vq, &flags_elem[1], 1, - &flags_iov[1], 1, NULL, - hdrlen + optlen, NULL); - if (elem_cnt == 1 && - flags_elem[1].in_sg[0].iov_len >= - flags_elem[0].in_sg[0].iov_len) { - memcpy(flags_elem[1].in_sg[0].iov_base, - flags_elem[0].in_sg[0].iov_base, - flags_elem[0].in_sg[0].iov_len); - - vu_flush(vdev, vq, &flags_elem[1], 1, hdrlen + optlen); - - if (*c->pcap) { - pcap_iov(&flags_elem[1].in_sg[0], 1, VNET_HLEN, - l2len); - } - } + dup_elem_cnt = tcp_vu_send_dup(c, vq, &flags_elem[elem_cnt], + &flags_iov[iov_cnt], + ARRAY_SIZE(flags_iov) - iov_cnt, + flags_elem[0].in_sg, iov_cnt, + hdrlen + optlen); } + vu_flush(vdev, vq, flags_elem, elem_cnt, hdrlen + optlen); + if (dup_elem_cnt) { + vu_flush(vdev, vq, &flags_elem[elem_cnt], dup_elem_cnt, + hdrlen + optlen); + } + vu_queue_notify(vdev, vq);

return 0; diff --git a/vu_common.h b/vu_common.h index 51f70084a7cb..817384175a1d 100644 --- a/vu_common.h +++ b/vu_common.h @@ -15,26 +15,6 @@ #include "ip.h" #include "virtio.h"

-static inline void *vu_eth(void *base) -{ - return ((char *)base + VNET_HLEN); -} - -static inline void *vu_ip(void *base) -{ - return (struct ethhdr *)vu_eth(base) + 1; -} - -static inline void *vu_payloadv4(void *base) -{ - return (struct iphdr *)vu_ip(base) + 1; -} - -static inline void *vu_payloadv6(void *base) -{ - return (struct ipv6hdr *)vu_ip(base) + 1; -} - int vu_collect(const struct vu_dev *vdev, struct vu_virtq *vq, struct vu_virtq_element *elem, int max_elem, struct iovec *in_sg, size_t max_in_sg, size_t *in_total, -- 2.53.0

-- David Gibson (he or they) | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you, not the other way | around. http://www.ozlabs.org/~dgibson