| Message ID | 20230428104124.1044-2-alejandro.vallejo@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Rationalize usage of xc_domain_getinfo{,list}() | expand |
On Fri, Apr 28, 2023 at 11:41:18AM +0100, Alejandro Vallejo wrote: > xc_domain_getinfo() is slow and prone to races because N hypercalls are > needed to find information about N domains. xc_domain_getinfolist() finds > the same information in a single hypercall as long as a big enough buffer > is provided. Plus, xc_domain_getinfo() is disappearing on a future patch > so migrate the callers interested in more than 1 domain to the the *list() > version. > > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> For the Python part: Acked-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com> > --- > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: Wei Liu <wl@xen.org> > Cc: Anthony PERARD <anthony.perard@citrix.com> > Cc: Juergen Gross <jgross@suse.com> > --- > tools/include/xenctrl.h | 12 ++++++++++++ > tools/python/xen/lowlevel/xc/xc.c | 28 ++++++++++++++-------------- > tools/xenmon/xenbaked.c | 6 +++--- > 3 files changed, 29 insertions(+), 17 deletions(-) > > diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h > index 05967ecc92..f5bc7f58b6 100644 > --- a/tools/include/xenctrl.h > +++ b/tools/include/xenctrl.h > @@ -468,6 +468,18 @@ typedef struct xc_dominfo { > > typedef xen_domctl_getdomaininfo_t xc_domaininfo_t; > > +static inline unsigned int dominfo_shutdown_reason(const xc_domaininfo_t *info) > +{ > + return (info->flags >> XEN_DOMINF_shutdownshift) & XEN_DOMINF_shutdownmask; > +} > + > +static inline bool dominfo_shutdown_with(xc_domaininfo_t *info, unsigned int expected_reason) > +{ > + /* The reason doesn't make sense unless the domain is actually shutdown */ > + return (info->flags & XEN_DOMINF_shutdown) && > + (dominfo_shutdown_reason(info) == expected_reason); > +} > + > typedef union > { > #if defined(__i386__) || defined(__x86_64__) > diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c > index 35901c2d63..d7ce299650 100644 > --- a/tools/python/xen/lowlevel/xc/xc.c > +++ b/tools/python/xen/lowlevel/xc/xc.c > @@ -342,7 +342,7 @@ static PyObject *pyxc_domain_getinfo(XcObject *self, > uint32_t first_dom = 0; > int max_doms = 1024, nr_doms, i; > size_t j; > - xc_dominfo_t *info; > + xc_domaininfo_t *info; > > static char *kwd_list[] = { "first_dom", "max_doms", NULL }; > > @@ -350,11 +350,11 @@ static PyObject *pyxc_domain_getinfo(XcObject *self, > &first_dom, &max_doms) ) > return NULL; > > - info = calloc(max_doms, sizeof(xc_dominfo_t)); > + info = calloc(max_doms, sizeof(*info)); > if (info == NULL) > return PyErr_NoMemory(); > > - nr_doms = xc_domain_getinfo(self->xc_handle, first_dom, max_doms, info); > + nr_doms = xc_domain_getinfolist(self->xc_handle, first_dom, max_doms, info); > > if (nr_doms < 0) > { > @@ -368,21 +368,21 @@ static PyObject *pyxc_domain_getinfo(XcObject *self, > info_dict = Py_BuildValue( > "{s:i,s:i,s:i,s:i,s:i,s:i,s:i,s:i,s:i,s:i" > ",s:L,s:L,s:L,s:i,s:i,s:i}", > - "domid", (int)info[i].domid, > + "domid", (int)info[i].domain, > "online_vcpus", info[i].nr_online_vcpus, > "max_vcpu_id", info[i].max_vcpu_id, > - "hvm", info[i].hvm, > - "dying", info[i].dying, > - "crashed", info[i].crashed, > - "shutdown", info[i].shutdown, > - "paused", info[i].paused, > - "blocked", info[i].blocked, > - "running", info[i].running, > - "mem_kb", (long long)info[i].nr_pages*(XC_PAGE_SIZE/1024), > + "hvm", !!(info[i].flags & XEN_DOMINF_hvm_guest), > + "dying", !!(info[i].flags & XEN_DOMINF_dying), > + "crashed", dominfo_shutdown_with(&info[i], SHUTDOWN_crash), > + "shutdown", !!(info[i].flags & XEN_DOMINF_shutdown), > + "paused", !!(info[i].flags & XEN_DOMINF_paused), > + "blocked", !!(info[i].flags & XEN_DOMINF_blocked), > + "running", !!(info[i].flags & XEN_DOMINF_running), > + "mem_kb", (long long)info[i].tot_pages*(XC_PAGE_SIZE/1024), > "cpu_time", (long long)info[i].cpu_time, > - "maxmem_kb", (long long)info[i].max_memkb, > + "maxmem_kb", (long long)(info[i].max_pages << (XC_PAGE_SHIFT - 10)), > "ssidref", (int)info[i].ssidref, > - "shutdown_reason", info[i].shutdown_reason, > + "shutdown_reason", dominfo_shutdown_reason(&info[i]), > "cpupool", (int)info[i].cpupool); > pyhandle = PyList_New(sizeof(xen_domain_handle_t)); > if ( (pyhandle == NULL) || (info_dict == NULL) ) > diff --git a/tools/xenmon/xenbaked.c b/tools/xenmon/xenbaked.c > index 4dddbd20e2..8632b10ea4 100644 > --- a/tools/xenmon/xenbaked.c > +++ b/tools/xenmon/xenbaked.c > @@ -775,7 +775,7 @@ static void global_init_domain(int domid, int idx) > static int indexof(int domid) > { > int idx; > - xc_dominfo_t dominfo[NDOMAINS]; > + xc_domaininfo_t dominfo[NDOMAINS]; > xc_interface *xc_handle; > int ndomains; > > @@ -797,7 +797,7 @@ static int indexof(int domid) > > // call domaininfo hypercall to try and garbage collect unused entries > xc_handle = xc_interface_open(0,0,0); > - ndomains = xc_domain_getinfo(xc_handle, 0, NDOMAINS, dominfo); > + ndomains = xc_domain_getinfolist(xc_handle, 0, NDOMAINS, dominfo); > xc_interface_close(xc_handle); > > // for each domain in our data, look for it in the system dominfo structure > @@ -808,7 +808,7 @@ static int indexof(int domid) > int jdx; > > for (jdx=0; jdx<ndomains; jdx++) { > - if (dominfo[jdx].domid == domid) > + if (dominfo[jdx].domain == domid) > break; > } > if (jdx == ndomains) // we didn't find domid in the dominfo struct > -- > 2.34.1 > >
On 28/04/2023 11:41 am, Alejandro Vallejo wrote: > xc_domain_getinfo() is slow and prone to races because N hypercalls are > needed to find information about N domains. xc_domain_getinfolist() finds > the same information in a single hypercall as long as a big enough buffer > is provided. Plus, xc_domain_getinfo() is disappearing on a future patch > so migrate the callers interested in more than 1 domain to the the *list() > version. > > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> > --- > Cc: Andrew Cooper <andrew.cooper3@citrix.com> > Cc: Wei Liu <wl@xen.org> > Cc: Anthony PERARD <anthony.perard@citrix.com> > Cc: Juergen Gross <jgross@suse.com> FYI, expected practice is to put your v2 notes in each patch, here in the commit message. They're more accessible here than in the cover letter. However, this patch is now fine, so Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h index 05967ecc92..f5bc7f58b6 100644 --- a/tools/include/xenctrl.h +++ b/tools/include/xenctrl.h @@ -468,6 +468,18 @@ typedef struct xc_dominfo { typedef xen_domctl_getdomaininfo_t xc_domaininfo_t; +static inline unsigned int dominfo_shutdown_reason(const xc_domaininfo_t *info) +{ + return (info->flags >> XEN_DOMINF_shutdownshift) & XEN_DOMINF_shutdownmask; +} + +static inline bool dominfo_shutdown_with(xc_domaininfo_t *info, unsigned int expected_reason) +{ + /* The reason doesn't make sense unless the domain is actually shutdown */ + return (info->flags & XEN_DOMINF_shutdown) && + (dominfo_shutdown_reason(info) == expected_reason); +} + typedef union { #if defined(__i386__) || defined(__x86_64__) diff --git a/tools/python/xen/lowlevel/xc/xc.c b/tools/python/xen/lowlevel/xc/xc.c index 35901c2d63..d7ce299650 100644 --- a/tools/python/xen/lowlevel/xc/xc.c +++ b/tools/python/xen/lowlevel/xc/xc.c @@ -342,7 +342,7 @@ static PyObject *pyxc_domain_getinfo(XcObject *self, uint32_t first_dom = 0; int max_doms = 1024, nr_doms, i; size_t j; - xc_dominfo_t *info; + xc_domaininfo_t *info; static char *kwd_list[] = { "first_dom", "max_doms", NULL }; @@ -350,11 +350,11 @@ static PyObject *pyxc_domain_getinfo(XcObject *self, &first_dom, &max_doms) ) return NULL; - info = calloc(max_doms, sizeof(xc_dominfo_t)); + info = calloc(max_doms, sizeof(*info)); if (info == NULL) return PyErr_NoMemory(); - nr_doms = xc_domain_getinfo(self->xc_handle, first_dom, max_doms, info); + nr_doms = xc_domain_getinfolist(self->xc_handle, first_dom, max_doms, info); if (nr_doms < 0) { @@ -368,21 +368,21 @@ static PyObject *pyxc_domain_getinfo(XcObject *self, info_dict = Py_BuildValue( "{s:i,s:i,s:i,s:i,s:i,s:i,s:i,s:i,s:i,s:i" ",s:L,s:L,s:L,s:i,s:i,s:i}", - "domid", (int)info[i].domid, + "domid", (int)info[i].domain, "online_vcpus", info[i].nr_online_vcpus, "max_vcpu_id", info[i].max_vcpu_id, - "hvm", info[i].hvm, - "dying", info[i].dying, - "crashed", info[i].crashed, - "shutdown", info[i].shutdown, - "paused", info[i].paused, - "blocked", info[i].blocked, - "running", info[i].running, - "mem_kb", (long long)info[i].nr_pages*(XC_PAGE_SIZE/1024), + "hvm", !!(info[i].flags & XEN_DOMINF_hvm_guest), + "dying", !!(info[i].flags & XEN_DOMINF_dying), + "crashed", dominfo_shutdown_with(&info[i], SHUTDOWN_crash), + "shutdown", !!(info[i].flags & XEN_DOMINF_shutdown), + "paused", !!(info[i].flags & XEN_DOMINF_paused), + "blocked", !!(info[i].flags & XEN_DOMINF_blocked), + "running", !!(info[i].flags & XEN_DOMINF_running), + "mem_kb", (long long)info[i].tot_pages*(XC_PAGE_SIZE/1024), "cpu_time", (long long)info[i].cpu_time, - "maxmem_kb", (long long)info[i].max_memkb, + "maxmem_kb", (long long)(info[i].max_pages << (XC_PAGE_SHIFT - 10)), "ssidref", (int)info[i].ssidref, - "shutdown_reason", info[i].shutdown_reason, + "shutdown_reason", dominfo_shutdown_reason(&info[i]), "cpupool", (int)info[i].cpupool); pyhandle = PyList_New(sizeof(xen_domain_handle_t)); if ( (pyhandle == NULL) || (info_dict == NULL) ) diff --git a/tools/xenmon/xenbaked.c b/tools/xenmon/xenbaked.c index 4dddbd20e2..8632b10ea4 100644 --- a/tools/xenmon/xenbaked.c +++ b/tools/xenmon/xenbaked.c @@ -775,7 +775,7 @@ static void global_init_domain(int domid, int idx) static int indexof(int domid) { int idx; - xc_dominfo_t dominfo[NDOMAINS]; + xc_domaininfo_t dominfo[NDOMAINS]; xc_interface *xc_handle; int ndomains; @@ -797,7 +797,7 @@ static int indexof(int domid) // call domaininfo hypercall to try and garbage collect unused entries xc_handle = xc_interface_open(0,0,0); - ndomains = xc_domain_getinfo(xc_handle, 0, NDOMAINS, dominfo); + ndomains = xc_domain_getinfolist(xc_handle, 0, NDOMAINS, dominfo); xc_interface_close(xc_handle); // for each domain in our data, look for it in the system dominfo structure @@ -808,7 +808,7 @@ static int indexof(int domid) int jdx; for (jdx=0; jdx<ndomains; jdx++) { - if (dominfo[jdx].domid == domid) + if (dominfo[jdx].domain == domid) break; } if (jdx == ndomains) // we didn't find domid in the dominfo struct
xc_domain_getinfo() is slow and prone to races because N hypercalls are needed to find information about N domains. xc_domain_getinfolist() finds the same information in a single hypercall as long as a big enough buffer is provided. Plus, xc_domain_getinfo() is disappearing on a future patch so migrate the callers interested in more than 1 domain to the the *list() version. Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> --- Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: Wei Liu <wl@xen.org> Cc: Anthony PERARD <anthony.perard@citrix.com> Cc: Juergen Gross <jgross@suse.com> --- tools/include/xenctrl.h | 12 ++++++++++++ tools/python/xen/lowlevel/xc/xc.c | 28 ++++++++++++++-------------- tools/xenmon/xenbaked.c | 6 +++--- 3 files changed, 29 insertions(+), 17 deletions(-)