Re: [RFC PATCH v2 3/5] virtio-balloon: Add QMP functions for Working Set

[Markus Armbruster <armbru@redhat.com>]

"T.J. Alumbaugh" <talumbau@google.com> writes:

>   - Adds QMP function 'working-set-config'
>   - Adds QMP function 'working-set-request'
>   - Retrieve working set via 'guest-working-set' property on balloon
>
>>> cat script.py
>
> NAME = "name"
> SOCKET = 'vm.sock'
> BALLOON =  "/machine/peripheral/balloon0"
>
> import json
> import asyncio
> from qemu.qmp import QMPClient
>
> async def main():
>     client = QMPClient(NAME)
>     await client.connect(SOCKET)
>     config = { "i0": 200, "i1": 800, "i2": 3000, "refresh": 750, "report": 1000 }
>     await client.execute('working-set-config', config)
>     await client.execute('working-set-request')
>     property = {"path":BALLOON, "property":"guest-working-set"}
>     res = await client.execute('qom-get', property)
>     return res
>
> if __name__ == "__main__":
>     ret = asyncio.run(main())
>     print(json.dumps(ret, indent=2))
>
>>> (Execute qemu with flag '-qmp unix:path=vm.sock,server=on,wait=off'
>>> (Perform normal activities on VM to exercise MM code)
>
>>> python3 script.py
> {
>   "working_set": {
>     "ws3": {
>       "memory-size-bytes": {
>         "anon": 890478592,
>         "file": 1285832704
>       },
>       "idle-age": 4294967292
>     },
>     "ws2": {
>       "memory-size-bytes": {
>         "anon": 173465600,
>         "file": 83353600
>       },
>       "idle-age": 3000
>     },
>     "ws1": {
>       "memory-size-bytes": {
>         "anon": 44236800,
>         "file": 20889600
>       },
>       "idle-age": 800
>     },
>     "ws0": {
>       "memory-size-bytes": {
>         "anon": 14540800,
>         "file": 6963200
>       },
>       "idle-age": 200
>     }
>   }
> }
>
> Signed-off-by: T.J. Alumbaugh <talumbau@google.com>

[...]

> diff --git a/monitor/monitor.c b/monitor/monitor.c
> index 602535696c..fad1b4aed5 100644
> --- a/monitor/monitor.c
> +++ b/monitor/monitor.c
> @@ -333,6 +333,7 @@ static MonitorQAPIEventConf monitor_qapi_event_conf[QAPI_EVENT__MAX] = {
>      [QAPI_EVENT_QUORUM_FAILURE]    = { 1000 * SCALE_MS },
>      [QAPI_EVENT_VSERPORT_CHANGE]   = { 1000 * SCALE_MS },
>      [QAPI_EVENT_MEMORY_DEVICE_SIZE_CHANGE] = { 1000 * SCALE_MS },
> +    [QAPI_EVENT_WORKING_SET_EVENT] = { 1000 * SCALE_MS },
>  };
>  
>  /*
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 37660d8f2a..5e03ff21e2 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1055,6 +1055,57 @@
>  ##
>  { 'command': 'balloon', 'data': {'value': 'int'} }
>  
> +##
> +# @working-set-config:
> +#
> +# Specify the config parameters for Working Set reporting.

Don't capitalize "Working Set" unless it is a proper noun.  Is it?

What about "Configure working set reporting"?

> +#
> +# @i0: the endpoint of the first interval (in ms)
> +#
> +# @i1: the endpoint of the second interval (in ms)
> +#
> +# @i2: the endpoint of the third interval (in ms)

An "endpoint" doesn't define an interval.  Also, a "in milliseconds"
suggests relative time.  Relative to what?

What do these intervals do?

> +#
> +# @refresh: the refresh threshold (in ms) for Working Set reporting
> +#
> +# @report: the report threshold (in ms) for Working Set reporting

What do these do?

> +#
> +# Returns: - Nothing on success
> +#          - If no balloon device is present, DeviceNotActive

We use errors other than GenericError only when we have a compelling
reason.  I figure the reason is consistency with other commands
operating on a ballon device.  Correct?

Please format like

   # Returns:
   #     - Nothing on success
   #     - If no balloon device is present, DeviceNotActive

to blend in with recent commit a937b6aa739 (qapi: Reformat doc comments
to conform to current conventions).

> +#
> +# Example:
> +#
> +# -> { "execute": "working-set-config",
> +#                 "arguments": { "i0": 100,
> +#                                "i1": 500,
> +#                                "i2": 2000,
> +#                                "refresh": 750,
> +#                                "report": 1000 } }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'working-set-config', 'data': {'i0': 'uint64',
> +                                            'i1': 'uint64',
> +                                            'i2': 'uint64',
> +                                            'refresh': 'uint64',
> +                                            'report': 'uint64'} }
> +##
> +# @working-set-request:
> +#
> +# Request the Working Set report from the guest.

"Request a"

> +#
> +# Returns: - Nothing on success
> +#          - If no balloon device is present, DeviceNotActive
> +#
> +# Example:
> +#
> +# -> { "execute": "working-set-request", "arguments": {} }
> +# <- { "return": {} }
> +#
> +##
> +{ 'command': 'working-set-request', 'data': {} }
> +
> +
>  ##
>  # @BalloonInfo:
>  #
> @@ -1113,6 +1164,21 @@
>  { 'event': 'BALLOON_CHANGE',
>    'data': { 'actual': 'int' } }
>  
> +##
> +# @WORKING_SET_EVENT:
> +#
> +# Emitted when the guest sends a new Working Set report.

Where does it send the report to?

> +#
> +# Note: this event is rate-limited.
> +#
> +# Example:
> +#
> +# <- { "event": "WORKING_SET_EVENT",
> +#      "timestamp": { "seconds": 1267020223, "microseconds": 435656 } }
> +#
> +##
> +{ 'event': 'WORKING_SET_EVENT' }
> +
>  ##
>  # @MemoryInfo:
>  #

Documentation needs to answer:

1. What is working set reporting about, and why would I want to use it?

2. How are the reports to be interpreted?

3. What do the configuration parameters do?

[...]