Re: [PATCH] xen-hvm: Avoid livelock while handling buffered ioreqs

[Anthony PERARD <anthony.perard@xxxxxxxxx>]

On Thu, Apr 04, 2024 at 03:08:33PM +0100, Ross Lagerwall wrote:
> diff --git a/hw/xen/xen-hvm-common.c b/hw/xen/xen-hvm-common.c
> index 1627da739822..1116b3978938 100644
> --- a/hw/xen/xen-hvm-common.c
> +++ b/hw/xen/xen-hvm-common.c
> @@ -521,22 +521,30 @@ static bool handle_buffered_iopage(XenIOState *state)
[...]
>  
>  static void handle_buffered_io(void *opaque)
>  {
> +    unsigned int handled;
>      XenIOState *state = opaque;
>  
> -    if (handle_buffered_iopage(state)) {
> +    handled = handle_buffered_iopage(state);
> +    if (handled >= IOREQ_BUFFER_SLOT_NUM) {
> +        /* We handled a full page of ioreqs. Schedule a timer to continue
> +         * processing while giving other stuff a chance to run.
> +         */

./scripts/checkpatch.pl report a style issue here:
    WARNING: Block comments use a leading /* on a separate line

I can try to remember to fix that on commit.

>          timer_mod(state->buffered_io_timer,
> -                BUFFER_IO_MAX_DELAY + 
> qemu_clock_get_ms(QEMU_CLOCK_REALTIME));
> -    } else {
> +                qemu_clock_get_ms(QEMU_CLOCK_REALTIME));
> +    } else if (handled == 0) {

Just curious, why did you check for `handled == 0` here instead of
`handled != 0`? That would have avoided to invert the last 2 cases, and
the patch would just have introduce a new case without changing the
order of the existing ones. But not that important I guess.

>          timer_del(state->buffered_io_timer);
>          qemu_xen_evtchn_unmask(state->xce_handle, 
> state->bufioreq_local_port);
> +    } else {
> +        timer_mod(state->buffered_io_timer,
> +                BUFFER_IO_MAX_DELAY + 
> qemu_clock_get_ms(QEMU_CLOCK_REALTIME));
>      }
>  }

Cheers,

-- 
Anthony PERARD