On Tue, Jul 28, 2026 at 01:09:03PM -0400, Peter Xu wrote: > On Tue, Jul 28, 2026 at 05:24:36PM +0100, Daniel P. Berrangé wrote: > > On Tue, Jul 28, 2026 at 11:52:46AM -0400, Peter Xu wrote: > > > In an unlikely case, when a migration stream is attached to the > > > destination > > > QEMU and only send <4 bytes to the channel as magic, it's possible that > > > migration_channel_read_peek() may spin forever without yielding in the > > > main > > > thread causing two unwanted consequences: > > > > > > - CPU will spin 100% waiting for the rest bytes until it reaches 4 > > > - (more importantly..) Main thread is stuck during this process as the qio > > > operation won't really yield the coroutine > > > > > > Fix it by consuming the bytes that arrived. > > > > > > Resolves: https://gitlab.com/qemu-project/qemu/-/work_items/3889 > > > Cc: Daniel P. Berrangé <[email protected]> > > > Reported-by: Feifan Qian <[email protected]> > > > Signed-off-by: Peter Xu <[email protected]> > > > --- > > > migration/channel.c | 26 +++++++++++++++++++++++--- > > > 1 file changed, 23 insertions(+), 3 deletions(-) > > > > > > diff --git a/migration/channel.c b/migration/channel.c > > > index 1e2935f926..28fe1d2906 100644 > > > --- a/migration/channel.c > > > +++ b/migration/channel.c > > > @@ -294,11 +294,31 @@ int migration_channel_read_peek(QIOChannel *ioc, > > > return -1; > > > } > > > > > > - if (len == buflen) { > > > + if (len == iov.iov_len) { > > > break; > > > - } > > > + } else if (len == 0) { > > > + qio_channel_wait_cond(ioc, G_IO_IN); > > > + } else { > > > + ssize_t received = len; > > > > > > - qio_channel_wait_cond(ioc, G_IO_IN); > > > + /* > > > + * Partially arrived, read out to make > > > qio_channel_wait_cond() > > > + * won't return immediately, causing an unwanted spin on this > > > + * CPU. > > > + */ > > > + iov.iov_len = len; > > > + len = qio_channel_readv_full(ioc, &iov, 1, NULL, NULL, 0, > > > errp); > > > > Sure this breaks the API behaviour that the caller is expecting to > > see. > > > > migration_channel_identify will call migration_channel_read_peek > > to match the magic bytes. > > > > But something later in the flow will actually try to read the magic > > bytes. By consuming them in this migration_channel_read_peek > > method, surely we're breaking the code that wants to read the bytes > > later. > > Ah right, stupid me. The best then is for partial read we apply a manual > wait. > > I can also revert 604bb1badc ("migration: Properly wait on G_IO_IN when > peeking messages"), looping with 1ms interval for data isn't too bad. But > the best is we only do that for partial, so: > > diff --git a/migration/channel.c b/migration/channel.c > index 1e2935f926..88f89521f8 100644 > --- a/migration/channel.c > +++ b/migration/channel.c > @@ -296,9 +296,19 @@ int migration_channel_read_peek(QIOChannel *ioc, > > if (len == buflen) { > break; > + } else if (len == 0) { > + qio_channel_wait_cond(ioc, G_IO_IN); > + } else { > + /* > + * When partially ready, we can't use qio_channel_wait_cond() > + * because it will return immediately. Apply a manual wait. > + */ > + if (qemu_in_coroutine()) { > + qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 1000000); > + } else { > + g_usleep(1000); > + } > } > - > - qio_channel_wait_cond(ioc, G_IO_IN); > }
This approach looks best - a "normal" QEMU will never hit the sleep loop, so we'll only get this spin if the client is intentionally playing games by sending short packets. Reviewed-by: Daniel P. Berrangé <[email protected]> With regards, Daniel -- |: https://berrange.com ~~ https://hachyderm.io/@berrange :| |: https://libvirt.org ~~ https://entangle-photo.org :| |: https://pixelfed.art/berrange ~~ https://fstop138.berrange.com :|
