Re: [Xen-devel] [PATCH v4 5/7] libxl: support unmapping static shared memory areas during domain destruction

[Wei Liu <wei.liu2@xxxxxxxxxx>]

On Tue, Feb 06, 2018 at 01:24:30PM +0000, Julien Grall wrote:
> >       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 2cfe4c08a7..c398b6a6b8 100644
> > --- a/tools/libxl/libxl_internal.h
> > +++ b/tools/libxl/libxl_internal.h
> > @@ -4424,6 +4424,8 @@ static inline bool libxl__string_is_default(char **s)
> >   _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 562f46f299..1bf4d4c2dc 100644
> > --- a/tools/libxl/libxl_sshm.c
> > +++ b/tools/libxl/libxl_sshm.c
> > @@ -86,6 +86,112 @@ int libxl__sshm_check_overlap(libxl__gc *gc, uint32_t 
> > domid,
> >       return 0;
> >   }
> > +/* Decrease the refcount of an sshm. When refcount reaches 0,
> 
> NIT: Libxl coding style regarding the comment seems to be uncleared (Ian,
> Wei?). But I feel keep /* and */ in separate line is nicer.

I don't have an opinion here.

> 
> > + * clean up the whole sshm path.
> > + */
> > +static void libxl__sshm_decref(libxl__gc *gc, xs_transaction_t xt,
> > +                               const char *sshm_path)
> > +static void libxl__sshm_del_slave(libxl__gc *gc, xs_transaction_t xt,
> > +                                  uint32_t domid, const char *id, bool 
> > isretry)
> > +{
> > +    const char *slave_path, *begin_str, *end_str;
> > +    uint64_t begin, end;
> > +
> > +    slave_path = GCSPRINTF("%s/slaves/%"PRIu32, SSHM_PATH(id), domid);
> > +
> > +    begin_str = libxl__xs_read(gc, xt, GCSPRINTF("%s/begin", slave_path));
> > +    end_str = libxl__xs_read(gc, xt, GCSPRINTF("%s/end", slave_path));
> > +    begin = strtoull(begin_str, NULL, 16);
> > +    end = strtoull(end_str, NULL, 16);
> > +
> > +    /* Avoid calling do_unmap many times in case of xs transaction retry */
> > +    if (!isretry)
> > +        libxl__sshm_do_unmap(gc, domid, id, begin, end);
> 
> IHMO, by unmapping the regions in middle of the transaction, you increase
> the potential failure of it. I would move that out of the transaction path.
> 
> I would be interested to hear the opinion of the tools maintainers here.
> 

If you move the unmap after the loop you create a window in which
the pages are still mapped but the toolstack thinks they are unmapped.

While the code as-is now makes sure (assuming no error in unmap) the
pages are unmapped no later than the transaction is committed. I think
this can be done by moving unmap before the transaction.

Zhongze, do you think the unmap must be done inside the loop? What kind
of invariants do you have in mind?

Then there is the question of "what do we do if unmap fails". Honestly I
don't have an answer. It seems rather screwed up in that case and I
doubt there is much libxl can do to rectify things.

Wei.

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