[PATCH v7 00/31] Introduce discontiguous frames management

newer
[PATCH v3] treewide: By default,...

Laurent Vivier

23 Jun 2025 23 Jun '25

1:06 p.m.

This series introduces iov_tail to convey frame information between functions. This is only an API change, for the moment the memory pool is only able to store contiguous buffer, so, except for vhost-user in a special case, we only play with iovec array with only one entry. v7: - Add a patch to fix comment style of 'Return:' - Fix ignore_arp()/accept_arp() - Fix coverity error - Fix several comments v6: - Replaced iov_slice() with the clearer iov_tail_clone() for creating iovec subsets. - Standardized local header variable names (to *_storage suffix). - Renamed functions for better semantics (e.g., ignore_arp to accept_arp, packet_data to packet_get). - Corrected OPTLEN_MAX definition in TCP. - Addressed minor logic issues (e.g., DHCPv6 FQDN flags, NDP null check). - Updated ipv6_l4hdr() return type to boolean. - Improved comments and documentation across several modules. v5: - store in the pool iovec array with several entries v4: Prepare to introduce iovec array in the pool: - passe iov_tail rather than pool to ndp,icmp, dhcp, dhcpv6 and arp - remove unused pool macros - add memory regions in the pool structure, this will allow us to use the buf pointer to store the iovec array for vhost-user v3: Address comments from David Laurent Vivier (31): style: Fix 'Return' comment style arp: Don't mix incoming and outgoing buffers iov: Introduce iov_tail_clone() and iov_tail_drop(). iov: Update IOV_REMOVE_HEADER() and IOV_PEEK_HEADER() tap: Use iov_tail with tap_add_packet() packet: Use iov_tail with packet_add() packet: Add packet_data() arp: Convert to iov_tail ndp: Convert to iov_tail icmp: Convert to iov_tail udp: Convert to iov_tail tcp: Convert tcp_tap_handler() to use iov_tail tcp: Convert tcp_data_from_tap() to use iov_tail dhcpv6: move offset initialization out of dhcpv6_opt() dhcpv6: Extract sending of NotOnLink status dhcpv6: Convert to iov_tail dhcpv6: Use iov_tail in dhcpv6_opt() dhcp: Convert to iov_tail ip: Use iov_tail in ipv6_l4hdr() tap: Convert tap4_handler() to iov_tail tap: Convert tap6_handler() to iov_tail packet: rename packet_data() to packet_get() arp: use iov_tail rather than pool dhcp: use iov_tail rather than pool dhcpv6: use iov_tail rather than pool icmp: use iov_tail rather than pool ndp: use iov_tail rather than pool packet: remove PACKET_POOL() and PACKET_POOL_P() packet: remove unused parameter from PACKET_POOL_DECL() packet: add memory regions information into pool packet: use buf to store iovec array arp.c | 86 ++++++++++++------- arp.h | 2 +- checksum.c | 4 +- conf.c | 12 +-- dhcp.c | 48 ++++++----- dhcp.h | 2 +- dhcpv6.c | 223 +++++++++++++++++++++++++++++++------------------- dhcpv6.h | 2 +- flow.c | 4 +- flow_table.h | 2 +- icmp.c | 43 ++++++---- icmp.h | 2 +- inany.c | 4 +- iov.c | 118 ++++++++++++++++++++++---- iov.h | 58 +++++++++---- ip.c | 32 ++++---- ip.h | 3 +- lineread.c | 2 +- ndp.c | 16 +++- ndp.h | 4 +- packet.c | 227 ++++++++++++++++++++++++++++++++++++++------------- packet.h | 57 ++++++------- pcap.c | 1 + tap.c | 134 +++++++++++++++++------------- tap.h | 8 +- tcp.c | 67 ++++++++++----- tcp_buf.c | 2 +- tcp_splice.c | 4 +- tcp_vu.c | 4 +- udp.c | 37 ++++++--- udp_vu.c | 6 +- util.c | 6 +- util.h | 4 +- vhost_user.c | 72 ++++++++-------- virtio.c | 6 +- virtio.h | 24 ++++-- vu_common.c | 48 ++++------- 37 files changed, 870 insertions(+), 504 deletions(-) -- 2.49.0

Show replies by date

Laurent Vivier

...

In vhost-user mode, where `pool->buf` doesn't store packet data directly, this patch repurposes it to hold the `struct iovec` array describing a scattered packet. This enables pools to manage true scatter-gather descriptors, with iovecs pointing to guest memory.

