| Message ID | 20240227132628.2157031-1-fouad.hilly@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] tools/xentop: Add VBD3 support to xentop | expand |
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"? > + > + 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,
On Tue, Feb 27, 2024 at 6:32 PM Anthony PERARD <anthony.perard@cloud.com> 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
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) +{ + FILE *fp; + struct vbd3_stats vbd3_stats; + + fp = fopen(vbd3_path, "rb"); + + if (fp == NULL) { + 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 +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); 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; 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; + 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; + + 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);