Re: [PATCH] tools/xentop: Add VBD3 support to xentop

[Frediano Ziglio <frediano.ziglio@xxxxxxxxx>]

On Mon, Feb 26, 2024 at 2:12 PM Fouad Hilly <fouad.hilly@xxxxxxxxx> wrote:
>
> From: Pritha Srivastava <pritha.srivastava@xxxxxxxxxx>
>
> xl now knows how to drive tapdisk, so modified libxenstat to
> understand vbd3 statistics.
>
> Signed-off-by: Jorge Martin <jorge.martin@xxxxxxxxxx>
> Signed-off-by: Pritha Srivastava <pritha.srivastava@xxxxxxxxxx>
> Signed-off-by: Fouad Hilly <fouad.hilly@xxxxxxxxx>
> ---
> CC: Wei Liu <wl@xxxxxxx>
> CC: Anthony PERARD <anthony.perard@xxxxxxxxxx>
> CC: Juergen Gross <jgross@xxxxxxxx>
> ---
>  tools/libs/stat/xenstat_linux.c | 69 ++++++++++++++++++++++++++++++++-
>  tools/libs/stat/xenstat_priv.h  | 16 ++++++++
>  tools/xentop/xentop.c           |  1 +
>  3 files changed, 85 insertions(+), 1 deletion(-)
>
> diff --git a/tools/libs/stat/xenstat_linux.c b/tools/libs/stat/xenstat_linux.c
> index cbba54aa83ee..5a4a03634182 100644
> --- a/tools/libs/stat/xenstat_linux.c
> +++ b/tools/libs/stat/xenstat_linux.c
> @@ -390,6 +390,39 @@ void xenstat_uninit_networks(xenstat_handle * handle)
>                 fclose(priv->procnetdev);
>  }
>
> +static int read_attributes_vbd3(char *vbd3_path, xenstat_vbd *vbd)
> +{
> +       FILE *fp;
> +       struct vbd3_stats vbd3_stats;
> +
> +       fp = fopen(vbd3_path, "rb");
> +
> +       if (fp == NULL)
> +       {

The syntax of this if statement is different from the ones below.

> +               return -1;
> +       }
> +
> +       if (fread(&vbd3_stats, sizeof(struct vbd3_stats), 1, fp) != 1) {
> +               fclose(fp);
> +               return -1;
> +       }
> +
> +       if (vbd3_stats.version != 1) {
> +               fclose(fp);
> +               return -1;
> +       }
> +
> +       vbd->oo_reqs = vbd3_stats.oo_reqs;
> +       vbd->rd_reqs = vbd3_stats.read_reqs_submitted;
> +       vbd->rd_sects = vbd3_stats.read_sectors;
> +       vbd->wr_reqs = vbd3_stats.write_reqs_submitted;
> +       vbd->wr_sects = vbd3_stats.write_sectors;
> +
> +       fclose(fp);
> +
> +       return 0;
> +}
> +
>  static int read_attributes_vbd(const char *vbd_directory, const char *what, 
> char *ret, int cap)
>  {
>         static char file_name[80];
> @@ -438,7 +471,7 @@ int xenstat_collect_vbds(xenstat_node * node)
>                 int ret;
>                 char buf[256];
>
> -               ret = sscanf(dp->d_name, "%3s-%u-%u", buf, &domid, &vbd.dev);
> +               ret = sscanf(dp->d_name, "%255[^-]-%u-%u", buf, &domid, 
> &vbd.dev);
>                 if (ret != 3)
>                         continue;
>                 if (!(strstr(buf, "vbd")) && !(strstr(buf, "tap")))
> @@ -448,6 +481,8 @@ int xenstat_collect_vbds(xenstat_node * node)
>                         vbd.back_type = 1;
>                 else if (strcmp(buf,"tap") == 0)
>                         vbd.back_type = 2;
> +               else if (strcmp(buf,"vbd3") == 0)
> +                       vbd.back_type = 3;
>                 else
>                         vbd.back_type = 0;
>
> @@ -479,6 +514,38 @@ int xenstat_collect_vbds(xenstat_node * node)
>                                 vbd.error = 1;
>                         }
>                 }
> +               else if (vbd.back_type == 3)
> +               {
> +
> +                       char *td3_pid;
> +                       char *path;
> +
> +                       vbd.back_type = 3;
> +                       vbd.error = 0;
> +
> +                       if (asprintf(&path, 
> "/local/domain/0/backend/vbd3/%u/%u/kthread-pid", domid, vbd.dev) < 0)
> +                               continue;
> +
> +                       td3_pid = xs_read(node->handle->xshandle, XBT_NULL, 
> path, NULL);
> +
> +                       if (td3_pid == NULL) {
> +                               free(path);
> +                               continue;
> +                       }
> +
> +                       free(path);

Why not freeing "path" unconditionally? We free it in all cases.

> +
> +                       if (asprintf(&path, "/dev/shm/td3-%s/vbd-%u-%u", 
> td3_pid, domid, vbd.dev) < 0) {
> +                               free(td3_pid);
> +                               continue;
> +                       }
> +
> +                       if (read_attributes_vbd3(path, &vbd) < 0)
> +                               vbd.error = 1;
> +
> +                       free(td3_pid);
> +                       free(path);
> +               }
>                 else
>                 {
>                         vbd.error = 1;
> diff --git a/tools/libs/stat/xenstat_priv.h b/tools/libs/stat/xenstat_priv.h
> index 4eb44a8ebb84..c3a9635240e9 100644
> --- a/tools/libs/stat/xenstat_priv.h
> +++ b/tools/libs/stat/xenstat_priv.h
> @@ -98,6 +98,22 @@ struct xenstat_vbd {
>         unsigned long long wr_sects;
>  };
>
> +struct vbd3_stats {
> +       uint32_t version;
> +       uint32_t __pad;
> +       uint64_t oo_reqs;
> +       uint64_t read_reqs_submitted;
> +       uint64_t read_reqs_completed;
> +       uint64_t read_sectors;
> +       uint64_t read_total_ticks;
> +       uint64_t write_reqs_submitted;
> +       uint64_t write_reqs_completed;
> +       uint64_t write_sectors;
> +       uint64_t write_total_ticks;
> +       uint64_t io_errors;
> +       uint64_t flags;
> +};
> +
>  extern int xenstat_collect_networks(xenstat_node * node);
>  extern void xenstat_uninit_networks(xenstat_handle * handle);
>  extern int xenstat_collect_vbds(xenstat_node * node);
> diff --git a/tools/xentop/xentop.c b/tools/xentop/xentop.c
> index 0a2fab7f15a3..f5a456fd4dfd 100644
> --- a/tools/xentop/xentop.c
> +++ b/tools/xentop/xentop.c
> @@ -1124,6 +1124,7 @@ void do_vbd(xenstat_domain *domain)
>                 "Unidentified",           /* number 0 */
>                 "BlkBack",           /* number 1 */
>                 "BlkTap",            /* number 2 */
> +               "Tapdisk3"           /* number 3 */
>         };
>
>         num_vbds = xenstat_domain_num_vbds(domain);
> --
> 2.42.0
>
>