`p->pkt[idx].iov_base` is now an index into this `pool->buf` iovec array, and `p->pkt[idx].iov_len` is the count of iovecs. `packet_add_do` uses `iov_tail_slice` to store these iovec descriptors from an input iov_tail into `pool->buf`. `packet_data_do` reconstructs the iov_tail for a packet by pointing to this stored array of iovecs.

Signed-off-by: Laurent Vivier --- packet.c | 164 +++++++++++++++++++++++++++++++++++++++++++++++++------ 1 file changed, 148 insertions(+), 16 deletions(-)

diff --git a/packet.c b/packet.c index fd3b6db57c30..8dbe00af12c6 100644 --- a/packet.c +++ b/packet.c @@ -84,6 +84,122 @@ bool pool_full(const struct pool *p) return p->count >= p->size; }

+/** + * packet_iov_max_cnt() - Return the maximum number of iovec entries we can + * store + * @p: Pointer to packet pool + * + * Return: the maximum number of iovec entries we can store in the memory of + * the pool buffer + */ +static size_t packet_iov_max_cnt(const struct pool *p) +{ + return p->buf_size / sizeof(struct iovec); +} + +/** + * packet_iov_idx() - For a given packet index, return the iovec index and + * the number of iovec entry of the packet + * @p: Pointer to packet pool + * @idx: Index of packet descriptor in pool + * @iov_cnt: Pointer to store the number of the iovec entry of the packet + * @func: For tracing: name of calling function + * @line: For tracing: caller line of function call + * + * Return: the iovec index for the given packet index, @iov_cnt is set + * to the number of the iovec entry of the packet + */ +static int packet_iov_idx(const struct pool *p, size_t idx, size_t *iov_cnt, + const char *func, int line) +{ + size_t iov_idx, max = packet_iov_max_cnt(p); + + iov_idx = (size_t)p->pkt[idx].iov_base; + *iov_cnt = p->pkt[idx].iov_len; + + ASSERT_WITH_MSG(iov_idx + *iov_cnt <= max, + "Corrupt iov entry: (%zu, %zu), max: %zu, %s:%i", + iov_idx, *iov_cnt, max, func, line); + + return iov_idx; +} + +/** + * packet_iov_next_idx() - Give the the next available iovec index

I'm taking the liberty to fix this comment while applying ("Get the next available iovec index"). -- Stefano

Stefano Brivio

8:45 p.m.

On Mon, 23 Jun 2025 13:06:04 +0200 Laurent Vivier wrote:

...

This series introduces iov_tail to convey frame information between functions.

This is only an API change, for the moment the memory pool is only able to store contiguous buffer, so, except for vhost-user in a special case, we only play with iovec array with only one entry.

v7: - Add a patch to fix comment style of 'Return:' - Fix ignore_arp()/accept_arp() - Fix coverity error - Fix several comments

I was about to apply this without 1/31 (I applied the v2 of it you sent outside of this series instead, which is actually up to date) and with the minor comment fix to 31/31... but the test perf/passt_vu_tcp fails rather consistently now (and I triple checked without this series): - "TCP throughput over IPv6: guest to host" with MTU 1500 and 9000 bytes now reports between 0 and 0.6 Gbps. The guest kernel prints a series of two messages with ~1-10 µs interval: [ 21.159827] TCP: out of memory -- consider tuning tcp_mem [ 21.159831] TCP: out of memory -- consider tuning tcp_mem - "TCP throughput over IPv4: guest to host" never reports 0 Gbps, but the throughput figure for large MTU (65520 bytes) is very low (5.4 Gbps in the last run). Here I'm getting four messages: [ 40.807818] TCP: out of memory -- consider tuning tcp_mem [ 40.807829] TCP: out of memory -- consider tuning tcp_mem [ 40.807829] TCP: out of memory -- consider tuning tcp_mem [ 40.807830] TCP: out of memory -- consider tuning tcp_mem - on the reverse direction, "TCP throughput over IPv4: host to guest" (but not with IPv6), the iperf3 client gets SIGSEGV, but not consistently, it happened once out of five times. To me it smells a bit like we're leaking virtqueue slots but I looked again at the whole series and I couldn't find anything obvious... at least not yet. UDP tests never fail and the throughput is the same as before. I'm stopping here for the moment, let me know if I should run more tests or anything. By the way, I'm running just those tests by enclosing the rest of the sequence in test/run in a dummy function, say: --- diff --git a/test/run b/test/run index f73c311..97b96a4 100755 --- a/test/run +++ b/test/run @@ -67,6 +67,7 @@ run() { perf_init [ ${CI} -eq 1 ] && video_start ci +dont() { setup build test build/all test build/cppcheck @@ -178,6 +179,8 @@ run() { test perf/pasta_udp test passt_in_ns/shutdown teardown passt_in_ns +} + VALGRIND=0 VHOST_USER=1 setup passt_in_ns --- ...you could also pass the setup steps plus test to ./run, but you would need to set VHOST_USER=1 as well. -- Stefano

Laurent Vivier

24 Jul 24 Jul

3:01 p.m.

On 18/07/2025 20:45, Stefano Brivio wrote:

...

On Mon, 23 Jun 2025 13:06:04 +0200 Laurent Vivier wrote:

...
This series introduces iov_tail to convey frame information between functions.

This is only an API change, for the moment the memory pool is only able to store contiguous buffer, so, except for vhost-user in a special case, we only play with iovec array with only one entry.

v7: - Add a patch to fix comment style of 'Return:' - Fix ignore_arp()/accept_arp() - Fix coverity error - Fix several comments

I was about to apply this without 1/31 (I applied the v2 of it you sent outside of this series instead, which is actually up to date) and with the minor comment fix to 31/31... but the test perf/passt_vu_tcp fails rather consistently now (and I triple checked without this series):

- "TCP throughput over IPv6: guest to host" with MTU 1500 and 9000 bytes now reports between 0 and 0.6 Gbps. The guest kernel prints a series of two messages with ~1-10 µs interval:

[ 21.159827] TCP: out of memory -- consider tuning tcp_mem [ 21.159831] TCP: out of memory -- consider tuning tcp_mem

- "TCP throughput over IPv4: guest to host" never reports 0 Gbps, but the throughput figure for large MTU (65520 bytes) is very low (5.4 Gbps in the last run). Here I'm getting four messages:

[ 40.807818] TCP: out of memory -- consider tuning tcp_mem [ 40.807829] TCP: out of memory -- consider tuning tcp_mem [ 40.807829] TCP: out of memory -- consider tuning tcp_mem [ 40.807830] TCP: out of memory -- consider tuning tcp_mem

- on the reverse direction, "TCP throughput over IPv4: host to guest" (but not with IPv6), the iperf3 client gets SIGSEGV, but not consistently, it happened once out of five times.

To me it smells a bit like we're leaking virtqueue slots but I looked again at the whole series and I couldn't find anything obvious... at least not yet.

UDP tests never fail and the throughput is the same as before.

I think the problem is the way we use the iovec array. In tap4_handler() we have a packet_get() that provides a pointer to the iovec array from pool. Idx is 0, iovec idx is 0. Then we have a pool_flush(), so first available idx is now 0 again. And then we have packet_add() with the iovec idx (in "data") of the previous packet_get() that we try to add at the same index (as pool is empty again, and first available idx is 0). When I wrote this patch I guessed that when we release packet (pool_flush()) we don't use anymore the iovec array of the packet, it appears to be not true. Thanks, Laurent

Laurent Vivier

25 Jul 25 Jul

1:45 p.m.

On 24/07/2025 15:01, Laurent Vivier wrote:

...

On 18/07/2025 20:45, Stefano Brivio wrote:

...
On Mon, 23 Jun 2025 13:06:04 +0200 Laurent Vivier wrote:

...
This series introduces iov_tail to convey frame information between functions.

This is only an API change, for the moment the memory pool is only able to store contiguous buffer, so, except for vhost-user in a special case, we only play with iovec array with only one entry.

v7:      - Add a patch to fix comment style of 'Return:'      - Fix ignore_arp()/accept_arp()      - Fix coverity error      - Fix several comments

I was about to apply this without 1/31 (I applied the v2 of it you sent outside of this series instead, which is actually up to date) and with the minor comment fix to 31/31... but the test perf/passt_vu_tcp fails rather consistently now (and I triple checked without this series):

- "TCP throughput over IPv6: guest to host" with MTU 1500 and 9000    bytes now reports between 0 and 0.6 Gbps. The guest kernel prints a    series of two messages with ~1-10 µs interval:

[   21.159827] TCP: out of memory -- consider tuning tcp_mem [   21.159831] TCP: out of memory -- consider tuning tcp_mem

- "TCP throughput over IPv4: guest to host" never reports 0 Gbps, but    the throughput figure for large MTU (65520 bytes) is very low (5.4    Gbps in the last run). Here I'm getting four messages:

[   40.807818] TCP: out of memory -- consider tuning tcp_mem [   40.807829] TCP: out of memory -- consider tuning tcp_mem [   40.807829] TCP: out of memory -- consider tuning tcp_mem [   40.807830] TCP: out of memory -- consider tuning tcp_mem

- on the reverse direction, "TCP throughput over IPv4: host to guest"    (but not with IPv6), the iperf3 client gets SIGSEGV, but not    consistently, it happened once out of five times.

To me it smells a bit like we're leaking virtqueue slots but I looked again at the whole series and I couldn't find anything obvious... at least not yet.

UDP tests never fail and the throughput is the same as before.

I think the problem is the way we use the iovec array.

In tap4_handler() we have a packet_get() that provides a pointer to the iovec array from pool. Idx is 0, iovec idx is 0.

Then we have a pool_flush(), so first available idx is now 0 again.

And then we have packet_add() with the iovec idx (in "data") of the previous packet_get() that we try to add at the same index (as pool is empty again, and first available idx is 0).

When I wrote this patch I guessed that when we release packet (pool_flush()) we don't use anymore the iovec array of the packet, it appears to be not true.

Could you try the following patch (my iperf3 continue to crash <defunct> on my host system, not related to this problem), it's a little bit ugly (use of alloca()) but it's an easy fix. diff --git a/packet.c b/packet.c index 8dbe00af12c6..6d187192dd3a 100644 --- a/packet.c +++ b/packet.c @@ -155,18 +155,23 @@ static size_t packet_iov_next_idx(const struct pool *p, size_t idx, * @func: For tracing: name of calling function * @line: For tracing: caller line of function call */ -static void packet_iov_data(const struct pool *p, size_t idx, - struct iov_tail *data, - const char *func, int line) +static size_t packet_iov_data(const struct pool *p, size_t idx, + struct iovec *iov, size_t cnt, + const char *func, int line) { - struct iovec *iov = (struct iovec *)p->buf; - size_t iov_idx, iov_cnt; + const struct iovec *src_iov = (struct iovec *)p->buf; + size_t src_idx, src_cnt; + size_t i; - iov_idx = packet_iov_idx(p, idx, &iov_cnt, func, line); + src_idx = packet_iov_idx(p, idx, &src_cnt, func, line); - data->iov = &iov[iov_idx]; - data->cnt = iov_cnt; - data->off = 0; + if (cnt < src_cnt) + return 0; + + for (i = 0; i < src_cnt; i++) + iov[i] = src_iov[src_idx + i]; + + return i; } /** @@ -268,6 +273,7 @@ void packet_add_do(struct pool *p, struct iov_tail *data, */ bool packet_get_do(const struct pool *p, size_t idx, struct iov_tail *data, + struct iovec *iov, size_t cnt, const char *func, int line) { ASSERT_WITH_MSG(p->count <= p->size, @@ -281,12 +287,13 @@ bool packet_get_do(const struct pool *p, size_t idx, } if (p->memory) { - packet_iov_data(p, idx, data, func, line); + data->cnt = packet_iov_data(p, idx, iov, cnt, func, line); + data->iov = iov; } else { data->cnt = 1; - data->off = 0; data->iov = &p->pkt[idx]; } + data->off = 0; ASSERT_WITH_MSG(!packet_iov_check_range(p, data, func, line), "Corrupt packet pool, %s:%i", func, line); diff --git a/packet.h b/packet.h index 45843a6775ab..f1e28b3f31d3 100644 --- a/packet.h +++ b/packet.h @@ -35,14 +35,16 @@ int vu_packet_check_range(struct vdev_memory *memory, void packet_add_do(struct pool *p, struct iov_tail *data, const char *func, int line); bool packet_get_do(const struct pool *p, const size_t idx, - struct iov_tail *data, const char *func, int line); + struct iov_tail *data, + struct iovec *iov, size_t cnt, + const char *func, int line); bool pool_full(const struct pool *p); void pool_flush(struct pool *p); #define packet_add(p, data) \ packet_add_do(p, data, __func__, __LINE__) #define packet_get(p, idx, data) \ - packet_get_do(p, idx, data, __func__, __LINE__) + packet_get_do(p, idx, data, (struct iovec *)alloca(sizeof(struct iovec) * 10), 10, __func__, __LINE__) #define PACKET_POOL_DECL(_name, _size) \ struct _name ## _t { \

Stefano Brivio

4:18 p.m.

On Fri, 25 Jul 2025 13:45:36 +0200 Laurent Vivier wrote:

...

On 24/07/2025 15:01, Laurent Vivier wrote:

...
On 18/07/2025 20:45, Stefano Brivio wrote:

...
On Mon, 23 Jun 2025 13:06:04 +0200 Laurent Vivier wrote:

...
This series introduces iov_tail to convey frame information between functions.

This is only an API change, for the moment the memory pool is only able to store contiguous buffer, so, except for vhost-user in a special case, we only play with iovec array with only one entry.

v7:      - Add a patch to fix comment style of 'Return:'      - Fix ignore_arp()/accept_arp()      - Fix coverity error      - Fix several comments

I was about to apply this without 1/31 (I applied the v2 of it you sent outside of this series instead, which is actually up to date) and with the minor comment fix to 31/31... but the test perf/passt_vu_tcp fails rather consistently now (and I triple checked without this series):

- "TCP throughput over IPv6: guest to host" with MTU 1500 and 9000    bytes now reports between 0 and 0.6 Gbps. The guest kernel prints a    series of two messages with ~1-10 µs interval:

[   21.159827] TCP: out of memory -- consider tuning tcp_mem [   21.159831] TCP: out of memory -- consider tuning tcp_mem

- "TCP throughput over IPv4: guest to host" never reports 0 Gbps, but    the throughput figure for large MTU (65520 bytes) is very low (5.4    Gbps in the last run). Here I'm getting four messages:

[   40.807818] TCP: out of memory -- consider tuning tcp_mem [   40.807829] TCP: out of memory -- consider tuning tcp_mem [   40.807829] TCP: out of memory -- consider tuning tcp_mem [   40.807830] TCP: out of memory -- consider tuning tcp_mem

- on the reverse direction, "TCP throughput over IPv4: host to guest"    (but not with IPv6), the iperf3 client gets SIGSEGV, but not    consistently, it happened once out of five times.

To me it smells a bit like we're leaking virtqueue slots but I looked again at the whole series and I couldn't find anything obvious... at least not yet.

UDP tests never fail and the throughput is the same as before.

I think the problem is the way we use the iovec array.

In tap4_handler() we have a packet_get() that provides a pointer to the iovec array from pool. Idx is 0, iovec idx is 0.

Then we have a pool_flush(), so first available idx is now 0 again.

And then we have packet_add() with the iovec idx (in "data") of the previous packet_get() that we try to add at the same index (as pool is empty again, and first available idx is 0).

When I wrote this patch I guessed that when we release packet (pool_flush()) we don't use anymore the iovec array of the packet, it appears to be not true.

Could you try the following patch (my iperf3 continue to crash <defunct> on my host system, not related to this problem), it's a little bit ugly (use of alloca()) but it's an easy fix.

Weird, I don't get the "TCP: out of memory" messages anymore, but throughput still has some obvious issues: === perf/passt_vu_tcp

...

passt: throughput and latency Throughput in Gbps, latency in µs, 6 threads at 3.6 GHz MTU: | 256B | 576B | 1280B | 1500B | 9000B | 65520B | |--------|--------|--------|--------|--------|--------| TCP throughput over IPv6: guest to host | - | - | 0.6 | 2.0 | 0 | 13.3 | TCP RR latency over IPv6: guest to host | - | - | - | - | - | 38 | TCP CRR latency over IPv6: guest to host | - | - | - | - | - | 95 | |--------|--------|--------|--------|--------|--------| TCP throughput over IPv4: guest to host | 0 | 2.2 | 1.0 | 1.7 | 9.9 | 16.7 | TCP RR latency over IPv4: guest to host | - | - | - | - | - | 35 | TCP CRR latency over IPv4: guest to host | - | - | - | - | - | 91 | |--------|--------|--------|--------|--------|--------|

...we should never have < 0.1 Gbps, and with 64k MTU, we otherwise have 30 - 40 Gbps. Even more weird, iperf3 now reliably crashes for me too, on the other path (host to guest). It never did without this series. I wonder if the issue is actually "fixed" with this patch but the alloca() is so large as to actually mess with throughput. I can play with that if it helps, or bisect... let me know. I have to admit I didn't fully grasp the problem at hand, simply because I haven't read carefully what you wrote yet. -- Stefano

Age (days ago)

Last active (days ago)

List overview

Download

36 comments

2 participants

participants (2)

Laurent Vivier
Stefano Brivio

[PATCH v7 00/31] Introduce discontiguous frames management

tags

participants (2)