Re: [PATCH 2/2] monitor: increase amount of data for monitor to read

[Andrey Shinkevich]

On 09.11.2020 12:55, Vladimir Sementsov-Ogievskiy wrote:
> 06.11.2020 15:42, Andrey Shinkevich wrote:
> > QMP and HMP monitors read one byte at a time from the socket or stdin,
> > which is very inefficient. With 100+ VMs on the host, this results in
> > multiple extra system calls and CPU overuse.
> > This patch increases the amount of read data up to 4096 bytes that fits
> > the buffer size on the channel level.
> >
> > Suggested-by: Denis V. Lunev <den@openvz.org>
> > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com>
> > ---
> >   chardev/char-fd.c          | 64 
> > +++++++++++++++++++++++++++++++++++++++++++++-
> >   chardev/char-socket.c      | 54 +++++++++++++++++++++++++++-----------
> >   chardev/char.c             | 40 +++++++++++++++++++++++++++++
> >   include/chardev/char.h     | 15 +++++++++++
> >   monitor/monitor.c          |  2 +-
> >   tests/qemu-iotests/247.out |  2 +-
> >   6 files changed, 159 insertions(+), 18 deletions(-)

[...]

> > +        ret = qio_channel_read(
> > +            chan, (gchar *)thl.buf, len, NULL);
> > +        if (ret == 0) {
> > +            remove_fd_in_watch(chr);
> > +            qemu_chr_be_event(chr, CHR_EVENT_CLOSED);
> > +            thl = (const JSONthrottle){0};
> > +            return FALSE;
> > +        }
> > +        if (ret < 0) {
> > +            return TRUE;
> > +        }
>
> large code chunk is shared with fd_chr_read_hmp(). Would be not bad to 
> avoid duplication..

There were two reasons to split the function:
1. Not to make the code complicated.
2. Avoid unused buffer of 4k on the stack:
   fd_chr_read_hmp() { uint8_t buf[CHR_READ_BUF_LEN];..

> > +        thl.load = ret;
> > +        thl.cursor = 0;
> > +    }
> > +
> > +    size = thl.load;
> > +    start = thl.buf + thl.cursor;
>
> you may use uint8_t* pointer type for thl.curser and get rid of size and 
> start variables.

For the 'start', yes. And I will want the 'size' anyway.

[...]

> > +int qemu_chr_end_position(const char *buf, int size, JSONthrottle *thl)
> > +{
> > +    int i;
> > +
> > +    for (i = 0; i < size; i++) {
> > +        switch (buf[i]) {
> > +        case ' ':
> > +        case '\n':
> > +        case '\r':
> > +            continue;
> > +        case '{':
> > +            thl->brace_count++;
> > +            break;
> > +        case '}':
> > +            thl->brace_count--;
> > +            break;
> > +        case '[':
> > +            thl->bracket_count++;
> > +            break;
> > +        case ']':
> > +            thl->bracket_count--;
>
> I don't think you need to care about square brackets, as QMP queries and 
> answers are always json objects, i.e. in pair of '{' and '}'.

I've kept the brackets because it is another condition to put a command 
into the requests queue (see json_message_process_token()).


Andrey