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

[Ross Lagerwall <ross.lagerwall@xxxxxxxxxx>]

On Tue, Apr 9, 2024 at 3:19 PM Ross Lagerwall <ross.lagerwall@xxxxxxxxxx> wrote:
>
> 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?

It looks like this hasn't been committed anywhere. Were you expecting
an updated version with the style issue fixed or has it fallen through
the cracks?

Thanks,
Ross