Re: [PATCH v2 1/7] tools: Make some callers of xc_domain_getinfo() use xc_domain_getinfolist()

["Marek Marczykowski-Górecki" <marmarek@invisiblethingslab.com>]

[-- Attachment #1: Type: text/plain, Size: 6441 bytes --]

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
> 
> 

-- 
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]