Re: [Qemu-devel] [PATCH 1/1] monitor: increase amount of data for monitor to read

[Denis V. Lunev]

On 05/02/2017 07:48 PM, Daniel P. Berrange wrote:
> On Tue, May 02, 2017 at 05:36:30PM +0100, Dr. David Alan Gilbert wrote:
>> * Markus Armbruster (address@hidden) wrote:
>>> "Denis V. Lunev" <address@hidden> writes:
>>>
>>>> On 05/02/2017 05:43 PM, Markus Armbruster wrote:
>>>>> "Denis V. Lunev" <address@hidden> writes:
>>>>>
>>>>>> Right now QMP and HMP monitors read 1 byte at a time from the socket, 
>>>>>> which
>>>>>> is very inefficient. With 100+ VMs on the host this easily reasults in
>>>>>> a lot of unnecessary system calls and CPU usage in the system.
>>>>>>
>>>>>> This patch changes the amount of data to read to 4096 bytes, which 
>>>>>> matches
>>>>>> buffer size on the channel level. Fortunately, monitor protocol is
>>>>>> synchronous right now thus we should not face side effects in reality.
>>>>> Can you explain briefly why this relies on "synchronous"?  I've spent
>>>>> all of two seconds on the question myself...
>>>> Each command is processed in sequence as it appears in the
>>>> channel. The answer to the command is sent and only after that
>>>> next command is processed.
>>> Yes, that's how QMP works.
>>>
>>>> Theoretically tith asynchronous processing we can have some side
>>>> effects due to changed buffer size.
>>> What kind of side effects do you have in mind?
>>>
>>> It's quite possible that this obviously inefficient way to read had some
>>> deep reason back when it was created.  Hmm, git-blame is our friend:
>>>
>>> commit c62313bbdc48f72e93fa8196f2fff96ba35e4e9d
>>> Author: Jan Kiszka <address@hidden>
>>> Date:   Fri Dec 4 14:05:29 2009 +0100
>>>
>>>     monitor: Accept input only byte-wise
>>>     
>>>     This allows to suspend command interpretation and execution
>>>     synchronously, e.g. during migration.
>>>     
>>>     Signed-off-by: Jan Kiszka <address@hidden>
>>>     Signed-off-by: Anthony Liguori <address@hidden>
>> I don't think I understand why that's a problem; if we read more bytes,
>> we're not going to interpret them and execute them until after the previous
>> command returns are we?
> Actually it sees we might do, due to the way the "migrate" command works
> in HMP when you don't give the '-d' flag.
>
> Most monitors commands will block the caller until they are finished,
> but "migrate" is different. The hmp_migrate() method will return
> immediately, but we call monitor_suspend() to block processing of
> further commands. If another command has already been read off
> the wire though (due to "monitor_read" having a buffer that contains
> multiple commands), we would in fact start processing this command
> despite having suspended the monitor.
>
> This is only a problem, however, if the client app has issued "migrate"
> followed by another command, at the same time without waiting for the
> respond to "migrate". So in practice the only way you'd hit the bug
> is probably if you just cut+paste a big chunk of commands into the
> monitor at once without waiting for completion and one of the commands
> was "migrate" without "-d".
>
> Still, I think we would need to figure out a proper fix for this before
> we could increase the buffer size.
>
> Regards,
> Daniel

There is one thing, which simplifies things a lot.
- suspend_cnt can be increased only from 2 places:
  1) monitor_event(), which is called for real HMP monitor only

  2) monitor_suspend(), which can increment suspend_cnt
      only if mon->rs != NULL, which also means that the
      monitor is specifically configured HMP monitor.

So, we can improve the patch (for now) with the following
tweak:

static int monitor_can_read(void *opaque)
{
    Monitor *mon = opaque;
   
    if (monitor_is_qmp(mon))
        return 4096;   
    return (mon->suspend_cnt == 0) ? 1 : 0;
}

This will solve my case completely and does not break any
backward compatibility.

Den