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

[Fouad Hilly <fouad.hilly@xxxxxxxxx>]

On Tue, Feb 27, 2024 at 6:32 PM Anthony PERARD <anthony.perard@xxxxxxxxx> wrote:
>
> On Tue, Feb 27, 2024 at 01:26:28PM +0000, Fouad Hilly wrote:
> > diff --git a/tools/libs/stat/xenstat_linux.c 
> > b/tools/libs/stat/xenstat_linux.c
> > index cbba54aa83ee..6d82e204aad4 100644
> > --- a/tools/libs/stat/xenstat_linux.c
> > +++ b/tools/libs/stat/xenstat_linux.c
> > @@ -390,6 +390,38 @@ void xenstat_uninit_networks(xenstat_handle * handle)
> >               fclose(priv->procnetdev);
> >  }
> >
> > +static int read_attributes_vbd3(char *vbd3_path, xenstat_vbd *vbd)
>
> "const char *vbd3_path"?
>
>
> >  static int read_attributes_vbd(const char *vbd_directory, const char 
> > *what, char *ret, int cap)
> >  {
> >       static char file_name[80];
> > @@ -438,7 +470,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);
>
> 255 is overly ambitious, but it match the size of buf, so I guess it's
> kind of ok, even if unnecessary.
>
> >               if (ret != 3)
> >                       continue;
> >               if (!(strstr(buf, "vbd")) && !(strstr(buf, "tap")))
> > @@ -448,6 +480,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;
>
> Yay for magic number... Do you think you could introduce an enum or
> define to replace this "3" by a meaningful? Maybe something like
> XENSTAT_VBD_TYPE_VBD3, (name derived from the existing function
> xenstat_vbd_type()).
>
> I'd like at least to replace the "3". But if you feel like having
> another patch to replace the "2" and "1", that would be a plus.
>
> >               else
> >                       vbd.back_type = 0;
> >
> > @@ -479,6 +513,35 @@ 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;
>
> `back_type` should already be 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);
> > +
> > +                     free(path);
> > +
> > +                     if (td3_pid == NULL)
> > +                             continue;
> > +
> > +                     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;
>
> Why sometime we do "continue" and sometime we do "vbd.error=1"?
continue is used when path to domain statistics is checked, which is
"dynamic"; However, if the path existed but failed to read statistics,
that is when we set the error.

>
> > +
> > +                     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;
> > +};
>
> Is that a binary interface for an external project? I'm asking because
> `__pad` would seems useless otherwise.
> If it is part of an interface, please add a comment about it, add a link
> to the project/source where this interface is described, and where is
> the canonical location? Also, is there an header we could maybe just
> use if it's in the system or an header we could import into the
> repository?
>
> Thanks,
>
> --
> Anthony PERARD