Re: [PATCH v7 42/65] virtio-balloon: convert guest-stats property to QAPI type

[Markus Armbruster <armbru@redhat.com>]

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

> Define VirtioBalloonStats and VirtioBalloonGuestStats QAPI struct types
> in the schema, replacing the hand-rolled visitor in balloon_stats_get_all()
> with a generated visit_type_VirtioBalloonGuestStats() call.
>
> The wire format is preserved: the JSON output from qom-get is identical
> to the previous hand-rolled visitor output.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  docs/interop/virtio-balloon-stats.rst | 22 +--------
>  hw/virtio/virtio-balloon.c            | 84 ++++++++++++--------------------
>  qapi/machine.json                     | 92 +++++++++++++++++++++++++++++++++++
>  3 files changed, 123 insertions(+), 75 deletions(-)
>
> diff --git a/docs/interop/virtio-balloon-stats.rst b/docs/interop/virtio-balloon-stats.rst
> index b9a6a6edb229..88d3271bbb02 100644
> --- a/docs/interop/virtio-balloon-stats.rst
> +++ b/docs/interop/virtio-balloon-stats.rst
> @@ -22,27 +22,7 @@ polling the guest's balloon driver for new stats in the specified time
>  interval.
>  
>  To retrieve those stats, clients have to query the guest-stats property,
> -which will return a dictionary containing:
> -
> -  * A key named 'stats', containing all available stats. If the guest
> -    doesn't support a particular stat, or if it couldn't be retrieved,
> -    its value will be -1. Currently, the following stats are supported:
> -
> -      - stat-swap-in
> -      - stat-swap-out
> -      - stat-major-faults
> -      - stat-minor-faults
> -      - stat-free-memory
> -      - stat-total-memory
> -      - stat-available-memory
> -      - stat-disk-caches
> -      - stat-htlb-pgalloc
> -      - stat-htlb-pgfail

Missing here:

          - stat-oom-kills
          - stat-alloc-stalls
          - stat-async-scans
          - stat-direct-scans
          - stat-async-reclaims
          - stat-direct-reclaims

Review note, not a request for anything.

> -
> -  * A key named last-update, which contains the last stats update
> -    timestamp in seconds. Since this timestamp is generated by the host,
> -    a buggy guest can't influence its value. The value is 0 if the guest
> -    has not updated the stats (yet).
> +which will return a VirtioBalloonGuestStats (see QAPI documentation).
>  
>  It's also important to note the following:
>  
> diff --git a/hw/virtio/virtio-balloon.c b/hw/virtio/virtio-balloon.c
> index 7ffbe48e5134..bfc75f5dfc72 100644
> --- a/hw/virtio/virtio-balloon.c
> +++ b/hw/virtio/virtio-balloon.c
> @@ -29,6 +29,8 @@
>  #include "qapi/error.h"
>  #include "qapi/qapi-builtin-type-infos.h"
>  #include "qapi/qapi-events-machine.h"
> +#include "qapi/qapi-type-infos-machine.h"
> +#include "qapi/qapi-visit-machine.h"
>  #include "qapi/visitor.h"
>  #include "trace.h"
>  #include "qemu/error-report.h"
> @@ -170,33 +172,8 @@ static void balloon_deflate_page(VirtIOBalloon *balloon,
>      }
>  }
>  
> -/*
> - * All stats upto VIRTIO_BALLOON_S_NR /must/ have a
> - * non-NULL name declared here, since these are used
> - * as keys for populating the QDict with stats
> - */
> -static const char *balloon_stat_names[] = {
> -   [VIRTIO_BALLOON_S_SWAP_IN] = "stat-swap-in",
> -   [VIRTIO_BALLOON_S_SWAP_OUT] = "stat-swap-out",
> -   [VIRTIO_BALLOON_S_MAJFLT] = "stat-major-faults",
> -   [VIRTIO_BALLOON_S_MINFLT] = "stat-minor-faults",
> -   [VIRTIO_BALLOON_S_MEMFREE] = "stat-free-memory",
> -
> -   [VIRTIO_BALLOON_S_MEMTOT] = "stat-total-memory",
> -   [VIRTIO_BALLOON_S_AVAIL] = "stat-available-memory",
> -   [VIRTIO_BALLOON_S_CACHES] = "stat-disk-caches",
> -   [VIRTIO_BALLOON_S_HTLB_PGALLOC] = "stat-htlb-pgalloc",
> -   [VIRTIO_BALLOON_S_HTLB_PGFAIL] = "stat-htlb-pgfail",
> -
> -   [VIRTIO_BALLOON_S_OOM_KILL] = "stat-oom-kills",
> -   [VIRTIO_BALLOON_S_ALLOC_STALL] = "stat-alloc-stalls",
> -   [VIRTIO_BALLOON_S_ASYNC_SCAN] = "stat-async-scans",
> -   [VIRTIO_BALLOON_S_DIRECT_SCAN] = "stat-direct-scans",
> -   [VIRTIO_BALLOON_S_ASYNC_RECLAIM] = "stat-async-reclaims",
> -
> -   [VIRTIO_BALLOON_S_DIRECT_RECLAIM] = "stat-direct-reclaims",

Not missing here.

> -};
> -G_STATIC_ASSERT(G_N_ELEMENTS(balloon_stat_names) == VIRTIO_BALLOON_S_NR);
> +/* Update VirtioBalloonStats QAPI type when new stats are added */
> +G_STATIC_ASSERT(VIRTIO_BALLOON_S_NR == 16);

