| Message ID | 20230426145932.3340-2-alejandro.vallejo@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Rationalize usage of xc_domain_getinfo{,list}() | expand |
Just as a note for the subject, we more commonly write function names with ()'s. On 26/04/2023 3:59 pm, Alejandro Vallejo wrote: > diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h > index 05967ecc92..90b33aa3a7 100644 > --- a/tools/include/xenctrl.h > +++ b/tools/include/xenctrl.h > @@ -468,6 +468,11 @@ 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; > +} > + > 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..38212e8091 100644 > --- a/tools/python/xen/lowlevel/xc/xc.c > +++ b/tools/python/xen/lowlevel/xc/xc.c > @@ -368,21 +368,22 @@ 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", (info[i].flags & XEN_DOMINF_shutdown) && > + (dominfo_shutdown_reason(&info[i]) == SHUTDOWN_crash), Isn't this your dominfo_shutdown_with() from patch 6 ? I'd pull that forward to this patch too, and use it here. > + "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); Not to do with your patch, but this is logic is mad. xenbaked open and closes a xenctrl handle every time its set of domids changes. I'm very seriously tempted to delete all of tools/xenmon because it shows no signs of being in use, and right now all it does is spit out an unending stream of gotten<100ns in qos_switchout(domid=32767) gotten<100ns in qos_switchout(domid=0) to stdout, which is antisocial for something calling itself a daemon. ~Andrew
Answers inlined On Thu, Apr 27, 2023 at 10:51:03AM +0100, Andrew Cooper wrote: > Just as a note for the subject, we more commonly write function names > with ()'s. No harm in abiding by that. Done on v2. > > + "crashed", (info[i].flags & XEN_DOMINF_shutdown) && > > + (dominfo_shutdown_reason(&info[i]) == SHUTDOWN_crash), > > Isn't this your dominfo_shutdown_with() from patch 6 ? > > I'd pull that forward to this patch too, and use it here. It is indeed. Done locally on v2. Cheers, Alejandro
diff --git a/tools/include/xenctrl.h b/tools/include/xenctrl.h index 05967ecc92..90b33aa3a7 100644 --- a/tools/include/xenctrl.h +++ b/tools/include/xenctrl.h @@ -468,6 +468,11 @@ 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; +} + 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..38212e8091 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,22 @@ 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", (info[i].flags & XEN_DOMINF_shutdown) && + (dominfo_shutdown_reason(&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 | 5 +++++ tools/python/xen/lowlevel/xc/xc.c | 29 +++++++++++++++-------------- tools/xenmon/xenbaked.c | 6 +++--- 3 files changed, 23 insertions(+), 17 deletions(-)