On Wed, 26 Feb 2025 20:05:25 +1100 David Gibson <david(a)gibson.dropbear.id.au> wrote:Amongst other things, I spotted some additional complications using tcp_rst() in the migration path (some of which might also have implications in other contexts). These might be things we can safely ignore, at least for now, but I haven't thought through them enough to be sure. 1) Sending RST to guest during migration The first issue is that tcp_rst() will send an actual RST to the guest on the tap interface. During migration, that means we're sending to the guest while it's suspended. At the very least that means we probably have a much higher that usual chance of getting a queue full failure writing to the tap interface, which could hit problem (2). But, beyond that, with vhost-user that means we're writing to guest memory while the guest is suspended. Kind of the whole point of the suspended time is that the guest memory doesn't change during it, so I'm not sure what the consequences will be.If I recall correctly I checked this and something in the vhost-user code will tell us that the queue is not ready yet, done. Ideally we want to make sure we queue those, but queue sizes are finite and I don't think we can guarantee we can pile up 128k RST segments. Right now I would check that the functionality is not spectacularly broken (I looked into that briefly, QEMU didn't crash, guest kept running, but I didn't check further than that). If we miss a RST too bad, things will time out eventually. As a second step we could perhaps introduce a post-migration stage and move calling tcp_rst() to there if the connection is in a given state?Now, at the moment I think all our tcp_rst() calls are either on the source during rollback (i.e. we're committed to resuming only on the source) or on the target past the point of no return (i.e. we're committed to resuming only on the target). I suspect that means we can get away with it, but I do worry this could break something in qeme by violating that assumption. 2) tcp_rst() failures tcp_rst() can fail if tcp_send_flag() fails. In this case we *don't* change the events to CLOSED. I _think_ that's a bug: even if we weren't able to send the RST to the guest, we've already closed the socket so the flow is dead. Moving to CLOSED state (and then removing the flow entirely) should mean that we'll resend an RST if the guest attempts to use the flow again later. But.. I was worried there might be some subtle reason for not changing the event state in that case.Not very subtle: my original idea was that if we fail to send the RST, we should note that (by not moving to CLOSED) and try again from a timer. In practice I've never observed a tcp_send_flag() failure so I'm not sure if that mechanism even works. Moving the socket to CLOSED sounds totally okay to me, surely simpler, and probably more robust. -- Stefano