The assertion becomes awkward.

What about ...

>  
>  /*
>   * reset_stats - Mark all items in the stats array as unset
> @@ -258,33 +235,31 @@ static void balloon_stats_get_all(Object *obj, Visitor *v, const char *name,
>                                    void *opaque, Error **errp)
>  {
>      VirtIOBalloon *s = VIRTIO_BALLOON(obj);
> -    bool ok = false;
> -    int i;
> +    VirtioBalloonStats stats = {
> +        .stat_swap_in = s->stats[VIRTIO_BALLOON_S_SWAP_IN],
> +        .stat_swap_out = s->stats[VIRTIO_BALLOON_S_SWAP_OUT],
> +        .stat_major_faults = s->stats[VIRTIO_BALLOON_S_MAJFLT],
> +        .stat_minor_faults = s->stats[VIRTIO_BALLOON_S_MINFLT],
> +        .stat_free_memory = s->stats[VIRTIO_BALLOON_S_MEMFREE],
> +        .stat_total_memory = s->stats[VIRTIO_BALLOON_S_MEMTOT],
> +        .stat_available_memory = s->stats[VIRTIO_BALLOON_S_AVAIL],
> +        .stat_disk_caches = s->stats[VIRTIO_BALLOON_S_CACHES],
> +        .stat_htlb_pgalloc = s->stats[VIRTIO_BALLOON_S_HTLB_PGALLOC],
> +        .stat_htlb_pgfail = s->stats[VIRTIO_BALLOON_S_HTLB_PGFAIL],
> +        .stat_oom_kills = s->stats[VIRTIO_BALLOON_S_OOM_KILL],
> +        .stat_alloc_stalls = s->stats[VIRTIO_BALLOON_S_ALLOC_STALL],
> +        .stat_async_scans = s->stats[VIRTIO_BALLOON_S_ASYNC_SCAN],
> +        .stat_direct_scans = s->stats[VIRTIO_BALLOON_S_DIRECT_SCAN],
> +        .stat_async_reclaims = s->stats[VIRTIO_BALLOON_S_ASYNC_RECLAIM],
> +        .stat_direct_reclaims = s->stats[VIRTIO_BALLOON_S_DIRECT_RECLAIM],
> +    };

... putting it here?  This initializer also needs to be updated when
stats are added.

Another review note...  The kernel gives us an array of stats with
well-known indexes.  Since we "improved" on that interface, we need to
manually convert to struct.  Oh well.

> +    VirtioBalloonGuestStats guest_stats = {
> +        .last_update = s->stats_last_update,
> +        .stats = &stats,
> +    };
> +    VirtioBalloonGuestStats *argp = &guest_stats;
>  
> -    if (!visit_start_struct(v, name, NULL, 0, errp)) {
> -        return;
> -    }
> -    if (!visit_type_int(v, "last-update", &s->stats_last_update, errp)) {
> -        goto out_end;
> -    }
> -
> -    if (!visit_start_struct(v, "stats", NULL, 0, errp)) {
> -        goto out_end;
> -    }
> -    for (i = 0; i < VIRTIO_BALLOON_S_NR; i++) {
> -        if (!visit_type_uint64(v, balloon_stat_names[i], &s->stats[i], errp)) {
> -            goto out_nested;
> -        }
> -    }
> -    ok = visit_check_struct(v, errp);
> -out_nested:
> -    visit_end_struct(v, NULL);
> -
> -    if (ok) {
> -        visit_check_struct(v, errp);
> -    }
> -out_end:
> -    visit_end_struct(v, NULL);
> +    visit_type_VirtioBalloonGuestStats(v, name, &argp, errp);
>  }
>  
>  static void balloon_stats_get_poll_interval(Object *obj, Visitor *v,
> @@ -1037,7 +1012,8 @@ static void virtio_balloon_instance_init(Object *obj)
>      s->free_page_hint_cmd_id = VIRTIO_BALLOON_FREE_PAGE_HINT_CMD_ID_MIN;
>      s->free_page_hint_notify.notify = virtio_balloon_free_page_hint_notify;
>  
> -    object_property_add(obj, "guest-stats", "guest statistics",
> +    object_property_add_qapi(obj, "guest-stats",
> +                        &VirtioBalloonGuestStats_type_info,
>                          balloon_stats_get_all, NULL, NULL, NULL);

The @type reported by qom-list & friends changes from "guest statistics"
to "VirtioBalloonStats".  Such changes can be compatibility breaks.

We accept this one, because:

1. What "guest statistics" might mean was never documented.
   ObjectPropertyInfo's (flawed) comment covers only "primitive", child
   and link types, which misleadingly suggests values like this one
   don't exist.

2. We are not aware of anything relying on it, and we can't quite see
   why anything would want to rely on it.

3. @type is a bad interface we ought to deprecate in favor of
   @qapi-type.

All this needs to be noted in the commit message.  Perhaps even in
release notes.  I guess the latter will be easier if we do deprecate
@type.

>  
>      object_property_add_qapi(obj, "guest-stats-polling-interval", &int_type_info,
> diff --git a/qapi/machine.json b/qapi/machine.json
> index 9b145c635baf..34be3156270c 100644
> --- a/qapi/machine.json
> +++ b/qapi/machine.json
> @@ -1126,6 +1126,98 @@
>  ##
>  { 'struct': 'BalloonInfo', 'data': {'actual': 'int' } }
>  
> +##
> +# @VirtioBalloonStats:
> +#
> +# VirtIO balloon device guest memory statistics.
> +#
> +# If the guest doesn't support a particular stat, or if it couldn't
> +# be retrieved, its value will be -1.

Please use the revised syntax for introductions:

    ##
    # @name: introduction, best to keep it brief, but you may continue it
    #     like this if necessary.  If your text need to refer to arguments
    #     / members, it's almost certainly not brief introduction anymore.
    #
    # Additional text can go here.  But do consider putting it further
    # down.

I apologize for the inconvenience.  We need this to eliminate certain
syntactic ambiguities that plagued the doc generator.  We'll need it
some more for the doc inliner.

> +#
> +# @stat-swap-in: The amount of memory that has been
> +#     swapped in (in bytes).

Wrap lines at column 70, please.

> +#
> +# @stat-swap-out: The amount of memory that has been
> +#     swapped out to disk (in bytes).
> +#
> +# @stat-major-faults: The number of major page faults
> +#     that have occurred.
> +#
> +# @stat-minor-faults: The number of minor page faults
> +#     that have occurred.
> +#
> +# @stat-free-memory: The amount of memory not being used
> +#     for any purpose (in bytes).
> +#
> +# @stat-total-memory: The total amount of memory available
> +#     (in bytes).
> +#
> +# @stat-available-memory: An estimate of how much memory is available
> +#     (in bytes) for starting new applications, without pushing the
> +#     system to swap.
> +#
> +# @stat-disk-caches: The amount of memory, in bytes, that can be
> +#     quickly reclaimed without additional I/O.  Typically these pages
> +#     are used for caching files from disk.
> +#
> +# @stat-htlb-pgalloc: The number of successful hugetlb page
> +#     allocations
> +#
> +# @stat-htlb-pgfail: The number of failed hugetlb page
> +#     allocations
> +#
> +# @stat-oom-kills: The number of OOM killer invocations
> +#
> +# @stat-alloc-stalls: The number of memory allocation stalls
> +#
> +# @stat-async-scans: The number of memory scanned asynchronously
> +#
> +# @stat-direct-scans: The number of memory scanned directly
> +#
> +# @stat-async-reclaims: The number of memory reclaimed asynchronously
> +#
> +# @stat-direct-reclaims: The number of memory reclaimed directly
> +#
> +# Since: 11.2

The actual interface, namely the device property, is much older (1.4?).
Do we care?  I'm not sure; this isn't part of QMP proper, it's for use
with QOM, where documentation has always been rather weak.

> +##
> +{ 'struct': 'VirtioBalloonStats',
> +  'data': {
> +    'stat-swap-in': 'uint64',
> +    'stat-swap-out': 'uint64',
> +    'stat-major-faults': 'uint64',
> +    'stat-minor-faults': 'uint64',
> +    'stat-free-memory': 'uint64',
> +    'stat-total-memory': 'uint64',
> +    'stat-available-memory': 'uint64',
> +    'stat-disk-caches': 'uint64',
> +    'stat-htlb-pgalloc': 'uint64',
> +    'stat-htlb-pgfail': 'uint64',
> +    'stat-oom-kills': 'uint64',
> +    'stat-alloc-stalls': 'uint64',
> +    'stat-async-scans': 'uint64',
> +    'stat-direct-scans': 'uint64',
> +    'stat-async-reclaims': 'uint64',
> +    'stat-direct-reclaims': 'uint64' } }

I don't like the "stat-" prefix, but we can't change that now.

> +
> +##
> +# @VirtioBalloonGuestStats:
> +#
> +# Guest statistics from the VirtIO balloon device.

Please use the revised syntax for introductions.

> +#
> +# @last-update: timestamp in seconds of the last stats
> +#     update from the guest (since this timestamp is generated
> +#     by the host, a buggy guest can't influence its value),
> +#     or 0 if no update has been received yet.

The original text is 

  * A key named last-update, which contains the last stats update
    timestamp in seconds. Since this timestamp is generated by the host,
    a buggy guest can't influence its value. The value is 0 if the guest
    has not updated the stats (yet).

Threee sentences instead of one with a parenthesis.  I find this easier
to read.

> +#
> +# @stats: balloon memory statistics
> +#
> +# Since: 11.2
> +##
> +{ 'struct': 'VirtioBalloonGuestStats',
> +  'data': {
> +    'last-update': 'int',
> +    'stats': 'VirtioBalloonStats' } }
> +
>  ##
>  # @query-balloon:
>  #     Return information about the balloon device.