| Message ID | 20230502111338.16757-2-alejandro.vallejo@cloud.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Rationalize usage of xc_domain_getinfo{,list}() | expand |
On Tue, May 02, 2023 at 12:13:36PM +0100, Alejandro Vallejo wrote: > xc_domain_getinfolist() internally relies on a sysctl that performs > a linear search for the domids. Many callers of xc_domain_getinfolist() > who require information about a precise domid are much better off calling > xc_domain_getinfo_single() instead, that will use the getdomaininfo domctl > instead and ensure the returned domid matches the requested one. The domtctl > will find the domid faster too, because that uses hashed lists. > > 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> > Cc: Christian Lindig <christian.lindig@citrix.com> > > v3: > * Replaced single-domid xc_domain_getinfolist() call in ocaml stub with > xc_domain_getinfo_single() My mistake here. It's supposed to have a "R-by: Andrew Cooper"
> On 2 May 2023, at 12:13, Alejandro Vallejo <alejandro.vallejo@cloud.com> wrote: > > xc_domain_getinfolist() internally relies on a sysctl that performs > a linear search for the domids. Many callers of xc_domain_getinfolist() > who require information about a precise domid are much better off calling > xc_domain_getinfo_single() instead, that will use the getdomaininfo domctl > instead and ensure the returned domid matches the requested one. The domtctl > will find the domid faster too, because that uses hashed lists. > > Signed-off-by: Alejandro Vallejo <alejandro.vallejo@cloud.com> Acked-by: Christian Lindig <christian.lindig@cloud.com> I mostly care about the OCaml bindings - looks good to me. — C
On Tue, May 02, 2023 at 12:13:36PM +0100, Alejandro Vallejo wrote: > diff --git a/tools/libs/light/libxl_domain.c b/tools/libs/light/libxl_domain.c > index 7f0986c185..5709b3e62f 100644 > --- a/tools/libs/light/libxl_domain.c > +++ b/tools/libs/light/libxl_domain.c > @@ -349,16 +349,12 @@ int libxl_domain_info(libxl_ctx *ctx, libxl_dominfo *info_r, > int ret; > GC_INIT(ctx); > > - ret = xc_domain_getinfolist(ctx->xch, domid, 1, &xcinfo); > + ret = xc_domain_getinfo_single(ctx->xch, domid, &xcinfo); > if (ret<0) { > - LOGED(ERROR, domid, "Getting domain info list"); > + LOGED(ERROR, domid, "Getting domain info"); > GC_FREE; > return ERROR_FAIL; > } > - if (ret==0 || xcinfo.domain != domid) { > - GC_FREE; > - return ERROR_DOMAIN_NOTFOUND; I kind of think we should keep returning ERROR_DOMAIN_NOTFOUND on error as this is the most likely explanation. Also, the comment for this function in libxl.h explain this: /* May be called with info_r == NULL to check for domain's existence. * Returns ERROR_DOMAIN_NOTFOUND if domain does not exist (used to return * ERROR_INVAL for this scenario). */ int libxl_domain_info(libxl_ctx*, libxl_dominfo *info_r, uint32_t domid); Would it be possible to find out from xc_domain_getinfo_single() if it's a domain not found scenario vs other error (like permission denied)? > - } > > if (info_r) > libxl__xcinfo2xlinfo(ctx, &xcinfo, info_r); > @@ -1657,14 +1653,15 @@ int libxl__resolve_domid(libxl__gc *gc, const char *name, uint32_t *domid) > libxl_vcpuinfo *libxl_list_vcpu(libxl_ctx *ctx, uint32_t domid, > int *nr_vcpus_out, int *nr_cpus_out) > { > + int rc; > GC_INIT(ctx); > libxl_vcpuinfo *ptr, *ret; > xc_domaininfo_t domaininfo; > xc_vcpuinfo_t vcpuinfo; > unsigned int nr_vcpus; > > - if (xc_domain_getinfolist(ctx->xch, domid, 1, &domaininfo) != 1) { > - LOGED(ERROR, domid, "Getting infolist"); > + if ((rc = xc_domain_getinfo_single(ctx->xch, domid, &domaininfo)) < 0) { The variable name "rc" is reserved for libxl return code. For syscall and other external call, we should use the name "r". (I know that in other part of this patch the name used is "ret", but as it is already existing in the code base, I'm not asking for a change elsewhere) Also, assignment to the variable should be done outside of the if(). So the new code should look like: r = xc_domain_getinfolist(...); if (r < 0) { (There's tools/libs/light/CODING_STYLE for all this explained) > + LOGED(ERROR, domid, "Getting dominfo"); > GC_FREE; > return NULL; > } > diff --git a/tools/libs/light/libxl_sched.c b/tools/libs/light/libxl_sched.c > index 7c53dc60e6..21a65442c0 100644 > --- a/tools/libs/light/libxl_sched.c > +++ b/tools/libs/light/libxl_sched.c > @@ -219,13 +219,11 @@ static int sched_credit_domain_set(libxl__gc *gc, uint32_t domid, > xc_domaininfo_t domaininfo; > int rc; > > - rc = xc_domain_getinfolist(CTX->xch, domid, 1, &domaininfo); > + rc = xc_domain_getinfo_single(CTX->xch, domid, &domaininfo); > if (rc < 0) { > - LOGED(ERROR, domid, "Getting domain info list"); > + LOGED(ERROR, domid, "Getting domain info"); > return ERROR_FAIL; > } > - if (rc != 1 || domaininfo.domain != domid) > - return ERROR_INVAL; We can probably return ERROR_INVAL on error instead of ERROR_FAIL, as I guess it's more likely that we try to change a non-existing domain rather than having another error. > > rc = xc_sched_credit_domain_get(CTX->xch, domid, &sdom); > if (rc != 0) { > @@ -426,13 +424,11 @@ static int sched_credit2_domain_set(libxl__gc *gc, uint32_t domid, > xc_domaininfo_t info; > int rc; > > - rc = xc_domain_getinfolist(CTX->xch, domid, 1, &info); > + rc = xc_domain_getinfo_single(CTX->xch, domid, &info); > if (rc < 0) { > LOGED(ERROR, domid, "Getting domain info"); > return ERROR_FAIL; dito. > } > - if (rc != 1 || info.domain != domid) > - return ERROR_INVAL; > > rc = xc_sched_credit2_domain_get(CTX->xch, domid, &sdom); > if (rc != 0) { Thanks,
diff --git a/tools/libs/light/libxl_dom.c b/tools/libs/light/libxl_dom.c index 25fb716084..94fef37401 100644 --- a/tools/libs/light/libxl_dom.c +++ b/tools/libs/light/libxl_dom.c @@ -32,9 +32,9 @@ libxl_domain_type libxl__domain_type(libxl__gc *gc, uint32_t domid) xc_domaininfo_t info; int ret; - ret = xc_domain_getinfolist(ctx->xch, domid, 1, &info); - if (ret != 1 || info.domain != domid) { - LOG(ERROR, "unable to get domain type for domid=%"PRIu32, domid); + ret = xc_domain_getinfo_single(ctx->xch, domid, &info); + if (ret < 0) { + LOGED(ERROR, domid, "unable to get dominfo"); return LIBXL_DOMAIN_TYPE_INVALID; } if (info.flags & XEN_DOMINF_hvm_guest) { @@ -70,15 +70,10 @@ int libxl__domain_cpupool(libxl__gc *gc, uint32_t domid) xc_domaininfo_t info; int ret; - ret = xc_domain_getinfolist(CTX->xch, domid, 1, &info); - if (ret != 1) + ret = xc_domain_getinfo_single(CTX->xch, domid, &info); + if (ret < 0) { - LOGE(ERROR, "getinfolist failed %d", ret); - return ERROR_FAIL; - } - if (info.domain != domid) - { - LOGE(ERROR, "got info for dom%d, wanted dom%d\n", info.domain, domid); + LOGED(ERROR, domid, "get domaininfo failed"); return ERROR_FAIL; } return info.cpupool; diff --git a/tools/libs/light/libxl_dom_suspend.c b/tools/libs/light/libxl_dom_suspend.c index 4fa22bb739..6091a5f3f6 100644 --- a/tools/libs/light/libxl_dom_suspend.c +++ b/tools/libs/light/libxl_dom_suspend.c @@ -332,13 +332,8 @@ static void suspend_common_wait_guest_check(libxl__egc *egc, /* Convenience aliases */ const uint32_t domid = dsps->domid; - ret = xc_domain_getinfolist(CTX->xch, domid, 1, &info); + ret = xc_domain_getinfo_single(CTX->xch, domid, &info); if (ret < 0) { - LOGED(ERROR, domid, "unable to check for status of guest"); - goto err; - } - - if (!(ret == 1 && info.domain == domid)) { LOGED(ERROR, domid, "guest we were suspending has been destroyed"); goto err; } diff --git a/tools/libs/light/libxl_domain.c b/tools/libs/light/libxl_domain.c index 7f0986c185..5709b3e62f 100644 --- a/tools/libs/light/libxl_domain.c +++ b/tools/libs/light/libxl_domain.c @@ -349,16 +349,12 @@ int libxl_domain_info(libxl_ctx *ctx, libxl_dominfo *info_r, int ret; GC_INIT(ctx); - ret = xc_domain_getinfolist(ctx->xch, domid, 1, &xcinfo); + ret = xc_domain_getinfo_single(ctx->xch, domid, &xcinfo); if (ret<0) { - LOGED(ERROR, domid, "Getting domain info list"); + LOGED(ERROR, domid, "Getting domain info"); GC_FREE; return ERROR_FAIL; } - if (ret==0 || xcinfo.domain != domid) { - GC_FREE; - return ERROR_DOMAIN_NOTFOUND; - } if (info_r) libxl__xcinfo2xlinfo(ctx, &xcinfo, info_r); @@ -1657,14 +1653,15 @@ int libxl__resolve_domid(libxl__gc *gc, const char *name, uint32_t *domid) libxl_vcpuinfo *libxl_list_vcpu(libxl_ctx *ctx, uint32_t domid, int *nr_vcpus_out, int *nr_cpus_out) { + int rc; GC_INIT(ctx); libxl_vcpuinfo *ptr, *ret; xc_domaininfo_t domaininfo; xc_vcpuinfo_t vcpuinfo; unsigned int nr_vcpus; - if (xc_domain_getinfolist(ctx->xch, domid, 1, &domaininfo) != 1) { - LOGED(ERROR, domid, "Getting infolist"); + if ((rc = xc_domain_getinfo_single(ctx->xch, domid, &domaininfo)) < 0) { + LOGED(ERROR, domid, "Getting dominfo"); GC_FREE; return NULL; } diff --git a/tools/libs/light/libxl_mem.c b/tools/libs/light/libxl_mem.c index 92ec09f4cf..44e554adba 100644 --- a/tools/libs/light/libxl_mem.c +++ b/tools/libs/light/libxl_mem.c @@ -323,8 +323,8 @@ retry_transaction: libxl__xs_printf(gc, t, GCSPRINTF("%s/memory/target", dompath), "%"PRIu64, new_target_memkb); - r = xc_domain_getinfolist(ctx->xch, domid, 1, &info); - if (r != 1 || info.domain != domid) { + r = xc_domain_getinfo_single(ctx->xch, domid, &info); + if (r < 0) { abort_transaction = 1; rc = ERROR_FAIL; goto out; diff --git a/tools/libs/light/libxl_sched.c b/tools/libs/light/libxl_sched.c index 7c53dc60e6..21a65442c0 100644 --- a/tools/libs/light/libxl_sched.c +++ b/tools/libs/light/libxl_sched.c @@ -219,13 +219,11 @@ static int sched_credit_domain_set(libxl__gc *gc, uint32_t domid, xc_domaininfo_t domaininfo; int rc; - rc = xc_domain_getinfolist(CTX->xch, domid, 1, &domaininfo); + rc = xc_domain_getinfo_single(CTX->xch, domid, &domaininfo); if (rc < 0) { - LOGED(ERROR, domid, "Getting domain info list"); + LOGED(ERROR, domid, "Getting domain info"); return ERROR_FAIL; } - if (rc != 1 || domaininfo.domain != domid) - return ERROR_INVAL; rc = xc_sched_credit_domain_get(CTX->xch, domid, &sdom); if (rc != 0) { @@ -426,13 +424,11 @@ static int sched_credit2_domain_set(libxl__gc *gc, uint32_t domid, xc_domaininfo_t info; int rc; - rc = xc_domain_getinfolist(CTX->xch, domid, 1, &info); + rc = xc_domain_getinfo_single(CTX->xch, domid, &info); if (rc < 0) { LOGED(ERROR, domid, "Getting domain info"); return ERROR_FAIL; } - if (rc != 1 || info.domain != domid) - return ERROR_INVAL; rc = xc_sched_credit2_domain_get(CTX->xch, domid, &sdom); if (rc != 0) { diff --git a/tools/ocaml/libs/xc/xenctrl_stubs.c b/tools/ocaml/libs/xc/xenctrl_stubs.c index 6ec9ed6d1e..f686db3124 100644 --- a/tools/ocaml/libs/xc/xenctrl_stubs.c +++ b/tools/ocaml/libs/xc/xenctrl_stubs.c @@ -497,10 +497,8 @@ CAMLprim value stub_xc_domain_getinfo(value xch_val, value domid) xc_domaininfo_t info; int ret; - ret = xc_domain_getinfolist(xch, Int_val(domid), 1, &info); - if (ret != 1) - failwith_xc(xch); - if (info.domain != Int_val(domid)) + ret = xc_domain_getinfo_single(xch, Int_val(domid), &info); + if (ret < 0) failwith_xc(xch); result = alloc_domaininfo(&info); diff --git a/tools/xenpaging/xenpaging.c b/tools/xenpaging/xenpaging.c index 6e5490315d..c7a9a82477 100644 --- a/tools/xenpaging/xenpaging.c +++ b/tools/xenpaging/xenpaging.c @@ -169,8 +169,8 @@ static int xenpaging_get_tot_pages(struct xenpaging *paging) xc_domaininfo_t domain_info; int rc; - rc = xc_domain_getinfolist(xch, paging->vm_event.domain_id, 1, &domain_info); - if ( rc != 1 ) + rc = xc_domain_getinfo_single(xch, paging->vm_event.domain_id, &domain_info); + if ( rc < 0 ) { PERROR("Error getting domain info"); return -1; @@ -424,9 +424,9 @@ static struct xenpaging *xenpaging_init(int argc, char *argv[]) /* Get max_pages from guest if not provided via cmdline */ if ( !paging->max_pages ) { - rc = xc_domain_getinfolist(xch, paging->vm_event.domain_id, 1, - &domain_info); - if ( rc != 1 ) + rc = xc_domain_getinfo_single(xch, paging->vm_event.domain_id, + &domain_info); + if ( rc < 0 ) { PERROR("Error getting domain info"); goto err;
xc_domain_getinfolist() internally relies on a sysctl that performs a linear search for the domids. Many callers of xc_domain_getinfolist() who require information about a precise domid are much better off calling xc_domain_getinfo_single() instead, that will use the getdomaininfo domctl instead and ensure the returned domid matches the requested one. The domtctl will find the domid faster too, because that uses hashed lists. 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> Cc: Christian Lindig <christian.lindig@citrix.com> v3: * Replaced single-domid xc_domain_getinfolist() call in ocaml stub with xc_domain_getinfo_single() --- tools/libs/light/libxl_dom.c | 17 ++++++----------- tools/libs/light/libxl_dom_suspend.c | 7 +------ tools/libs/light/libxl_domain.c | 13 +++++-------- tools/libs/light/libxl_mem.c | 4 ++-- tools/libs/light/libxl_sched.c | 10 +++------- tools/ocaml/libs/xc/xenctrl_stubs.c | 6 ++---- tools/xenpaging/xenpaging.c | 10 +++++----- 7 files changed, 24 insertions(+), 43 deletions(-)