Re: [PATCH v3 2/4] python/qmp: increase read buffer size

[John Snow]

On Wed, Nov 9, 2022, 6:00 AM Daniel P. Berrangé <berrange@redhat.com> wrote:

On Wed, Nov 09, 2022 at 09:39:14AM +0000, Daniel P. Berrangé wrote:
> On Tue, Nov 08, 2022 at 03:38:21PM -0500, John Snow wrote:
> > On Thu, Nov 3, 2022 at 6:29 AM Maksim Davydov
> > <davydov-max@yandex-team.ru> wrote:
> > >
> > > After modification of "query-machines" command the buffer size should be
> > > more than 452kB to contain output with compat-props.
> > >
> > > Signed-off-by: Maksim Davydov <davydov-max@yandex-team.ru>
> > > Reviewed-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> > > ---
> > >  python/qemu/qmp/qmp_client.py | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/python/qemu/qmp/qmp_client.py b/python/qemu/qmp/qmp_client.py
> > > index 5dcda04a75..659fe4d98c 100644
> > > --- a/python/qemu/qmp/qmp_client.py
> > > +++ b/python/qemu/qmp/qmp_client.py
> > > @@ -197,8 +197,8 @@ async def run(self, address='/tmp/qemu.socket'):
> > >      #: Logger object used for debugging messages.
> > >      logger = logging.getLogger(__name__)
> > >
> > > -    # Read buffer limit; large enough to accept query-qmp-schema
> > > -    _limit = (256 * 1024)
> > > +    # Read buffer limit; large enough to accept query-machines
> > > +    _limit = (512 * 1024)
> >
> > wow :)
>
> Meanwhile over in python/qemu/qmp/protocol.py the read buffer limit is
> set to just 64 kb.

This one will override the other - the protocol limit is for any arbitrary full-duplex message-based protocol. It can stay at the lower limit.

(I used protocol.py to implement a qtest client as well, though I didn't upstream that piece. If there's interest, I will.)

>
> If the current output of a particular command is known to 450 kb, then
> setting this limit to 512 kb is waaaaaaay to conservative, and we'll
> inevitably have to change it again when someone finds the next command
> that overflows.
>
> Recall this thread
>
>    https://lists.gnu.org/archive/html/qemu-devel/2022-09/msg01060.html
>
> In fact, let me be the someone who demonstrates a real case where 512kb
> is not enough....

Another example...

Create a guest with 255 vCPUs (current RHEL downstream vCPU limit),
and run

  {"execute":"query-stats","arguments":{"target": "vcpu"}}

it'll get back a 0.38 MB  QMP reply.  RHEL raised the limit to 710
vCPUs, giving a little over 1 MB QMP reply. There is a strong desire
to go even higher. With 4096 vCPUs it'd get an ~6 MB QMP reply.

With 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 :|

You're right, of course. I recalled the thread but I was being lazy about it. (Sorry.) I thought, naively, that it was better to speed Maksim along for now.

I recall you (Daniel) stating that libvirt used a default of 10MB (iirc). I'd be happy to adopt that default as well, if only for parity.

Maksim, can I trouble you to send a revised patch as an MR to gitlab.com/qemu-project/python-qemu-qmp ? If not, a revised patch to the mailing list here is fine and with your permission I'll forward-port it over to the standalone repo myself. (Or I can just handle it entirely myself, if you'd prefer - just let me know.)

Sorry for the fuss, and thanks for helping to improve QMP testing and tooling

--js