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

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

On Tue, Apr 9, 2024 at 11:20 AM Anthony PERARD <anthony.perard@xxxxxxxxx> wrote:
>
> 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.

I copied the comment style from a few lines above but I guess it was
wrong.

>
> >          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.
>

In general I try to use conditionals with the least amount of negation
since I think it is easier to read. I can change it if you would prefer?

Ross