Re: [Qemu-devel] [PATCH v4 4/9] {hmp, hw/pvrdma}: Expose device internals via monitor interface

[Markus Armbruster <armbru@redhat.com>]

Yuval Shaia <yuval.shaia@oracle.com> writes:

> Allow interrogating device internals through HMP interface.
> The exposed indicators can be used for troubleshooting by developers or
> sysadmin.
> There is no need to expose these attributes to a management system (e.x.
> libvirt) because (1) most of them are not "device-management' related
> info and (2) there is no guarantee the interface is stable.
>
> Signed-off-by: Yuval Shaia <yuval.shaia@oracle.com>
> ---
>  hmp-commands-info.hx       | 14 ++++++++
>  hmp.c                      | 27 +++++++++++++++
>  hmp.h                      |  1 +
>  hw/rdma/Makefile.objs      |  2 +-
>  hw/rdma/rdma_backend.c     | 70 +++++++++++++++++++++++++++++---------
>  hw/rdma/rdma_hmp.c         | 30 ++++++++++++++++
>  hw/rdma/rdma_rm.c          | 60 ++++++++++++++++++++++++++++++++
>  hw/rdma/rdma_rm.h          |  1 +
>  hw/rdma/rdma_rm_defs.h     | 27 ++++++++++++++-
>  hw/rdma/vmw/pvrdma.h       |  5 +++
>  hw/rdma/vmw/pvrdma_main.c  | 21 ++++++++++++
>  include/hw/rdma/rdma_hmp.h | 40 ++++++++++++++++++++++
>  12 files changed, 279 insertions(+), 19 deletions(-)
>  create mode 100644 hw/rdma/rdma_hmp.c
>  create mode 100644 include/hw/rdma/rdma_hmp.h
>
> diff --git a/hmp-commands-info.hx b/hmp-commands-info.hx
> index cbee8b944d..c59444c461 100644
> --- a/hmp-commands-info.hx
> +++ b/hmp-commands-info.hx
> @@ -202,6 +202,20 @@ STEXI
>  @item info pic
>  @findex info pic
>  Show PIC state.
> +ETEXI
> +
> +    {
> +        .name       = "rdma",
> +        .args_type  = "",
> +        .params     = "",
> +        .help       = "show RDMA state",
> +        .cmd        = hmp_info_rdma,
> +    },
> +
> +STEXI
> +@item info rdma
> +@findex info rdma
> +Show RDMA state.
>  ETEXI
>  
>      {
> diff --git a/hmp.c b/hmp.c
> index 1e006eeb49..68511b2441 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -51,6 +51,7 @@
>  #include "qemu/error-report.h"
>  #include "exec/ramlist.h"
>  #include "hw/intc/intc.h"
> +#include "hw/rdma/rdma_hmp.h"
>  #include "migration/snapshot.h"
>  #include "migration/misc.h"
>  
> @@ -968,6 +969,32 @@ void hmp_info_pic(Monitor *mon, const QDict *qdict)
>                                     hmp_info_pic_foreach, mon);
>  }
>  
> +static int hmp_info_rdma_foreach(Object *obj, void *opaque)
> +{
> +    RdmaStatsProvider *rdma;
> +    RdmaStatsProviderClass *k;
> +    Monitor *mon = opaque;
> +
> +    if (object_dynamic_cast(obj, TYPE_RDMA_STATS_PROVIDER)) {
> +        rdma = RDMA_STATS_PROVIDER(obj);
> +        k = RDMA_STATS_PROVIDER_GET_CLASS(obj);
> +        if (k->print_statistics) {
> +            k->print_statistics(mon, rdma);
> +        } else {
> +            monitor_printf(mon, "RDMA statistics not available for %s.\n",
> +                           object_get_typename(obj));
> +        }
> +    }
> +
> +    return 0;
> +}
> +
> +void hmp_info_rdma(Monitor *mon, const QDict *qdict)
> +{
> +    object_child_foreach_recursive(object_get_root(),
> +                                   hmp_info_rdma_foreach, mon);
> +}
> +
>  void hmp_info_pci(Monitor *mon, const QDict *qdict)
>  {
>      PciInfoList *info_list, *info;
> diff --git a/hmp.h b/hmp.h
> index 5f1addcca2..666949afc3 100644
> --- a/hmp.h
> +++ b/hmp.h
> @@ -36,6 +36,7 @@ void hmp_info_spice(Monitor *mon, const QDict *qdict);
>  void hmp_info_balloon(Monitor *mon, const QDict *qdict);
>  void hmp_info_irq(Monitor *mon, const QDict *qdict);
>  void hmp_info_pic(Monitor *mon, const QDict *qdict);
> +void hmp_info_rdma(Monitor *mon, const QDict *qdict);
>  void hmp_info_pci(Monitor *mon, const QDict *qdict);
>  void hmp_info_block_jobs(Monitor *mon, const QDict *qdict);
>  void hmp_info_tpm(Monitor *mon, const QDict *qdict);

HMP stuff looks good to me.

> diff --git a/hw/rdma/Makefile.objs b/hw/rdma/Makefile.objs
> index bd36cbf51c..dd59faff51 100644
> --- a/hw/rdma/Makefile.objs
> +++ b/hw/rdma/Makefile.objs
> @@ -1,5 +1,5 @@
>  ifeq ($(CONFIG_PVRDMA),y)
> -obj-$(CONFIG_PCI) += rdma_utils.o rdma_backend.o rdma_rm.o
> +obj-$(CONFIG_PCI) += rdma_utils.o rdma_backend.o rdma_rm.o rdma_hmp.o
>  obj-$(CONFIG_PCI) += vmw/pvrdma_dev_ring.o vmw/pvrdma_cmd.o \
>                       vmw/pvrdma_qp_ops.o vmw/pvrdma_main.o
>  endif
> diff --git a/hw/rdma/rdma_backend.c b/hw/rdma/rdma_backend.c
> index 9679b842d1..bc2fefcf93 100644
> --- a/hw/rdma/rdma_backend.c
> +++ b/hw/rdma/rdma_backend.c

The changes to this file look like they're for gathering stats.  The
patch would be much simpler to review had you kept the stats gathering
separate.  I can only review the HMP / QOM part, and to do that, I have
to identify what to ignore.

[...]
> diff --git a/hw/rdma/rdma_hmp.c b/hw/rdma/rdma_hmp.c
> new file mode 100644
> index 0000000000..c5814473c5
> --- /dev/null
> +++ b/hw/rdma/rdma_hmp.c
> @@ -0,0 +1,30 @@
> +/*
> + * RDMA device: Human Monitor interface

The file name and this comment are a bit akward.  Yes, you create
TYPE_RDMA_STATS_PROVIDER for use in HMP info rdma, but there's
absolutely nothing HMP-related in this file.  Same for rdma_hmp.h below.

Call them rdma_stats.c and rdma_stats.h?

> + *
> + * Copyright (C) 2018 Oracle
> + * Copyright (C) 2018 Red Hat Inc
> + *
> + * Authors:
> + *     Yuval Shaia <yuval.shaia@oracle.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +#include "hw/rdma/rdma_hmp.h"
> +#include "qemu/module.h"
> +
> +static const TypeInfo rdma_hmp_info = {
> +    .name = TYPE_RDMA_STATS_PROVIDER,
> +    .parent = TYPE_INTERFACE,
> +    .class_size = sizeof(RdmaStatsProviderClass),
> +};
> +
> +static void rdma_hmp_register_types(void)
> +{
> +    type_register_static(&rdma_hmp_info);
> +}
> +
> +type_init(rdma_hmp_register_types)

Also rename _hmp_ to _stats_.

> diff --git a/hw/rdma/rdma_rm.c b/hw/rdma/rdma_rm.c
> index 14580ca379..e019de1a14 100644
> --- a/hw/rdma/rdma_rm.c
> +++ b/hw/rdma/rdma_rm.c
> @@ -16,6 +16,7 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "cpu.h"
> +#include "monitor/monitor.h"
>  
>  #include "trace.h"
>  #include "rdma_utils.h"
> @@ -26,6 +27,58 @@
>  #define PG_DIR_SZ { TARGET_PAGE_SIZE / sizeof(__u64) }
>  #define PG_TBL_SZ { TARGET_PAGE_SIZE / sizeof(__u64) }
>  
> +void rdma_dump_device_counters(Monitor *mon, RdmaDeviceResources *dev_res)
> +{
> +    monitor_printf(mon, "\ttx               : %" PRId64 "\n",
> +                   dev_res->stats.tx);
> +    monitor_printf(mon, "\ttx_len           : %" PRId64 "\n",
> +                   dev_res->stats.tx_len);
> +    monitor_printf(mon, "\ttx_err           : %" PRId64 "\n",
> +                   dev_res->stats.tx_err);
> +    monitor_printf(mon, "\trx_bufs          : %" PRId64 "\n",
> +                   dev_res->stats.rx_bufs);
> +    monitor_printf(mon, "\trx_bufs_len      : %" PRId64 "\n",
> +                   dev_res->stats.rx_bufs_len);
> +    monitor_printf(mon, "\trx_bufs_err      : %" PRId64 "\n",
> +                   dev_res->stats.rx_bufs_err);
> +    monitor_printf(mon, "\tcomps            : %" PRId64 "\n",
> +                   dev_res->stats.completions);
> +    monitor_printf(mon, "\tmissing_comps    : %" PRId32 "\n",
> +                   dev_res->stats.missing_cqe);
> +    monitor_printf(mon, "\tpoll_cq (bk)     : %" PRId64 "\n",
> +                   dev_res->stats.poll_cq_from_bk);
> +    monitor_printf(mon, "\tpoll_cq_ppoll_to : %" PRId64 "\n",
> +                   dev_res->stats.poll_cq_ppoll_to);
> +    monitor_printf(mon, "\tpoll_cq (fe)     : %" PRId64 "\n",
> +                   dev_res->stats.poll_cq_from_guest);
> +    monitor_printf(mon, "\tpoll_cq_empty    : %" PRId64 "\n",
> +                   dev_res->stats.poll_cq_from_guest_empty);
> +    monitor_printf(mon, "\tmad_tx           : %" PRId64 "\n",
> +                   dev_res->stats.mad_tx);
> +    monitor_printf(mon, "\tmad_tx_err       : %" PRId64 "\n",
> +                   dev_res->stats.mad_tx_err);
> +    monitor_printf(mon, "\tmad_rx           : %" PRId64 "\n",
> +                   dev_res->stats.mad_rx);
> +    monitor_printf(mon, "\tmad_rx_err       : %" PRId64 "\n",
> +                   dev_res->stats.mad_rx_err);
> +    monitor_printf(mon, "\tmad_rx_bufs      : %" PRId64 "\n",
> +                   dev_res->stats.mad_rx_bufs);
> +    monitor_printf(mon, "\tmad_rx_bufs_err  : %" PRId64 "\n",
> +                   dev_res->stats.mad_rx_bufs_err);
> +    monitor_printf(mon, "\tPDs              : %" PRId32 "\n",
> +                   dev_res->pd_tbl.used);
> +    monitor_printf(mon, "\tMRs              : %" PRId32 "\n",
> +                   dev_res->mr_tbl.used);
> +    monitor_printf(mon, "\tUCs              : %" PRId32 "\n",
> +                   dev_res->uc_tbl.used);
> +    monitor_printf(mon, "\tQPs              : %" PRId32 "\n",
> +                   dev_res->qp_tbl.used);
> +    monitor_printf(mon, "\tCQs              : %" PRId32 "\n",
> +                   dev_res->cq_tbl.used);
> +    monitor_printf(mon, "\tCEQ_CTXs         : %" PRId32 "\n",
> +                   dev_res->cqe_ctx_tbl.used);
> +}
> +
>  static inline void res_tbl_init(const char *name, RdmaRmResTbl *tbl,
>                                  uint32_t tbl_sz, uint32_t res_sz)
>  {
> @@ -37,6 +90,7 @@ static inline void res_tbl_init(const char *name, RdmaRmResTbl *tbl,

More stats gathering, I think.

[...]
> diff --git a/hw/rdma/rdma_rm.h b/hw/rdma/rdma_rm.h
> index 9ec87d2667..a527d306f6 100644
> --- a/hw/rdma/rdma_rm.h
> +++ b/hw/rdma/rdma_rm.h
> @@ -20,6 +20,7 @@
>  #include "rdma_backend_defs.h"
>  #include "rdma_rm_defs.h"
>  
> +void rdma_dump_device_counters(Monitor *mon, RdmaDeviceResources *dev_res);
>  int rdma_rm_init(RdmaDeviceResources *dev_res, struct ibv_device_attr *dev_attr,
>                   Error **errp);
>  void rdma_rm_fini(RdmaDeviceResources *dev_res, RdmaBackendDev *backend_dev,
> diff --git a/hw/rdma/rdma_rm_defs.h b/hw/rdma/rdma_rm_defs.h
> index f0ee1f3072..4b8d704cfe 100644
> --- a/hw/rdma/rdma_rm_defs.h
> +++ b/hw/rdma/rdma_rm_defs.h

Still more stats gathering, I think.

[...]
> diff --git a/hw/rdma/vmw/pvrdma_main.c b/hw/rdma/vmw/pvrdma_main.c
> index b6061f4b6e..659331ac93 100644
> --- a/hw/rdma/vmw/pvrdma_main.c
> +++ b/hw/rdma/vmw/pvrdma_main.c
> @@ -25,6 +25,8 @@
>  #include "cpu.h"
>  #include "trace.h"
>  #include "sysemu/sysemu.h"
> +#include "monitor/monitor.h"
> +#include "hw/rdma/rdma_hmp.h"
>  
>  #include "../rdma_rm.h"
>  #include "../rdma_backend.h"
> @@ -55,6 +57,18 @@ static Property pvrdma_dev_properties[] = {
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> +static void pvrdma_print_statistics(Monitor *mon, RdmaStatsProvider *obj)
> +{
> +    PVRDMADev *dev = PVRDMA_DEV(obj);
> +    PCIDevice *pdev = PCI_DEVICE(dev);
> +
> +    monitor_printf(mon, "%s, %x.%x\n", pdev->name, PCI_SLOT(pdev->devfn),
> +                   PCI_FUNC(pdev->devfn));
> +    monitor_printf(mon, "\tcommands         : %" PRId64 "\n",
> +                   dev->stats.commands);
> +    rdma_dump_device_counters(mon, &dev->rdma_dev_res);
> +}
> +
>  static void free_dev_ring(PCIDevice *pci_dev, PvrdmaRing *ring,
>                            void *ring_state)
>  {
> @@ -394,6 +408,7 @@ static void pvrdma_regs_write(void *opaque, hwaddr addr, uint64_t val,

Still more stats gathering, I think.

[...]
> @@ -631,6 +648,7 @@ static void pvrdma_class_init(ObjectClass *klass, void *data)
>  {
>      DeviceClass *dc = DEVICE_CLASS(klass);
>      PCIDeviceClass *k = PCI_DEVICE_CLASS(klass);
> +    RdmaStatsProviderClass *ir = RDMA_STATS_PROVIDER_CLASS(klass);
>  
>      k->realize = pvrdma_realize;
>      k->exit = pvrdma_exit;
> @@ -642,6 +660,8 @@ static void pvrdma_class_init(ObjectClass *klass, void *data)
>      dc->desc = "RDMA Device";
>      dc->props = pvrdma_dev_properties;
>      set_bit(DEVICE_CATEGORY_NETWORK, dc->categories);
> +
> +    ir->print_statistics = pvrdma_print_statistics;
>  }
>  
>  static const TypeInfo pvrdma_info = {
> @@ -651,6 +671,7 @@ static const TypeInfo pvrdma_info = {
>      .class_init = pvrdma_class_init,
>      .interfaces = (InterfaceInfo[]) {
>          { INTERFACE_CONVENTIONAL_PCI_DEVICE },
> +        { TYPE_RDMA_STATS_PROVIDER },

We use INTERFACE_ rather than TYPE_ for some, but not all interfaces.
You follow TYPE_INTERRUPT_STATS_PROVIDER precedence.  We suck.  Not your
problem to solve.

>          { }
>      }
>  };
> diff --git a/include/hw/rdma/rdma_hmp.h b/include/hw/rdma/rdma_hmp.h
> new file mode 100644
> index 0000000000..dd23f2bc84
> --- /dev/null
> +++ b/include/hw/rdma/rdma_hmp.h
> @@ -0,0 +1,40 @@
> +/*
> + * RDMA device: Human Monitor interface
> + *
> + * Copyright (C) 2019 Oracle
> + * Copyright (C) 2019 Red Hat Inc
> + *
> + * Authors:
> + *     Yuval Shaia <yuval.shaia@oracle.com>
> + *
> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
> + * See the COPYING file in the top-level directory.
> + *
> + */
> +
> +#ifndef RDMA_HMP_H
> +#define RDMA_HMP_H
> +
> +#include "qom/object.h"
> +
> +#define TYPE_RDMA_STATS_PROVIDER "rdma"
> +
> +#define RDMA_STATS_PROVIDER_CLASS(klass) \
> +    OBJECT_CLASS_CHECK(RdmaStatsProviderClass, (klass), \
> +                       TYPE_RDMA_STATS_PROVIDER)
> +#define RDMA_STATS_PROVIDER_GET_CLASS(obj) \
> +    OBJECT_GET_CLASS(RdmaStatsProviderClass, (obj), \
> +                     TYPE_RDMA_STATS_PROVIDER)
> +#define RDMA_STATS_PROVIDER(obj) \
> +    INTERFACE_CHECK(RdmaStatsProvider, (obj), \
> +                    TYPE_RDMA_STATS_PROVIDER)
> +
> +typedef struct RdmaStatsProvider RdmaStatsProvider;
> +
> +typedef struct RdmaStatsProviderClass {
> +    InterfaceClass parent;
> +
> +    void (*print_statistics)(Monitor *mon, RdmaStatsProvider *obj);
> +} RdmaStatsProviderClass;
> +
> +#endif

I think using QOM to find the RDMA devices came out rather nicely.
Thanks for giving it a try.

Please consider the renames I recommended.

Acked-by: Markus Armbruster <armbru@redhat.com>