- dev - passt

Pesto Protocol Proposals, imProved
by David Gibson 06 Mar '26

06 Mar '26

Stefano convinced me that my earlier proposal for the dynamic update protocol was unnecessarily complex. Plus, I saw a much better way of handling socket continuity in the context of a "whole table" replacement. So here's an entirely revised protocol suggestion. # Outline I suggest that each connection to the control socket handles a single transaction. 1. Server hello - Server sends magic number, version - Possibly feature flags / limits (e.g. max number of rules allowed) 2. Client hello - Client sends magic number - Do we need anything else? 3. Server lists pifs - Server sends the number of pifs, their indices and names 4. Server lists rules - Server sends the list of rules, one pif at a time 5. Client gives new rules - Client sends the new list of rules, one pif at a time - Server loads them into the shadow table, and validates (no socket operations) 6. Server acknowledges - Either reports an error and disconnects, or acks waiting for client 7. Client signals apply - Server swaps shadow and active tables, and syncs sockets with new active table 8. Server gives error summary - Server reports bind/listen/whatever errors 9a. Client signals commit - Shadow table (now the old table) discarded or 9b. Client signals rollback - Shadow and active tables swapped back, syncs sockets - Discard shadow table (now the "new" table again) - New bind error report? 10. Server closes control connection # Client disconnects A client disconnect before step (7) is straightforward: discard the shadow table, nothing has changed. A client disconnect between (7) and (9) triggers a rollback, same as (9b). # Error reporting Error reporting at step (6) is fairly straightforward: we can send an error code and/or an error message. Error reporting at (8) is trickier. As a first cut, we could just report "yes" or "no" - taking into account the FWD_WEAK flag. But the client might be able to make better decisions or at least better messages to the user if we report more detailed information. Exactly how detailed is an open question: number of bind failures? number of failures per rule? specific ports which failed? # Interim steps I propose these steps toward implementing this: i. Merge TCP and UDP rule tables. The protocol above assumes a single rule table per-pif, which I think is an easier model to understand and more extensible for future protocol support. ii. Read-only client. Implement steps (1) to (4). Client can query and list the current rules, but not change them. iii. Rule updates. Implement remaining protocol steps, but with a "close and re-open" approach on the server, so unaltered listening sockets might briefly disappear iv. Socket continuity. Have the socket sync "steal" sockets from the old table in preference to re-opening them. If you have any time to work on (ii) while I work on (i), those should be parallelizable. # Concurrent updates Server guarantees that a single transaction as above is atomic in the sense that nothing else is allowed to change the rules between (4) and (9). The easiest way to do that initially is probably to only allow a single client connection at a time. If there's a reason to, we could alter that so that concurrent connections are allowed, but if another client changed anything after step (4), then we give an error on the next op (or maybe just close the control socket from the server side). # Tweaks / variants - I'm not sure that step (2) is necessary - I'm not certain that step (7) is necessary, although I do kind of prefer the client getting a chance to see a "so far, so good" before any socket operations happen. -- 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

1 0

[PATCH v3] iov: Add iov_truncate() helper and use it in vu handlers
by Laurent Vivier 06 Mar '26

06 Mar '26

Add a generic iov_truncate() function that truncates an IO vector to a given number of bytes, returning the number of iov entries that contain data after truncation. Use it in udp_vu_sock_recv() and tcp_vu_sock_recv() to replace the open-coded truncation logic that adjusted iov entries after recvmsg(). Also convert the direct iov_len assignment in tcp_vu_send_flag() to use iov_truncate() for consistency. Signed-off-by: Laurent Vivier <lvivier(a)redhat.com> --- Notes: v3: use in tcp_vu_send_flag() too v2: use iov_truncate() in udp_vu_sock_recv() too iov.c | 22 ++++++++++++++++++++++ iov.h | 1 + tcp_vu.c | 14 +++----------- udp_vu.c | 12 +++--------- 4 files changed, 29 insertions(+), 20 deletions(-) diff --git a/iov.c b/iov.c index ad726daa4cd8..31a3f5bc29e5 100644 --- a/iov.c +++ b/iov.c @@ -147,6 +147,28 @@ size_t iov_size(const struct iovec *iov, size_t iov_cnt) return len; } +/** + * iov_truncate() - Truncate an IO vector to a given number of bytes + * @iov: IO vector (modified) + * @iov_cnt: Number of entries in @iov + * @size: Total number of bytes to keep + * + * Return: number of iov entries that contain data after truncation + */ +size_t iov_truncate(struct iovec *iov, size_t iov_cnt, size_t size) +{ + size_t i, offset; + + i = iov_skip_bytes(iov, iov_cnt, size, &offset); + + if (i < iov_cnt) { + iov[i].iov_len = offset; + i += !!offset; + } + + return i; +} + /** * iov_tail_prune() - Remove any unneeded buffers from an IOV tail * @tail: IO vector tail (modified) diff --git a/iov.h b/iov.h index d1ab91a94e22..b4e50b0fca5a 100644 --- a/iov.h +++ b/iov.h @@ -29,6 +29,7 @@ size_t iov_from_buf(const struct iovec *iov, size_t iov_cnt, size_t iov_to_buf(const struct iovec *iov, size_t iov_cnt, size_t offset, void *buf, size_t bytes); size_t iov_size(const struct iovec *iov, size_t iov_cnt); +size_t iov_truncate(struct iovec *iov, size_t iov_cnt, size_t size); /* * DOC: Theory of Operation, struct iov_tail diff --git a/tcp_vu.c b/tcp_vu.c index 88be232dca66..8ca4170f13f6 100644 --- a/tcp_vu.c +++ b/tcp_vu.c @@ -131,7 +131,7 @@ int tcp_vu_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags) return ret; } - flags_elem[0].in_sg[0].iov_len = hdrlen + optlen; + iov_truncate(&flags_iov[0], 1, hdrlen + optlen); payload = IOV_TAIL(flags_elem[0].in_sg, 1, hdrlen); if (flags & KEEPALIVE) @@ -192,9 +192,9 @@ static ssize_t tcp_vu_sock_recv(const struct ctx *c, struct vu_virtq *vq, struct msghdr mh_sock = { 0 }; uint16_t mss = MSS_GET(conn); int s = conn->sock; - ssize_t ret, len; size_t hdrlen; int elem_cnt; + ssize_t ret; int i; *iov_cnt = 0; @@ -247,15 +247,7 @@ static ssize_t tcp_vu_sock_recv(const struct ctx *c, struct vu_virtq *vq, ret -= already_sent; /* adjust iov number and length of the last iov */ - len = ret; - for (i = 0; len && i < elem_cnt; i++) { - struct iovec *iov = &elem[i].in_sg[0]; - - if (iov->iov_len > (size_t)len) - iov->iov_len = len; - - len -= iov->iov_len; - } + i = iov_truncate(&iov_vu[DISCARD_IOV_NUM], elem_cnt, ret); /* adjust head count */ while (*head_cnt > 0 && head[*head_cnt - 1] >= i) diff --git a/udp_vu.c b/udp_vu.c index 3520f89e5671..5effca777e0a 100644 --- a/udp_vu.c +++ b/udp_vu.c @@ -71,9 +71,9 @@ static int udp_vu_sock_recv(const struct ctx *c, struct vu_virtq *vq, int s, bool v6, ssize_t *dlen) { const struct vu_dev *vdev = c->vdev; - int iov_cnt, idx, iov_used; - size_t off, hdrlen, l2len; struct msghdr msg = { 0 }; + int iov_cnt, iov_used; + size_t hdrlen, l2len; ASSERT(!c->no_udp); @@ -115,13 +115,7 @@ static int udp_vu_sock_recv(const struct ctx *c, struct vu_virtq *vq, int s, iov_vu[0].iov_base = (char *)iov_vu[0].iov_base - hdrlen; iov_vu[0].iov_len += hdrlen; - /* count the numbers of buffer filled by recvmsg() */ - idx = iov_skip_bytes(iov_vu, iov_cnt, *dlen + hdrlen, &off); - - /* adjust last iov length */ - if (idx < iov_cnt) - iov_vu[idx].iov_len = off; - iov_used = idx + !!off; + iov_used = iov_truncate(iov_vu, iov_cnt, *dlen + hdrlen); /* pad frame to 60 bytes: first buffer is at least ETH_ZLEN long */ l2len = *dlen + hdrlen - VNET_HLEN; -- 2.53.0

2 1

[PATCH v2] iov: Add iov_truncate() helper and use it in vu handlers
by Laurent Vivier 05 Mar '26

05 Mar '26

Add a generic iov_truncate() function that truncates an IO vector to a given number of bytes, returning the number of iov entries that contain data after truncation. Use it in udp_vu_sock_recv() and tcp_vu_sock_recv() to replace the open-coded truncation logic that adjusted iov entries after recvmsg(). Signed-off-by: Laurent Vivier <lvivier(a)redhat.com> --- Notes: v2: use iov_truncate() in udp_vu_sock_recv() too iov.c | 22 ++++++++++++++++++++++ iov.h | 1 + tcp_vu.c | 12 ++---------- udp_vu.c | 12 +++--------- 4 files changed, 28 insertions(+), 19 deletions(-) diff --git a/iov.c b/iov.c index ad726daa4cd8..31a3f5bc29e5 100644 --- a/iov.c +++ b/iov.c @@ -147,6 +147,28 @@ size_t iov_size(const struct iovec *iov, size_t iov_cnt) return len; } +/** + * iov_truncate() - Truncate an IO vector to a given number of bytes + * @iov: IO vector (modified) + * @iov_cnt: Number of entries in @iov + * @size: Total number of bytes to keep + * + * Return: number of iov entries that contain data after truncation + */ +size_t iov_truncate(struct iovec *iov, size_t iov_cnt, size_t size) +{ + size_t i, offset; + + i = iov_skip_bytes(iov, iov_cnt, size, &offset); + + if (i < iov_cnt) { + iov[i].iov_len = offset; + i += !!offset; + } + + return i; +} + /** * iov_tail_prune() - Remove any unneeded buffers from an IOV tail * @tail: IO vector tail (modified) diff --git a/iov.h b/iov.h index d1ab91a94e22..b4e50b0fca5a 100644 --- a/iov.h +++ b/iov.h @@ -29,6 +29,7 @@ size_t iov_from_buf(const struct iovec *iov, size_t iov_cnt, size_t iov_to_buf(const struct iovec *iov, size_t iov_cnt, size_t offset, void *buf, size_t bytes); size_t iov_size(const struct iovec *iov, size_t iov_cnt); +size_t iov_truncate(struct iovec *iov, size_t iov_cnt, size_t size); /* * DOC: Theory of Operation, struct iov_tail diff --git a/tcp_vu.c b/tcp_vu.c index 88be232dca66..15405a26c7c3 100644 --- a/tcp_vu.c +++ b/tcp_vu.c @@ -192,9 +192,9 @@ static ssize_t tcp_vu_sock_recv(const struct ctx *c, struct vu_virtq *vq, struct msghdr mh_sock = { 0 }; uint16_t mss = MSS_GET(conn); int s = conn->sock; - ssize_t ret, len; size_t hdrlen; int elem_cnt; + ssize_t ret; int i; *iov_cnt = 0; @@ -247,15 +247,7 @@ static ssize_t tcp_vu_sock_recv(const struct ctx *c, struct vu_virtq *vq, ret -= already_sent; /* adjust iov number and length of the last iov */ - len = ret; - for (i = 0; len && i < elem_cnt; i++) { - struct iovec *iov = &elem[i].in_sg[0]; - - if (iov->iov_len > (size_t)len) - iov->iov_len = len; - - len -= iov->iov_len; - } + i = iov_truncate(&iov_vu[DISCARD_IOV_NUM], elem_cnt, ret); /* adjust head count */ while (*head_cnt > 0 && head[*head_cnt - 1] >= i) diff --git a/udp_vu.c b/udp_vu.c index 3520f89e5671..5effca777e0a 100644 --- a/udp_vu.c +++ b/udp_vu.c @@ -71,9 +71,9 @@ static int udp_vu_sock_recv(const struct ctx *c, struct vu_virtq *vq, int s, bool v6, ssize_t *dlen) { const struct vu_dev *vdev = c->vdev; - int iov_cnt, idx, iov_used; - size_t off, hdrlen, l2len; struct msghdr msg = { 0 }; + int iov_cnt, iov_used; + size_t hdrlen, l2len; ASSERT(!c->no_udp); @@ -115,13 +115,7 @@ static int udp_vu_sock_recv(const struct ctx *c, struct vu_virtq *vq, int s, iov_vu[0].iov_base = (char *)iov_vu[0].iov_base - hdrlen; iov_vu[0].iov_len += hdrlen; - /* count the numbers of buffer filled by recvmsg() */ - idx = iov_skip_bytes(iov_vu, iov_cnt, *dlen + hdrlen, &off); - - /* adjust last iov length */ - if (idx < iov_cnt) - iov_vu[idx].iov_len = off; - iov_used = idx + !!off; + iov_used = iov_truncate(iov_vu, iov_cnt, *dlen + hdrlen); /* pad frame to 60 bytes: first buffer is at least ETH_ZLEN long */ l2len = *dlen + hdrlen - VNET_HLEN; -- 2.53.0

1 0

Pesto protocol proposals
by David Gibson 05 Mar '26

05 Mar '26

Most of today and yesterday I've spent thinking about the dynamic update model and protocol. I certainly don't have all the details pinned down, let alone any implementation, but I have come to some conclusions. # Shadow forward table On further consideration, I think this is a bad idea. To avoid peer visible disruption, we don't want to destroy and recreate listening sockets that are associated with a forward rule that's not being altered. Doing that with a shadow table would mean we'd need to essentially diff the two tables as we switch. That seems moderately complex, and kind of silly when then client almost certainly have created the shadow table using specific adds/removes from the original table. # Rule states / active bit I think we *do* still want two stage activation of new rules: if the first stage fails we're guaranteed we can roll back with no peer visible consequences. The second stage (the actual bind()s and listen()s) doesn't have that property, but that's unavoidable. To implement that, I think each rule should have an "active" bit. Or, at least an active bit - it's possible that might not be enough, but we could extend it to a state field, loosely analagous to the state field in flow table entries. But let's assume just the active bit, until/unless a case shows up where it's insufficient. Entries are always inserted in inactive state. Entries must be moved to inactive state before deletion. fwd_listen_sync() would ignore inactive entries. Turning the active flag on triggers the actual bind() / listen() calls, but requires no rearrangement of the fwd table or socket array. Turning it off close()s the associated sockets, but again requires no rearrangement of the data structures. # Tentative client operations ## INSERT Parameters: rule specification + rule index Returns: error status Inserts the new rule (inactive) at the given index (moving later rules, if necessary). Fails with no effect, for a bad index or if there's no room in the table or socket array. Does *not* check for conflicts with other rules. NOTE: moving rules could mean thousands of epoll_ctl() calls to adjust rule indices. We don't expect those to fail, but if they did, what do we do? ## DELETE Parameters: rule index Returns: error status Deletes the given inactive rule (moving later rules, if necessary). Fails with no effect if it's a bad index, or the given rule is active. NOTE: As for INSERT ## ACTIVATE Parameters: rule index Returns: error status Enables the rule, bind()ing all the necessary listening sockets. Fails with no effect if the rule conflicts with another active rule. Completes, even if some bind()s fail (see later for handling of this). ## DEACTIVATE Parameters: rule index Returns: error status Disables the rule, close()ing any listening sockets. Fails with no effect for bad index or an already inactive rule. ## STATUS Parameters: rule index Returns: active/inactive bit + possible metadata Indicates whether the rule is currently active. Could also give limited metadata about the rule (see below for possible use in bind() error reporting). ## READ Parameters: rule index Returns: rule specification, or error code Reads out the rule spec and returns it. Fails for bad index. To dump the whole table, the client can READ each slot starting from 0, until it gets an error. # Suggested client workflow I suggest the client should: 1. Parse all rule modifications 2. INSERT all new rules -> On error, DELETE them again 3. DEACTIVATE all removed rules -> Should only fail if the client has done something wrong 4. ACTIVATE all new rules -> On error (rule conflict): DEACTIVATE rules we already ACTIVATEd ACTIVATE rules we already DEACTIVATEd DELETE rules we INSERTed 5. Check for bind errors (see details later) If there are failures we can't tolerate: DEACTIVATE rules we already ACTIVATEd ACTIVATE rules we already DEACTIVATEd DELETE rules we INSERTed 6. DELETE rules we DEACTIVATEd -> Should only fail if the client has done something wrong DEACTIVATE comes before ACTIVATE to avoid spurious conflicts between new rules and rules we're deleting. I think that gets us closeish to "as atomic as we can be", at least from the perspective of peers. The main case it doesn't catch is that we don't detect rule conflicts until after we might have removed some rules. Is that good enough? # Bind error handling Note, in the below I'm considering pasta/passt's command line handling and conf path as a client rather than part of the backend. The distinction weak and non-weak entries is a bit clunky. How many failures is too many is kind of a question for the client, not the backend. So I'm suggesting we remove that concept from the backend. ACTIVATE completes even if some or all binds fail. However, we keep a count of how many sockets we got a bind() or listen() failure for in the rule, and it can be retrieved with STATUS. That lets the client make a decision as to whether to live with it or roll back as best it can. A client could also potentially poll later to see if some failures resolved themselves (we now reattempt bind()s on every fwd_listen_sync()). NOTE: the meaning of that count is pretty straightforward with regular rules, but with SCAN rules, we'd have to be more careful. # Concurrent updates I suggest we prevent concurrent updates by only allowing one client to connect to the control socket at a time. # Possible tweaks Not sure if these are improvements or not, but they're options to consider. ## Rule conflicts Currently fwd_rule_add() checks for rules with conflicts and rejects them. We can't really report that at INSERT, because we could get a bogus conflict with a rule we intend to DEACTIVATE/DELETE. But reporting at ACTIVATE is also a bit clunky. This could potentially be sidestepped by removing the notion of rule conflicts entirely. Instead overlapping rules are simply allowed, and the first rule to match a flow wins. ## Rollback in backend The proposal above has rollback essentially handled by the client. We could instead do it in the backend. - Instead of a single active bit, each rule has an "active now" and "active future" bit - On client connect, all active future bits are set equal to active now bits - INSERT adds a rule with active now false and active future true - If we conflict check, we check only against active future rules - DELETE clears the active future bit - ACTIVATE/DEACTIVATE no longer exist - ROLLBACK deletes all !active now rules and sets active future bits to active now bits again - COMMIT does the bind()s and close()s and on success, sets active now bits to active future bits. On failure... it's fairly complex, we'd need to think about it ## Persistent rule IDs Proposal above uses raw indices in the table to identify rules, which means INSERT and DELETE change the numbers of other rules. That in turn requires a bunch of epoll_ctl()s to update existing sockets. Here's one way we could avoid some of that with a persistent rule ID: - Each rule has an ID (say a u32), supplied by the client at INSERT - Rules still apply in ID order, so the order matters, but not the exact values - INSERT to an existing rule ID is not permitted - you must DELETE first Internally we still store the table packed, but sorted by ID. We could look up rule by ID either with a binary search, or maybe a radix lookup table. If we reduced the ID to a u16 (or so) we could potentially use a single level lookup table. I suspect binary search might be faster than a lookup table anyway, because of dcache impact. So, we still to memmove() things about for INSERT, and maybe update the lookup table, but that's relatively easy. epoll data holds the persistent ID, so that doesn't need to be altered. Clients (including the internal conf path) could choose to leave gaps in the IDs to leave space for future inserts. And/or certain ranges could be reserved by convention for different purposes. -- 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

2 2

[PATCH] tcp: Avoid comparison of expressions with different signedness in RTT_SET()
by Stefano Brivio 05 Mar '26

05 Mar '26

With gcc 14.2, building against musl 1.2.5 (slightly outdated Alpine on x86_64): tcp.c: In function 'tcp_update_seqack_wnd': util.h:40:39: warning: comparison of integer expressions of different signedness: 'unsigned int' and 'int' [-Wsign-compare] 40 | #define MIN(x, y) (((x) < (y)) ? (x) : (y)) | ^ tcp_conn.h:63:26: note: in expansion of macro 'MIN' 63 | (conn->rtt_exp = MIN(RTT_EXP_MAX, ilog2(MAX(1, rtt / RTT_STORE_MIN)))) | ^~~ tcp.c:1234:17: note: in expansion of macro 'RTT_SET' 1234 | RTT_SET(conn, tinfo->tcpi_rtt); | ^~~~~~~ util.h:40:54: warning: operand of '?:' changes signedness from 'int' to 'unsigned int' due to unsignedness of other operand [-Wsign-compare] 40 | #define MIN(x, y) (((x) < (y)) ? (x) : (y)) | ^~~ tcp_conn.h:63:26: note: in expansion of macro 'MIN' 63 | (conn->rtt_exp = MIN(RTT_EXP_MAX, ilog2(MAX(1, rtt / RTT_STORE_MIN)))) | ^~~ tcp.c:1234:17: note: in expansion of macro 'RTT_SET' 1234 | RTT_SET(conn, tinfo->tcpi_rtt); | ^~~~~~~ for some reason, that's not reported by gcc with glibc. Cast the result of ilog2() to unsigned before using it, as it's always positive the way we're using it. Should this ever break this for whatever unlikely reason, RTT_EXP_MAX is the fallback value we want to use anyway. Fixes: 000601ba86da ("tcp: Adaptive interval based on RTT for socket-side acknowledgement checks") Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- tcp_conn.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tcp_conn.h b/tcp_conn.h index b7b85c1..6418ac4 100644 --- a/tcp_conn.h +++ b/tcp_conn.h @@ -60,7 +60,8 @@ struct tcp_tap_conn { #define RTT_STORE_MIN 100 /* us, minimum representable */ #define RTT_STORE_MAX ((long)(RTT_STORE_MIN << RTT_EXP_MAX)) #define RTT_SET(conn, rtt) \ - (conn->rtt_exp = MIN(RTT_EXP_MAX, ilog2(MAX(1, rtt / RTT_STORE_MIN)))) + (conn->rtt_exp = MIN(RTT_EXP_MAX, \ + (unsigned)ilog2(MAX(1, rtt / RTT_STORE_MIN)))) #define RTT_GET(conn) (RTT_STORE_MIN << conn->rtt_exp) bool tap_inactive :1; -- 2.43.0

2 1

[PATCH] tcp: Avoid comparison of expressions with different signedness in tcp_timer_handler()
by Stefano Brivio 05 Mar '26

05 Mar '26

With gcc 14.2, building against musl 1.2.5 (slightly outdated Alpine on x86_64): tcp.c: In function 'tcp_timer_handler': util.h:40:39: warning: comparison of integer expressions of different signedness: 'unsigned int' and 'int' [-Wsign-compare] 40 | #define MIN(x, y) (((x) < (y)) ? (x) : (y)) | ^ tcp.c:2593:31: note: in expansion of macro 'MIN' 2593 | max = MIN(TCP_MAX_RETRIES, max); | ^~~ util.h:40:54: warning: operand of '?:' changes signedness from 'int' to 'unsigned int' due to unsignedness of other operand [-Wsign-compare] 40 | #define MIN(x, y) (((x) < (y)) ? (x) : (y)) | ^~~ tcp.c:2593:31: note: in expansion of macro 'MIN' 2593 | max = MIN(TCP_MAX_RETRIES, max); | ^~~ for some reason, that's not reported by gcc with glibc. Make the temporary 'max' variable unsigned, as we know it can't be negative anyway. While at it, add the customary blank line between variable declarations and code. Fixes: 3dde0e07804e ("tcp: Update data retransmission timeout") Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- tcp.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tcp.c b/tcp.c index 88ad886..68c92da 100644 --- a/tcp.c +++ b/tcp.c @@ -2588,7 +2588,8 @@ void tcp_timer_handler(const struct ctx *c, union epoll_ref ref) tcp_timer_ctl(c, conn); } else if (conn->flags & ACK_FROM_TAP_DUE) { if (!(conn->events & ESTABLISHED)) { - int max; + unsigned int max; + max = c->tcp.syn_retries + c->tcp.syn_linear_timeouts; max = MIN(TCP_MAX_RETRIES, max); if (conn->retries >= max) { -- 2.43.0

2 1

[PATCH] migrate: Rename v1 address functions to v2 for clarity
by Jon Maloy 05 Mar '26

05 Mar '26

Some migration address structures and functions have a _v1 suffix. This is confusing, since they are currently handling version 2 of the migration protocol. We are soon going to introduce a new version 3 of the protocol, so we choose to give these functions the correct suffix _v2 instead. This is in correspondence with current reality, and will help make a clearer distinction between the old and the new versions of those functions. Signed-off-by: Jon Maloy <jmaloy(a)redhat.com> Reviewed-by: David Gibson <david(a)gibson.dropbear.id.au> --- migrate.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/migrate.c b/migrate.c index 13bacab..1e8858a 100644 --- a/migrate.c +++ b/migrate.c @@ -29,13 +29,13 @@ #define MIGRATE_MAGIC 0xB1BB1D1B0BB1D1B0 /** - * struct migrate_seen_addrs_v1 - Migratable guest addresses for v1 state stream + * struct migrate_seen_addrs_v2 - Migratable guest addresses for v2 state stream * @addr6: Observed guest IPv6 address * @addr6_ll: Observed guest IPv6 link-local address * @addr4: Observed guest IPv4 address * @mac: Observed guest MAC address */ -struct migrate_seen_addrs_v1 { +struct migrate_seen_addrs_v2 { struct in6_addr addr6; struct in6_addr addr6_ll; struct in_addr addr4; @@ -43,7 +43,7 @@ struct migrate_seen_addrs_v1 { } __attribute__((packed)); /** - * seen_addrs_source_v1() - Copy and send guest observed addresses from source + * seen_addrs_source_v2() - Copy and send guest observed addresses from source * @c: Execution context * @stage: Migration stage, unused * @fd: File descriptor for state transfer @@ -51,10 +51,10 @@ struct migrate_seen_addrs_v1 { * Return: 0 on success, positive error code on failure */ /* cppcheck-suppress [constParameterCallback, unmatchedSuppression] */ -static int seen_addrs_source_v1(struct ctx *c, +static int seen_addrs_source_v2(struct ctx *c, const struct migrate_stage *stage, int fd) { - struct migrate_seen_addrs_v1 addrs = { + struct migrate_seen_addrs_v2 addrs = { .addr6 = c->ip6.addr_seen, .addr6_ll = c->ip6.addr_ll_seen, .addr4 = c->ip4.addr_seen, @@ -71,17 +71,17 @@ static int seen_addrs_source_v1(struct ctx *c, } /** - * seen_addrs_target_v1() - Receive and use guest observed addresses on target + * seen_addrs_target_v2() - Receive and use guest observed addresses on target * @c: Execution context * @stage: Migration stage, unused * @fd: File descriptor for state transfer * * Return: 0 on success, positive error code on failure */ -static int seen_addrs_target_v1(struct ctx *c, +static int seen_addrs_target_v2(struct ctx *c, const struct migrate_stage *stage, int fd) { - struct migrate_seen_addrs_v1 addrs; + struct migrate_seen_addrs_v2 addrs; (void)stage; @@ -100,8 +100,8 @@ static int seen_addrs_target_v1(struct ctx *c, static const struct migrate_stage stages_v2[] = { { .name = "observed addresses", - .source = seen_addrs_source_v1, - .target = seen_addrs_target_v1, + .source = seen_addrs_source_v2, + .target = seen_addrs_target_v2, }, { .name = "prepare flows", -- 2.52.0

3 2

[PATCH] vu_common: Always set num_buffers in virtio-net header
by Laurent Vivier 05 Mar '26

05 Mar '26

Legacy virtio used two different header formats: struct virtio_net_hdr (10 bytes) when VIRTIO_NET_F_MRG_RXBUF was not negotiated, and struct virtio_net_hdr_mrg_rxbuf (12 bytes) when it was. The num_buffers field only existed in the larger header. Modern virtio (VIRTIO_F_VERSION_1, i.e. virtio 1.0+) always uses the 12-byte struct virtio_net_hdr_mrg_rxbuf header regardless of whether VIRTIO_NET_F_MRG_RXBUF is negotiated, so num_buffers is always present in the header. passt only supports modern virtio and dies if VIRTIO_F_VERSION_1 is not negotiated (vhost_user.c), and VNET_HLEN is unconditionally defined as sizeof(struct virtio_net_hdr_mrg_rxbuf). The virtio specification (v1.1, section 5.1.6) requires that: "The device MUST set num_buffers to 1 if VIRTIO_NET_F_MRG_RXBUF has not been negotiated." vu_set_vnethdr() only set num_buffers when VIRTIO_NET_F_MRG_RXBUF was negotiated. When it was not, num_buffers was left uninitialised, violating the spec. Since vu_collect() already limits buffer collection to a single element when VIRTIO_NET_F_MRG_RXBUF is not negotiated, num_buffers passed by callers is guaranteed to be 1 in that case. We can therefore unconditionally set num_buffers, which makes the vdev parameter unnecessary. Drop the vdev parameter from vu_set_vnethdr() and update all callers. Signed-off-by: Laurent Vivier <lvivier(a)redhat.com> --- tcp_vu.c | 4 ++-- udp_vu.c | 2 +- vu_common.c | 13 ++++++------- vu_common.h | 4 +--- 4 files changed, 10 insertions(+), 13 deletions(-) diff --git a/tcp_vu.c b/tcp_vu.c index bb05fbf45826..88be232dca66 100644 --- a/tcp_vu.c +++ b/tcp_vu.c @@ -97,7 +97,7 @@ int tcp_vu_send_flag(const struct ctx *c, struct tcp_tap_conn *conn, int flags) ASSERT(flags_elem[0].in_sg[0].iov_len >= MAX(hdrlen + sizeof(*opts), ETH_ZLEN + VNET_HLEN)); - vu_set_vnethdr(vdev, flags_elem[0].in_sg[0].iov_base, 1); + vu_set_vnethdr(flags_elem[0].in_sg[0].iov_base, 1); eh = vu_eth(flags_elem[0].in_sg[0].iov_base); @@ -452,7 +452,7 @@ int tcp_vu_data_from_sock(const struct ctx *c, struct tcp_tap_conn *conn) bool push = i == head_cnt - 1; size_t l2len; - vu_set_vnethdr(vdev, iov->iov_base, buf_cnt); + vu_set_vnethdr(iov->iov_base, buf_cnt); /* The IPv4 header checksum varies only with dlen */ if (previous_dlen != dlen) diff --git a/udp_vu.c b/udp_vu.c index 51f3718f5925..3520f89e5671 100644 --- a/udp_vu.c +++ b/udp_vu.c @@ -127,7 +127,7 @@ static int udp_vu_sock_recv(const struct ctx *c, struct vu_virtq *vq, int s, l2len = *dlen + hdrlen - VNET_HLEN; vu_pad(&iov_vu[0], l2len); - vu_set_vnethdr(vdev, iov_vu[0].iov_base, iov_used); + vu_set_vnethdr(iov_vu[0].iov_base, iov_used); /* release unused buffers */ vu_queue_rewind(vq, iov_cnt - iov_used); diff --git a/vu_common.c b/vu_common.c index aa14598ea028..5f2ce18e5b71 100644 --- a/vu_common.c +++ b/vu_common.c @@ -121,17 +121,16 @@ int vu_collect(const struct vu_dev *vdev, struct vu_virtq *vq, /** * vu_set_vnethdr() - set virtio-net headers - * @vdev: vhost-user device * @vnethdr: Address of the header to set * @num_buffers: Number of guest buffers of the frame */ -void vu_set_vnethdr(const struct vu_dev *vdev, - struct virtio_net_hdr_mrg_rxbuf *vnethdr, - int num_buffers) +void vu_set_vnethdr(struct virtio_net_hdr_mrg_rxbuf *vnethdr, int num_buffers) { vnethdr->hdr = VU_HEADER; - if (vu_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF)) - vnethdr->num_buffers = htole16(num_buffers); + /* Note: if VIRTIO_NET_F_MRG_RXBUF is not negotiated, + * num_buffers must be 1 + */ + vnethdr->num_buffers = htole16(num_buffers); } /** @@ -269,7 +268,7 @@ int vu_send_single(const struct ctx *c, const void *buf, size_t size) goto err; } - vu_set_vnethdr(vdev, in_sg[0].iov_base, elem_cnt); + vu_set_vnethdr(in_sg[0].iov_base, elem_cnt); total -= VNET_HLEN; diff --git a/vu_common.h b/vu_common.h index 052aff710502..20868a7f62ce 100644 --- a/vu_common.h +++ b/vu_common.h @@ -49,9 +49,7 @@ void vu_init_elem(struct vu_virtq_element *elem, struct iovec *iov, int vu_collect(const struct vu_dev *vdev, struct vu_virtq *vq, struct vu_virtq_element *elem, int max_elem, size_t size, size_t *collected); -void vu_set_vnethdr(const struct vu_dev *vdev, - struct virtio_net_hdr_mrg_rxbuf *vnethdr, - int num_buffers); +void vu_set_vnethdr(struct virtio_net_hdr_mrg_rxbuf *vnethdr, int num_buffers); void vu_flush(const struct vu_dev *vdev, struct vu_virtq *vq, struct vu_virtq_element *elem, int elem_cnt); void vu_kick_cb(struct vu_dev *vdev, union epoll_ref ref, -- 2.53.0

3 2

[PATCH 1/3] tcp: Use flow_foreach_of_type() in tcp_{keepalive,inactivity}
by David Gibson 05 Mar '26

05 Mar '26

These functions step through all TCP flows. I forgot there was already a flow_foreach_of_type() macro which makes this easier. Use it. Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- tcp.c | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/tcp.c b/tcp.c index 88ad886d..16470703 100644 --- a/tcp.c +++ b/tcp.c @@ -2907,12 +2907,9 @@ static void tcp_keepalive(struct ctx *c, const struct timespec *now) c->tcp.keepalive_run = now->tv_sec; - flow_foreach(flow) { + flow_foreach_of_type(flow, FLOW_TCP) { struct tcp_tap_conn *conn = &flow->tcp; - if (flow->f.type != FLOW_TCP) - continue; - if (conn->tap_inactive) { flow_dbg(conn, "No tap activity for least %us, send keepalive", KEEPALIVE_INTERVAL); @@ -2938,12 +2935,9 @@ static void tcp_inactivity(struct ctx *c, const struct timespec *now) debug("TCP inactivity scan"); c->tcp.inactivity_run = now->tv_sec; - flow_foreach(flow) { + flow_foreach_of_type(flow, FLOW_TCP) { struct tcp_tap_conn *conn = &flow->tcp; - if (flow->f.type != FLOW_TCP) - continue; - if (conn->inactive) { /* No activity in this interval, reset */ flow_dbg(conn, "Inactive for at least %us, resetting", -- 2.53.0

2 1

[PATCH 3/3] clang-tidy: Don't insist on #ifdef over #if defined()
by David Gibson 05 Mar '26

05 Mar '26

Commit 036fb8770 ("checksum: add VSX fast path for POWER8/POWER9") changed an #ifdef to #if defined, in order to match a newly introduced #if which needs to check two different symbols. This causes clang-tidy to complain that the directive could be written more concisely. In this case consistency with the other #if branch seems more important, and in general insisting on #ifdef over #if seems unhelpfully pedantic. Suppress that warning globally. Signed-off-by: David Gibson <david(a)gibson.dropbear.id.au> --- .clang-tidy | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.clang-tidy b/.clang-tidy index 9d346ec2..773121f5 100644 --- a/.clang-tidy +++ b/.clang-tidy @@ -81,6 +81,11 @@ Checks: # precedence over addition in modern mathematical notation. Adding # parentheses to reinforce that certainly won't improve readability. - "-readability-math-missing-parentheses" + + # #if defined(FOO) is fine, and can be more consistent with other + # #if directives. Don't insist on #ifdef instead. + - "-readability-use-concise-preprocessor-directives" + WarningsAsErrors: "*" HeaderFileExtensions: - h -- 2.53.0

2 1