| Message ID | 20240226141211.1416866-1-fouad.hilly@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools/xentop: Add VBD3 support to xentop | expand |
On 26.02.2024 15:12, Fouad Hilly wrote: > From: Pritha Srivastava <pritha.srivastava@citrix.com> > > xl now knows how to drive tapdisk, so modified libxenstat to > understand vbd3 statistics. > > Signed-off-by: Jorge Martin <jorge.martin@citrix.com> > Signed-off-by: Pritha Srivastava <pritha.srivastava@citrix.com> Just a formal question (I'm not really qualified to review this change): With the above two S-o-b and the earlier From: - who is the original author of this patch? The general expectation is for the 1st S-o-b to be the author's. Jan > Signed-off-by: Fouad Hilly <fouad.hilly@cloud.com> > --- > CC: Wei Liu <wl@xen.org> > CC: Anthony PERARD <anthony.perard@citrix.com> > CC: Juergen Gross <jgross@suse.com> > --- > 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) > + { > + 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); > + > + 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);
On Mon, Feb 26, 2024 at 2:12 PM Fouad Hilly <fouad.hilly@cloud.com> wrote: > > From: Pritha Srivastava <pritha.srivastava@citrix.com> > > xl now knows how to drive tapdisk, so modified libxenstat to > understand vbd3 statistics. > > Signed-off-by: Jorge Martin <jorge.martin@citrix.com> > Signed-off-by: Pritha Srivastava <pritha.srivastava@citrix.com> > Signed-off-by: Fouad Hilly <fouad.hilly@cloud.com> > --- > CC: Wei Liu <wl@xen.org> > CC: Anthony PERARD <anthony.perard@citrix.com> > CC: Juergen Gross <jgross@suse.com> > --- > 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 > >
On 26/02/2024 2:22 pm, Jan Beulich wrote: > On 26.02.2024 15:12, Fouad Hilly wrote: >> From: Pritha Srivastava <pritha.srivastava@citrix.com> >> >> xl now knows how to drive tapdisk, so modified libxenstat to >> understand vbd3 statistics. >> >> Signed-off-by: Jorge Martin <jorge.martin@citrix.com> >> Signed-off-by: Pritha Srivastava <pritha.srivastava@citrix.com> > Just a formal question (I'm not really qualified to review this change): > With the above two S-o-b and the earlier From: - who is the original > author of this patch? The general expectation is for the 1st S-o-b to > be the author's. This patch has been sat in the XenServer patchqueue for a decade. Neither Pritha nor Jorge are with us any more. Sadly the review system we used back then is also no longer with us. From ticketing, I think it was co-developed at the same time. This is the form the patch has existed in the patchqueue for that time, so I'm tempted to say we reorder the SoB chain to make it match. That's the best I can figure out. ~Andrew
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) + { + 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); + + 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);