Re: [Qemu-devel] [PATCH 3/3] hw/char/serial: Don't retry on serial_xmit if errno == EPIPE

["Daniel P. Berrangé" <berrange@redhat.com>]

On Thu, May 31, 2018 at 11:52:01AM +0200, Marc-André Lureau wrote:
> Hi
> 
> On Thu, May 31, 2018 at 9:46 AM, Sergio Lopez <slp@redhat.com> wrote:
> > If writing to the frontend channel failed with EPIPE, don't set up a
> > retry. EPIPE is not a recoverable error, so trying again is a waste of CPU
> > cycles.
> >
> > If the vCPU writing to the serial device and emulator thread are pinned
> > to the same pCPU, it can also compromise the stability of the Guest OS,
> > as both threads will be competing for pCPU's time, with the vCPU
> > actively polling the serial device and barely giving time to the
> > emulator thread to make actual progress.
> > ---
> >  hw/char/serial.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/hw/char/serial.c b/hw/char/serial.c
> > index 2c080c9..f26e86b 100644
> > --- a/hw/char/serial.c
> > +++ b/hw/char/serial.c
> > @@ -262,6 +262,7 @@ static void serial_xmit(SerialState *s)
> >              /* in loopback mode, say that we just received a char */
> >              serial_receive1(s, &s->tsr, 1);
> >          } else if (qemu_chr_fe_write(&s->chr, &s->tsr, 1) != 1 &&
> > +                   errno != EPIPE &&
> >                     s->tsr_retry < MAX_XMIT_RETRY) {
> 
> Instead of adding explicit handling of EPIPE, shouldn't the code be
> rewritten to treat -1 return && errno != EAGAIN as fatal?

Yes, exactly this code is already broken for every single errno
value, not simply EPIPE. It needs fixing to treat '-1' return code
correctly instead of retrying everything.

> 
> >              assert(s->watch_tag == 0);
> >              s->watch_tag =

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|