Peter Xu <[email protected]> writes:
> 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()) {
There's no coroutine at this point, could replace this with an assert.
> + qemu_co_sleep_ns(QEMU_CLOCK_REALTIME, 1000000);
> + } else {
> + g_usleep(1000);
> + }
> }
> -
> - qio_channel_wait_cond(ioc, G_IO_IN);
> }
>
> return 0;
>
> Any preference?
>
> Thanks,
>
>>
>> > + /*
>> > + * QIO_CHANNEL_ERR_BLOCK also shouldn't happen, due to the
>> > + * prior peek just happened. We should be pretty sure we will
>> > + * read what we peeked, or the channel was broken.
>> > + */
>> > + if (len != received) {
>> > + return -1;
>> > + }
>> > + iov.iov_base += received;
>> > + iov.iov_len = buflen - received;
>> > + }
>> > }
>> >
>> > return 0;
>> > --
>> > 2.54.0
>> >
>>
>> 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 :|
>>
