Re: [Xen-devel] [PATCH v8 4/8] libxl: support unmapping static shared memory areas during domain destruction

[Ian Jackson <ian.jackson@xxxxxxxxxx>]

Stefano Stabellini writes ("[PATCH v8 4/8] libxl: support unmapping static 
shared memory areas during domain destruction"):
> Add libxl__sshm_del to unmap static shared memory areas mapped by
> libxl__sshm_add during domain creation. The unmapping process is:

This whole part should be in a comment in the code, I think.

> * For a master: decrease the refcount of the sshm region, if the refcount
>   reaches 0, cleanup the whole sshm path.

Does this not prevent actual domain destruction, if there are still
slaves ?

> * For a slave:
>   1) unmap the shared pages, and cleanup related xs entries. If the
>      system works normally, all the shared pages will be unmapped, so there
>      won't be page leaks. In case of errors, the unmapping process will go
>      on and unmap all the other pages that can be unmapped, so the other
>      pages won't be leaked, either.

When might pages be leaked ?

>   2) Decrease the refcount of the sshm region, if the refcount reaches
>      0, cleanup the whole sshm path.

> diff --git a/tools/libxl/libxl_domain.c b/tools/libxl/libxl_domain.c
> index 3377bba..3f7ffa6 100644
> --- a/tools/libxl/libxl_domain.c
> +++ b/tools/libxl/libxl_domain.c
> @@ -1060,6 +1060,11 @@ void libxl__destroy_domid(libxl__egc *egc, 
> libxl__destroy_domid_state *dis)
>          goto out;
>      }
>  
> +    rc = libxl__sshm_del(gc, domid);
> +    if (rc) {
> +        LOGD(ERROR, domid, "Deleting static shm failed.");

Please explain (preferably with a code comment) what a failure here
implies and how it is a legal and recoverable state.

> +    count_path = GCSPRINTF("%s/usercnt", sshm_path);
> +    if (libxl__xs_read_checked(gc, xt, count_path, &count_string))
> +        return;

Again, count_string may be 0, but I think you can probably do away
with this.

> +static void libxl__sshm_do_unmap(libxl__gc *gc, uint32_t domid, const char 
> *id,
> +                                 uint64_t begin, uint64_t size)
> +{
> +    uint64_t end;
> +    begin >>= XC_PAGE_SHIFT;
> +    size >>= XC_PAGE_SHIFT;
> +    end = begin + size;
> +    for (; begin < end; ++begin) {
> +        if (xc_domain_remove_from_physmap(CTX->xch, domid, begin)) {
> +            SSHM_ERROR(domid, id,
> +                       "unable to unmap shared page at 0x%"PRIx64".",
> +                       begin);
> +        }

Is this idempotent ?

> +    slave_path = GCSPRINTF("%s/slaves/%"PRIu32, SSHM_PATH(id), domid);
> +
> +    begin_str = libxl__xs_read(gc, xt, GCSPRINTF("%s/begin", slave_path));
> +    size_str = libxl__xs_read(gc, xt, GCSPRINTF("%s/size", slave_path));
> +    begin = strtoull(begin_str, NULL, 16);
> +    size = strtoull(size_str, NULL, 16);

Maybe this formulaic retrieval code could be combined with that for
adding a borrow.

> +        if (libxl__xs_read(gc, xt, dom_sshm_path)) {

Again, needs to be libxl__xs_read_checked.  But:

> +            sshm_ents = libxl__xs_directory(gc, xt, dom_sshm_path, 
> &sshm_num);
> +            if (!sshm_ents) continue;

I don't see why you can't just call libxl__xs_directory without
xs_read.  Also, you need to check errno, after libxl__xs_directory
fails.

> +            for (i = 0; i < sshm_num; ++i) {
> +                role = libxl__xs_read(gc, xt,
> +                                      GCSPRINTF("%s/%s/role",
> +                                                dom_sshm_path,
> +                                                sshm_ents[i]));
> +                assert(role);
> +                if (!strncmp(role, "slave", 5))

This should explicitly handle other values for `role', probably by
failing.  Also the error handling assert is poor.

> +                    libxl__sshm_del_slave(gc, xt, domid, sshm_ents[i], 
> isretry);
> +
> +                libxl__sshm_decref(gc, xt, SSHM_PATH(sshm_ents[i]));
> +            }
> +        }
> +
> +        rc = libxl__xs_transaction_commit(gc, &xt);
> +        if (!rc) break;
> +        if (rc < 0) goto out;
> +        isretry = true;

isretry seems to be a dead variable.

Thanks,
Ian.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel