Re: [PATCH 2/3] chardev: report blocked write to chardev backend

[Alex Bennée]

Marc-André Lureau <marcandre.lureau@redhat.com> writes:

> Hi
>
> On Tue, Nov 21, 2023 at 1:45 PM Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 21/11/2023 10.39, Marc-André Lureau wrote:
>> > Hi
>> >
>> > On Mon, Nov 20, 2023 at 5:36 PM Nicholas Piggin <npiggin@gmail.com> wrote:
>> >>
>> >> On Mon Nov 20, 2023 at 10:06 PM AEST, Marc-André Lureau wrote:
>> >>> Hi
>> >>>
>> >>> On Thu, Nov 16, 2023 at 3:54 PM Nicholas Piggin <npiggin@gmail.com> 
>> >>> wrote:
>> >>>>
>> >>>> If a chardev socket is not read, it will eventually fill and QEMU
>> >>>> can block attempting to write to it. A difficult bug in avocado
>> >>>> tests where the console socket was not being read from caused this
>> >>>> hang.
>> >>>>
>> >>>> warn if a chardev write is blocked for 100ms.
>> >>>>
>> >>>> Signed-off-by: Nicholas Piggin <npiggin@gmail.com>
<snip>
>> >>>> index 996a024c7a..7c375e3cc4 100644
>> >>>> --- a/chardev/char.c
>> >>>> +++ b/chardev/char.c
>> >>>> @@ -114,6 +114,8 @@ static int qemu_chr_write_buffer(Chardev *s,
>> >>>>   {
>> >>>>       ChardevClass *cc = CHARDEV_GET_CLASS(s);
>> >>>>       int res = 0;
>> >>>> +    int nr_retries = 0;
>> >>>> +
>> >>>>       *offset = 0;
>> >>>>
>> >>>>       qemu_mutex_lock(&s->chr_write_lock);
>> >>>> @@ -126,6 +128,10 @@ static int qemu_chr_write_buffer(Chardev *s,
>> >>>>               } else {
>> >>>>                   g_usleep(100);
>> >>>>               }
>> >>>> +            if (++nr_retries == 1000) { /* 100ms */
>> >>>> +                warn_report("Chardev '%s' write blocked for > 100ms, "
>> >>>> +                            "socket buffer full?", s->label);
>> >>>> +            }
>> >>>
>> >>> That shouldn't happen, the frontend should poll and only write when it
>> >>> can. What is the qemu command being used here?
<snip>
>
> Ok so the "frontend" is spapr-vty and there:
>
> void vty_putchars(SpaprVioDevice *sdev, uint8_t *buf, int len)
> {
>     SpaprVioVty *dev = VIO_SPAPR_VTY_DEVICE(sdev);
>
>     /* XXX this blocks entire thread. Rewrite to use
>      * qemu_chr_fe_write and background I/O callbacks */
>     qemu_chr_fe_write_all(&dev->chardev, buf, len);
> }
>
> (grep "XXX this blocks", we have a lot...)
>
> Can H_PUT_TERM_CHAR return the number of bytes written?
>
> Is there a way to tell the guest the console is ready to accept more bytes?

See also:

  Message-ID: <20231109192814.95977-1-philmd@linaro.org>
  Date: Thu,  9 Nov 2023 20:28:04 +0100
  Subject: [PATCH-for-8.2 v4 00/10] hw/char/pl011: Implement TX (async) FIFO to 
avoid blocking the main loop
  From: =?UTF-8?q?Philippe=20Mathieu-Daud=C3=A9?= <philmd@linaro.org>

Although it didn't make it into 8.2. I was hoping it would be a template
for fixing up the other cases.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro