Re: [Xen-devel] [PATCH v8 4/8] libxl: support unmapping static shared memory areas during domain destruction
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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [PATCH v8 4/8] libxl: support unmapping static shared memory areas during domain destruction
On Wed, Oct 24, 2018 at 2:49 PM Oleksandr Andrushchenko
wrote:
>
> On 10/09/2018 02:37 AM, Stefano Stabellini wrote:
> > From: Zhongze Liu
> >
> > Author: Zhongze Liu
> >
> > Add libxl__sshm_del to unmap static shared memory areas mapped by
> > libxl__sshm_add during domain creation. The unmapping process is:
> >
> > * For a master: decrease the refcount of the sshm region, if the refcount
> >reaches 0, cleanup the whole sshm path.
> >
> > * 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.
> >2) Decrease the refcount of the sshm region, if the refcount reaches
> > 0, cleanup the whole sshm path.
> >
> > This is for the proposal "Allow setting up shared memory areas between VMs
> > from xl config file" (see [1]).
> >
> > [1] https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html
> >
> > Signed-off-by: Zhongze Liu
> > Signed-off-by: Stefano Stabellini
> >
> > Cc: Ian Jackson
> > Cc: Wei Liu
> > Cc: Stefano Stabellini
> > Cc: Julien Grall
> > Cc: xen-de...@lists.xen.org
> >
> > ---
> > Changes in v5:
> > - fix typos
> > - add comments
> > - cannot move unmap before xenstore transaction because it needs to
> >retrieve begin/size values from xenstore
> > ---
> > tools/libxl/libxl_domain.c | 5 ++
> > tools/libxl/libxl_internal.h | 2 +
> > tools/libxl/libxl_sshm.c | 107
> > +++
> > 3 files changed, 114 insertions(+)
> >
> > 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.");
> > +}
> Do you need brackets here?
> > +
> > if (libxl__device_pci_destroy_all(gc, domid) < 0)
> > LOGD(ERROR, domid, "Pci shutdown failed");
> > rc = xc_domain_pause(ctx->xch, domid);
> > diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> > index 6f31a3d..e86d356 100644
> > --- a/tools/libxl/libxl_internal.h
> > +++ b/tools/libxl/libxl_internal.h
> > @@ -4442,6 +4442,8 @@ static inline const char
> > *libxl__qemu_qmp_path(libxl__gc *gc, int domid)
> > _hidden int libxl__sshm_add(libxl__gc *gc, uint32_t domid,
> > libxl_static_shm *sshm, int len);
> >
> > +_hidden int libxl__sshm_del(libxl__gc *gc, uint32_t domid);
> > +
> > _hidden int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
> > libxl_static_shm *sshms, int len);
> > _hidden int libxl__sshm_setdefault(libxl__gc *gc, uint32_t domid,
> > diff --git a/tools/libxl/libxl_sshm.c b/tools/libxl/libxl_sshm.c
> > index f61b80c..9672056 100644
> > --- a/tools/libxl/libxl_sshm.c
> > +++ b/tools/libxl/libxl_sshm.c
> > @@ -94,6 +94,113 @@ int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t
> > domid,
> > return 0;
> > }
> >
> > +/*
> > + * Decrease the refcount of an sshm. When refcount reaches 0,
> > + * clean up the whole sshm path.
> > + * Xenstore operations are done within the same transaction.
> > + */
> > +static void libxl__sshm_decref(libxl__gc *gc, xs_transaction_t xt,
> > + const char *sshm_path)
> > +{
> > +int count;
> > +const char *count_path, *count_string;
> > +
> > +count_path = GCSPRINTF("%s/usercnt", sshm_path);
> > +if (libxl__xs_read_checked(gc, xt, count_path, &count_string))
> > +return;
> > +count = atoi(count_string);
> > +
> > +if (--count == 0) {
> > +libxl__xs_path_cleanup(gc, xt, sshm_path);
> > +return;
> > +}
> > +
> > +count_string = GCSPRINTF("%d", count);
> > +libxl__xs_write_checked(gc, xt, count_path, count_string);
> > +
> > +return;
> > +}
> > +
> > +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)) {
What I was thinking is that xc_domain_remove_from_physmap_batch()
could speed up unmapping if was present)
> > +SSHM_ERROR(domid, id,
> > + "unable to unmap shared page at 0x%"PRIx64".",
> > + begin);
> > +}
> > +}
> > +}
>
Re: [Xen-devel] [PATCH v8 4/8] libxl: support unmapping static shared memory areas during domain destruction
On 10/09/2018 02:37 AM, Stefano Stabellini wrote:
From: Zhongze Liu
Author: Zhongze Liu
Add libxl__sshm_del to unmap static shared memory areas mapped by
libxl__sshm_add during domain creation. The unmapping process is:
* For a master: decrease the refcount of the sshm region, if the refcount
reaches 0, cleanup the whole sshm path.
* 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.
2) Decrease the refcount of the sshm region, if the refcount reaches
0, cleanup the whole sshm path.
This is for the proposal "Allow setting up shared memory areas between VMs
from xl config file" (see [1]).
[1] https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html
Signed-off-by: Zhongze Liu
Signed-off-by: Stefano Stabellini
Cc: Ian Jackson
Cc: Wei Liu
Cc: Stefano Stabellini
Cc: Julien Grall
Cc: xen-de...@lists.xen.org
---
Changes in v5:
- fix typos
- add comments
- cannot move unmap before xenstore transaction because it needs to
retrieve begin/size values from xenstore
---
tools/libxl/libxl_domain.c | 5 ++
tools/libxl/libxl_internal.h | 2 +
tools/libxl/libxl_sshm.c | 107 +++
3 files changed, 114 insertions(+)
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.");
+}
Do you need brackets here?
+
if (libxl__device_pci_destroy_all(gc, domid) < 0)
LOGD(ERROR, domid, "Pci shutdown failed");
rc = xc_domain_pause(ctx->xch, domid);
diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
index 6f31a3d..e86d356 100644
--- a/tools/libxl/libxl_internal.h
+++ b/tools/libxl/libxl_internal.h
@@ -4442,6 +4442,8 @@ static inline const char *libxl__qemu_qmp_path(libxl__gc
*gc, int domid)
_hidden int libxl__sshm_add(libxl__gc *gc, uint32_t domid,
libxl_static_shm *sshm, int len);
+_hidden int libxl__sshm_del(libxl__gc *gc, uint32_t domid);
+
_hidden int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
libxl_static_shm *sshms, int len);
_hidden int libxl__sshm_setdefault(libxl__gc *gc, uint32_t domid,
diff --git a/tools/libxl/libxl_sshm.c b/tools/libxl/libxl_sshm.c
index f61b80c..9672056 100644
--- a/tools/libxl/libxl_sshm.c
+++ b/tools/libxl/libxl_sshm.c
@@ -94,6 +94,113 @@ int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
return 0;
}
+/*
+ * Decrease the refcount of an sshm. When refcount reaches 0,
+ * clean up the whole sshm path.
+ * Xenstore operations are done within the same transaction.
+ */
+static void libxl__sshm_decref(libxl__gc *gc, xs_transaction_t xt,
+ const char *sshm_path)
+{
+int count;
+const char *count_path, *count_string;
+
+count_path = GCSPRINTF("%s/usercnt", sshm_path);
+if (libxl__xs_read_checked(gc, xt, count_path, &count_string))
+return;
+count = atoi(count_string);
+
+if (--count == 0) {
+libxl__xs_path_cleanup(gc, xt, sshm_path);
+return;
+}
+
+count_string = GCSPRINTF("%d", count);
+libxl__xs_write_checked(gc, xt, count_path, count_string);
+
+return;
+}
+
+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);
+}
+}
+}
+
+static void libxl__sshm_del_slave(libxl__gc *gc, xs_transaction_t xt,
+ uint32_t domid, const char *id, bool isretry)
isretry is not used, please remove
+{
+const char *slave_path, *begin_str, *size_str;
+uint64_t begin, size;
+
+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);
+
+libxl__sshm_do_unmap(gc, domid, id, begin, size);
+libxl__xs_path_cleanup(gc
Re: [Xen-devel] [PATCH v8 4/8] libxl: support unmapping static shared memory areas during domain destruction
Hi, Stefano
On Tue, Oct 9, 2018 at 2:39 AM Stefano Stabellini
wrote:
>
> From: Zhongze Liu
>
> Author: Zhongze Liu
>
> Add libxl__sshm_del to unmap static shared memory areas mapped by
> libxl__sshm_add during domain creation. The unmapping process is:
>
> * For a master: decrease the refcount of the sshm region, if the refcount
> reaches 0, cleanup the whole sshm path.
>
> * 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.
> 2) Decrease the refcount of the sshm region, if the refcount reaches
> 0, cleanup the whole sshm path.
>
> This is for the proposal "Allow setting up shared memory areas between VMs
> from xl config file" (see [1]).
>
> [1] https://lists.xen.org/archives/html/xen-devel/2017-08/msg03242.html
>
> Signed-off-by: Zhongze Liu
> Signed-off-by: Stefano Stabellini
>
> Cc: Ian Jackson
> Cc: Wei Liu
> Cc: Stefano Stabellini
> Cc: Julien Grall
> Cc: xen-de...@lists.xen.org
>
> ---
> Changes in v5:
> - fix typos
> - add comments
> - cannot move unmap before xenstore transaction because it needs to
> retrieve begin/size values from xenstore
> ---
> tools/libxl/libxl_domain.c | 5 ++
> tools/libxl/libxl_internal.h | 2 +
> tools/libxl/libxl_sshm.c | 107
> +++
> 3 files changed, 114 insertions(+)
>
> 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.");
> +}
> +
> if (libxl__device_pci_destroy_all(gc, domid) < 0)
> LOGD(ERROR, domid, "Pci shutdown failed");
> rc = xc_domain_pause(ctx->xch, domid);
> diff --git a/tools/libxl/libxl_internal.h b/tools/libxl/libxl_internal.h
> index 6f31a3d..e86d356 100644
> --- a/tools/libxl/libxl_internal.h
> +++ b/tools/libxl/libxl_internal.h
> @@ -4442,6 +4442,8 @@ static inline const char
> *libxl__qemu_qmp_path(libxl__gc *gc, int domid)
> _hidden int libxl__sshm_add(libxl__gc *gc, uint32_t domid,
> libxl_static_shm *sshm, int len);
>
> +_hidden int libxl__sshm_del(libxl__gc *gc, uint32_t domid);
> +
> _hidden int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t domid,
>libxl_static_shm *sshms, int len);
> _hidden int libxl__sshm_setdefault(libxl__gc *gc, uint32_t domid,
> diff --git a/tools/libxl/libxl_sshm.c b/tools/libxl/libxl_sshm.c
> index f61b80c..9672056 100644
> --- a/tools/libxl/libxl_sshm.c
> +++ b/tools/libxl/libxl_sshm.c
> @@ -94,6 +94,113 @@ int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t
> domid,
> return 0;
> }
>
> +/*
> + * Decrease the refcount of an sshm. When refcount reaches 0,
> + * clean up the whole sshm path.
> + * Xenstore operations are done within the same transaction.
> + */
> +static void libxl__sshm_decref(libxl__gc *gc, xs_transaction_t xt,
> + const char *sshm_path)
> +{
> +int count;
> +const char *count_path, *count_string;
> +
> +count_path = GCSPRINTF("%s/usercnt", sshm_path);
> +if (libxl__xs_read_checked(gc, xt, count_path, &count_string))
> +return;
> +count = atoi(count_string);
> +
> +if (--count == 0) {
> +libxl__xs_path_cleanup(gc, xt, sshm_path);
> +return;
> +}
> +
> +count_string = GCSPRINTF("%d", count);
> +libxl__xs_write_checked(gc, xt, count_path, count_string);
> +
> +return;
> +}
> +
> +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);
> +}
> +}
> +}
> +
> +static void libxl__sshm_del_slave(libxl__gc *gc, xs_transaction_t xt,
> + uint32_t domid, const char *id, bool
> isretry)
What is the purpose of passing "isretry" here? I don't see it being
used unlike for libxl__sshm_add_slave().
If this flag is not needed it can also be removed from libxl__sshm_del().
> +{
> +const char *slave_path, *begin_str, *size_str;
> +uint64_t begin, size;
> +
> +slave_path = GCSPRI
