Message ID | 20210917154625.89315-5-roger.pau@citrix.com (mailing list archive) |
---|---|
State | Superseded |
Headers | show |
Series | gnttab: add per-domain controls | expand |
On Fri, Sep 17, 2021 at 05:46:23PM +0200, Roger Pau Monne wrote: > Exploiting the talloc clean up routines to close the Xen interfaces > is cumbersome, specially when atexit can be used to the same effect. > > Convert xc and gnttab to use atexit which allows to drop one > indirection from the storing variables. > > No functional change intended. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> This patch is missing the following chunk: diff --git a/tools/xenstore/xenstored_minios.c b/tools/xenstore/xenstored_minios.c index c94493e52a..9b050c7e02 100644 --- a/tools/xenstore/xenstored_minios.c +++ b/tools/xenstore/xenstored_minios.c @@ -49,12 +49,12 @@ evtchn_port_t xenbus_evtchn(void) void *xenbus_map(void) { - return xengnttab_map_grant_ref(*xgt_handle, xenbus_master_domid(), + return xengnttab_map_grant_ref(xgt_handle, xenbus_master_domid(), GNTTAB_RESERVED_XENSTORE, PROT_READ|PROT_WRITE); } void unmap_xenbus(void *interface) { - xengnttab_unmap(*xgt_handle, interface, 1); + xengnttab_unmap(xgt_handle, interface, 1); }
On 17.09.21 17:46, Roger Pau Monne wrote: > Exploiting the talloc clean up routines to close the Xen interfaces > is cumbersome, specially when atexit can be used to the same effect. > > Convert xc and gnttab to use atexit which allows to drop one > indirection from the storing variables. > > No functional change intended. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > tools/xenstore/xenstored_core.h | 2 +- > tools/xenstore/xenstored_domain.c | 57 +++++++++++++------------------ > 2 files changed, 25 insertions(+), 34 deletions(-) > > diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c > index 47e9107c14..8930303773 100644 > --- a/tools/xenstore/xenstored_domain.c > +++ b/tools/xenstore/xenstored_domain.c > @@ -737,35 +725,38 @@ void dom0_init(void) > xenevtchn_notify(xce_handle, dom0->port); > } > > +void interface_cleanup(void) > +{ > + if (xc_handle) { > + xc_interface_close(xc_handle); > + xc_handle = NULL; > + } > + if (xgt_handle) { > + xengnttab_close(xgt_handle); > + xgt_handle = NULL; > + } Could you please add closing of xce_handle(), too? Juergen
Roger Pau Monne writes ("[PATCH 4/6] tools/xenstored: use atexit to close interfaces"): > Exploiting the talloc clean up routines to close the Xen interfaces > is cumbersome, specially when atexit can be used to the same effect. Wait, what ? Why do we need to do this at all ? xenstored can't be restarted so if it exits we might as well leave whatever wreckage. AIUI the live update doesn't end up calling atexit ? Ian.
On 20.09.21 12:34, Ian Jackson wrote: > Roger Pau Monne writes ("[PATCH 4/6] tools/xenstored: use atexit to close interfaces"): >> Exploiting the talloc clean up routines to close the Xen interfaces >> is cumbersome, specially when atexit can be used to the same effect. > > Wait, what ? Why do we need to do this at all ? xenstored can't be > restarted so if it exits we might as well leave whatever wreckage. In practice, yes. But having those cleanups might make it easier to find memory leaks. Why do we have cleanups in normal Xen tools before exiting? The system is doing cleanup anyway... ;-) > AIUI the live update doesn't end up calling atexit ? That's correct. Juergen
On Mon, Sep 20, 2021 at 11:22:15AM +0200, Juergen Gross wrote: > On 17.09.21 17:46, Roger Pau Monne wrote: > > Exploiting the talloc clean up routines to close the Xen interfaces > > is cumbersome, specially when atexit can be used to the same effect. > > > > Convert xc and gnttab to use atexit which allows to drop one > > indirection from the storing variables. > > > > No functional change intended. > > > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > > --- > > tools/xenstore/xenstored_core.h | 2 +- > > tools/xenstore/xenstored_domain.c | 57 +++++++++++++------------------ > > 2 files changed, 25 insertions(+), 34 deletions(-) > > > > diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c > > index 47e9107c14..8930303773 100644 > > --- a/tools/xenstore/xenstored_domain.c > > +++ b/tools/xenstore/xenstored_domain.c > > @@ -737,35 +725,38 @@ void dom0_init(void) > > xenevtchn_notify(xce_handle, dom0->port); > > } > > +void interface_cleanup(void) > > +{ > > + if (xc_handle) { > > + xc_interface_close(xc_handle); > > + xc_handle = NULL; > > + } > > + if (xgt_handle) { > > + xengnttab_close(xgt_handle); > > + xgt_handle = NULL; > > + } > > Could you please add closing of xce_handle(), too? Sure, I somehow assumed there was a reason for not closing it related to live update, but I see that's not the case as you use exec to launch the new image and atexit handlers are not called in that case. Thanks, Roger.
Roger Pau Monné writes ("Re: [PATCH 4/6] tools/xenstored: use atexit to close interfaces"): > On Mon, Sep 20, 2021 at 11:22:15AM +0200, Juergen Gross wrote: > > Could you please add closing of xce_handle(), too? > > Sure, I somehow assumed there was a reason for not closing it related > to live update, but I see that's not the case as you use exec to > launch the new image and atexit handlers are not called in that case. Are these fds marked CLOEXEC ? I don't think they are BICBW. It would probably be good idea to make them so. An fd leak might result in one fd being consumed per live update operation. Ian.
On 20.09.21 12:57, Ian Jackson wrote: > Roger Pau Monné writes ("Re: [PATCH 4/6] tools/xenstored: use atexit to close interfaces"): >> On Mon, Sep 20, 2021 at 11:22:15AM +0200, Juergen Gross wrote: >>> Could you please add closing of xce_handle(), too? >> >> Sure, I somehow assumed there was a reason for not closing it related >> to live update, but I see that's not the case as you use exec to >> launch the new image and atexit handlers are not called in that case. > > Are these fds marked CLOEXEC ? I don't think they are BICBW. They are. I checked when implementing LU and just rechecked. The event-channel fd is opened explicitly without CLOEXEC in order to support LU (the new xenstored won't open it again, but gets its fd via the migration stream). Juergen
Juergen Gross writes ("Re: [PATCH 4/6] tools/xenstored: use atexit to close interfaces"): > They are. I checked when implementing LU and just rechecked. > > The event-channel fd is opened explicitly without CLOEXEC in order > to support LU (the new xenstored won't open it again, but gets its > fd via the migration stream). Oh, excellent. Thanks. Ian.
diff --git a/tools/xenstore/xenstored_core.h b/tools/xenstore/xenstored_core.h index 258f6ff382..a813bc5a8c 100644 --- a/tools/xenstore/xenstored_core.h +++ b/tools/xenstore/xenstored_core.h @@ -266,7 +266,7 @@ void init_pipe(int reopen_log_pipe[2]); #ifndef NO_SOCKETS extern const struct interface_funcs socket_funcs; #endif -extern xengnttab_handle **xgt_handle; +extern xengnttab_handle *xgt_handle; int remember_string(struct hashtable *hash, const char *str); diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c index 47e9107c14..8930303773 100644 --- a/tools/xenstore/xenstored_domain.c +++ b/tools/xenstore/xenstored_domain.c @@ -35,8 +35,8 @@ #include <xenctrl.h> #include <xen/grant_table.h> -static xc_interface **xc_handle; -xengnttab_handle **xgt_handle; +static xc_interface *xc_handle; +xengnttab_handle *xgt_handle; static evtchn_port_t virq_port; xenevtchn_handle *xce_handle = NULL; @@ -198,14 +198,14 @@ static const struct interface_funcs domain_funcs = { static void *map_interface(domid_t domid) { - return xengnttab_map_grant_ref(*xgt_handle, domid, + return xengnttab_map_grant_ref(xgt_handle, domid, GNTTAB_RESERVED_XENSTORE, PROT_READ|PROT_WRITE); } static void unmap_interface(void *interface) { - xengnttab_unmap(*xgt_handle, interface, 1); + xengnttab_unmap(xgt_handle, interface, 1); } static int destroy_domain(void *_domain) @@ -240,7 +240,7 @@ static int destroy_domain(void *_domain) static bool get_domain_info(unsigned int domid, xc_dominfo_t *dominfo) { - return xc_domain_getinfo(*xc_handle, domid, 1, dominfo) == 1 && + return xc_domain_getinfo(xc_handle, domid, 1, dominfo) == 1 && dominfo->domid == domid; } @@ -648,18 +648,6 @@ int do_reset_watches(struct connection *conn, struct buffered_data *in) return 0; } -static int close_xc_handle(void *_handle) -{ - xc_interface_close(*(xc_interface**)_handle); - return 0; -} - -static int close_xgt_handle(void *_handle) -{ - xengnttab_close(*(xengnttab_handle **)_handle); - return 0; -} - /* Returns the implicit path of a connection (only domains have this) */ const char *get_implicit_path(const struct connection *conn) { @@ -737,35 +725,38 @@ void dom0_init(void) xenevtchn_notify(xce_handle, dom0->port); } +void interface_cleanup(void) +{ + if (xc_handle) { + xc_interface_close(xc_handle); + xc_handle = NULL; + } + if (xgt_handle) { + xengnttab_close(xgt_handle); + xgt_handle = NULL; + } +} + void domain_init(int evtfd) { - int rc; + int rc = atexit(interface_cleanup); - xc_handle = talloc(talloc_autofree_context(), xc_interface*); - if (!xc_handle) - barf_perror("Failed to allocate domain handle"); + if (rc) + barf_perror("Unable to register cleanup handler"); - *xc_handle = xc_interface_open(0,0,0); - if (!*xc_handle) + xc_handle = xc_interface_open(0,0,0); + if (!xc_handle) barf_perror("Failed to open connection to hypervisor"); - talloc_set_destructor(xc_handle, close_xc_handle); - - xgt_handle = talloc(talloc_autofree_context(), xengnttab_handle*); + xgt_handle = xengnttab_open(NULL, 0); if (!xgt_handle) - barf_perror("Failed to allocate domain gnttab handle"); - - *xgt_handle = xengnttab_open(NULL, 0); - if (*xgt_handle == NULL) barf_perror("Failed to open connection to gnttab"); /* * Allow max number of domains for mappings. We allow one grant per * domain so the theoretical maximum is DOMID_FIRST_RESERVED. */ - xengnttab_set_max_grants(*xgt_handle, DOMID_FIRST_RESERVED); - - talloc_set_destructor(xgt_handle, close_xgt_handle); + xengnttab_set_max_grants(xgt_handle, DOMID_FIRST_RESERVED); if (evtfd < 0) xce_handle = xenevtchn_open(NULL, XENEVTCHN_NO_CLOEXEC);
Exploiting the talloc clean up routines to close the Xen interfaces is cumbersome, specially when atexit can be used to the same effect. Convert xc and gnttab to use atexit which allows to drop one indirection from the storing variables. No functional change intended. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- tools/xenstore/xenstored_core.h | 2 +- tools/xenstore/xenstored_domain.c | 57 +++++++++++++------------------ 2 files changed, 25 insertions(+), 34 deletions(-)