On Mon, 3 Feb 2025 19:59:12 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:On Mon, Feb 03, 2025 at 07:09:37AM +0100, Stefano Brivio wrote:Okay, fine, then let me add that back.On Mon, 3 Feb 2025 13:05:33 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:Eh... up to a point. I'm not sure about the kubevirt case, but in general when migrating amongst managed hosts it would not surprise me to find that the destination has multiple IPs. One is the "public" IP used by the workload and will indeed be the same between the ends. However, there could also be a "management" IP that's different between them - after all the management system will need to talk to both source and destination simultaneously for a little while.On Fri, Jan 31, 2025 at 08:39:51PM +0100, Stefano Brivio wrote:I had that in a previous version, because I was actually restoring it as I thought it was needed, then I dropped it. We expect the configuration of the target to be the same as the source,For migration only: we need to store 'oport', our socket-side port, as we establish a connection from the guest, so that we can bind the same oport as source port in the migration target. Use getsockname() to fetch that. Signed-off-by: Stefano Brivio <sbrivio(a)redhat.com> --- flow.c | 4 ++-- flow_table.h | 4 ++-- tcp.c | 24 +++++++++++++++++++++++- 3 files changed, 27 insertions(+), 5 deletions(-) diff --git a/flow.c b/flow.c index 5638ff1..506cbac 100644 --- a/flow.c +++ b/flow.c @@ -411,8 +411,8 @@ const struct flowside *flow_initiate_sa(union flow *flow, uint8_t pif, * * Return: pointer to the target flowside information */ -const struct flowside *flow_target(const struct ctx *c, union flow *flow, - uint8_t proto) +struct flowside *flow_target(const struct ctx *c, union flow *flow, + uint8_t proto) { char estr[INANY_ADDRSTRLEN], fstr[INANY_ADDRSTRLEN]; struct flow_common *f = &flow->f; diff --git a/flow_table.h b/flow_table.h index 633805d..b107107 100644 --- a/flow_table.h +++ b/flow_table.h @@ -178,8 +178,8 @@ const struct flowside *flow_target_af(union flow *flow, uint8_t pif, sa_family_t af, const void *saddr, in_port_t sport, const void *daddr, in_port_t dport); -const struct flowside *flow_target(const struct ctx *c, union flow *flow, - uint8_t proto); +struct flowside *flow_target(const struct ctx *c, union flow *flow, + uint8_t proto); union flow *flow_set_type(union flow *flow, enum flow_type type); #define FLOW_SET_TYPE(flow_, t_, var_) (&flow_set_type((flow_), (t_))->var_) diff --git a/tcp.c b/tcp.c index 0bd2a02..4fd405b 100644 --- a/tcp.c +++ b/tcp.c @@ -1471,6 +1471,8 @@ static void tcp_bind_outbound(const struct ctx *c, * @opts: Pointer to start of options * @optlen: Bytes in options: caller MUST ensure available length * @now: Current timestamp + * + * #syscalls:vu getsockname */ static void tcp_conn_from_tap(const struct ctx *c, sa_family_t af, const void *saddr, const void *daddr, @@ -1479,9 +1481,10 @@ static void tcp_conn_from_tap(const struct ctx *c, sa_family_t af, { in_port_t srcport = ntohs(th->source); in_port_t dstport = ntohs(th->dest); - const struct flowside *ini, *tgt; + const struct flowside *ini; struct tcp_tap_conn *conn; union sockaddr_inany sa; + struct flowside *tgt; union flow *flow; int s = -1, mss; uint64_t hash; @@ -1586,6 +1589,25 @@ static void tcp_conn_from_tap(const struct ctx *c, sa_family_t af, } tcp_epoll_ctl(c, conn); + + if (c->mode == MODE_VU) { /* To rebind to same oport after migration */I suspect we'll want this local side information in more places in future, but this is fine for now.+ if (af == AF_INET) { + struct sockaddr_in s_in; + socklen_t sl; + + sl = sizeof(s_in); + getsockname(s, (struct sockaddr *)&s_in, &sl); + tgt->oport = ntohs(s_in.sin_port);Since we're already doing the getsockname() we should also update tgt->oaddr, and that might matter in cases where the host has multiple local addresses.so the same connect() should yield to the same source address being used (minus the fact that we don't set socket options yet (point 9. of the to-do list from cover letter). So should we really bind() to a specific source address just because we happen to know it? I'm not quite sure.Yes, we should. Note that right now, the outbound socket already has a fixed bound address, assigned at connect() time. We don't track it in our internal data structures, it's still there as part of the socket state, and our behaviour reflects that. That forms part of the socket state which we should maintain across migration.We could do this lazily - one of the pre-migrate steps could be a getsockname() on any flows which have an unassigned oaddr/oport.Sure, on the other hand that's an optimisation, and I already have code ready for this. -- Stefano