| Message ID | 20221101152842.4257-9-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | tools/xenstore: do some cleanup and fixes | expand |
Hi Juergen, On 01/11/2022 15:28, Juergen Gross wrote: > @@ -341,49 +339,56 @@ static bool get_domain_info(unsigned int domid, xc_dominfo_t *dominfo) > dominfo->domid == domid; > } > > -void check_domains(void) > +static int check_domain(void *k, void *v, void *arg) Looking at this callback, shouldn't 'k' be const? If not, wouldn't this mean a caller could potentially mess up with the hashtable? > { > xc_dominfo_t dominfo; > - struct domain *domain; > struct connection *conn; > - int notify = 0; > bool dom_valid; > + struct domain *domain = v; > + bool *notify = arg; > > - again: > - list_for_each_entry(domain, &domains, list) { > - dom_valid = get_domain_info(domain->domid, &dominfo); > - if (!domain->introduced) { > - if (!dom_valid) { > - talloc_free(domain); > - goto again; > - } > - continue; > - } > - if (dom_valid) { > - if ((dominfo.crashed || dominfo.shutdown) > - && !domain->shutdown) { > - domain->shutdown = true; > - notify = 1; > - /* > - * Avoid triggering watch events when the > - * domain's nodes are being deleted. > - */ > - if (domain->conn) > - conn_delete_all_watches(domain->conn); > - } > - if (!dominfo.dying) > - continue; > + dom_valid = get_domain_info(domain->domid, &dominfo); > + if (!domain->introduced) { > + if (!dom_valid) { > + talloc_free(domain); > + return 1; You are only freeing one domain here. So shouldn't we return 0? If not please explain in a comment. > } > - if (domain->conn) { > - /* domain is a talloc child of domain->conn. */ > - conn = domain->conn; > - domain->conn = NULL; > - talloc_unlink(talloc_autofree_context(), conn); > - notify = 0; /* destroy_domain() fires the watch */ > - goto again; > + return 0; > + } > + if (dom_valid) { > + if ((dominfo.crashed || dominfo.shutdown) > + && !domain->shutdown) { > + domain->shutdown = true; > + *notify = true; > + /* > + * Avoid triggering watch events when the domain's > + * nodes are being deleted. > + */ > + if (domain->conn) > + conn_delete_all_watches(domain->conn); > } > + if (!dominfo.dying) > + return 0; > + } > + if (domain->conn) { > + /* domain is a talloc child of domain->conn. */ > + conn = domain->conn; > + domain->conn = NULL; > + talloc_unlink(talloc_autofree_context(), conn); > + *notify = false; /* destroy_domain() fires the watch */ > + return 1; Can you add a comment explaining why 1 is returned here? Is this because we may free two domains? > } > > + return 0; > +} Cheers,
On 01.12.22 22:34, Julien Grall wrote: > Hi Juergen, > > On 01/11/2022 15:28, Juergen Gross wrote: >> @@ -341,49 +339,56 @@ static bool get_domain_info(unsigned int domid, >> xc_dominfo_t *dominfo) >> dominfo->domid == domid; >> } >> -void check_domains(void) >> +static int check_domain(void *k, void *v, void *arg) > > Looking at this callback, shouldn't 'k' be const? If not, wouldn't this mean a > caller could potentially mess up with the hashtable? I have modified the previous patch to make k const. I hope you are fine with me having kept your "Reviewed-by:". > >> { >> xc_dominfo_t dominfo; >> - struct domain *domain; >> struct connection *conn; >> - int notify = 0; >> bool dom_valid; >> + struct domain *domain = v; >> + bool *notify = arg; >> - again: >> - list_for_each_entry(domain, &domains, list) { >> - dom_valid = get_domain_info(domain->domid, &dominfo); >> - if (!domain->introduced) { >> - if (!dom_valid) { >> - talloc_free(domain); >> - goto again; >> - } >> - continue; >> - } >> - if (dom_valid) { >> - if ((dominfo.crashed || dominfo.shutdown) >> - && !domain->shutdown) { >> - domain->shutdown = true; >> - notify = 1; >> - /* >> - * Avoid triggering watch events when the >> - * domain's nodes are being deleted. >> - */ >> - if (domain->conn) >> - conn_delete_all_watches(domain->conn); >> - } >> - if (!dominfo.dying) >> - continue; >> + dom_valid = get_domain_info(domain->domid, &dominfo); >> + if (!domain->introduced) { >> + if (!dom_valid) { >> + talloc_free(domain); >> + return 1; > > You are only freeing one domain here. So shouldn't we return 0? If not please > explain in a comment. Oh, good catch. That was a leftover from a previous state where even removing the entry itself wasn't allowed. > >> } >> - if (domain->conn) { >> - /* domain is a talloc child of domain->conn. */ >> - conn = domain->conn; >> - domain->conn = NULL; >> - talloc_unlink(talloc_autofree_context(), conn); >> - notify = 0; /* destroy_domain() fires the watch */ >> - goto again; >> + return 0; >> + } >> + if (dom_valid) { >> + if ((dominfo.crashed || dominfo.shutdown) >> + && !domain->shutdown) { >> + domain->shutdown = true; >> + *notify = true; >> + /* >> + * Avoid triggering watch events when the domain's >> + * nodes are being deleted. >> + */ >> + if (domain->conn) >> + conn_delete_all_watches(domain->conn); >> } >> + if (!dominfo.dying) >> + return 0; >> + } >> + if (domain->conn) { >> + /* domain is a talloc child of domain->conn. */ >> + conn = domain->conn; >> + domain->conn = NULL; >> + talloc_unlink(talloc_autofree_context(), conn); >> + *notify = false; /* destroy_domain() fires the watch */ >> + return 1; > > Can you add a comment explaining why 1 is returned here? Is this because we may > free two domains? Should be the same case as above. In the initial version I just mapped the removed "goto again" to a restart of hashtable_iterate(). Juergen
Hi Juergen, On 12/12/2022 12:08, Juergen Gross wrote: > On 01.12.22 22:34, Julien Grall wrote: >> Hi Juergen, >> >> On 01/11/2022 15:28, Juergen Gross wrote: >>> @@ -341,49 +339,56 @@ static bool get_domain_info(unsigned int domid, >>> xc_dominfo_t *dominfo) >>> dominfo->domid == domid; >>> } >>> -void check_domains(void) >>> +static int check_domain(void *k, void *v, void *arg) >> >> Looking at this callback, shouldn't 'k' be const? If not, wouldn't >> this mean a caller could potentially mess up with the hashtable? > > I have modified the previous patch to make k const. I hope you are > fine with me having kept your "Reviewed-by:". Yes I am fine with that. Cheers,
On 01.12.22 22:34, Julien Grall wrote: > Hi Juergen, > > On 01/11/2022 15:28, Juergen Gross wrote: >> @@ -341,49 +339,56 @@ static bool get_domain_info(unsigned int domid, >> xc_dominfo_t *dominfo) >> dominfo->domid == domid; >> } >> -void check_domains(void) >> +static int check_domain(void *k, void *v, void *arg) > > Looking at this callback, shouldn't 'k' be const? If not, wouldn't this mean a > caller could potentially mess up with the hashtable? > >> { >> xc_dominfo_t dominfo; >> - struct domain *domain; >> struct connection *conn; >> - int notify = 0; >> bool dom_valid; >> + struct domain *domain = v; >> + bool *notify = arg; >> - again: >> - list_for_each_entry(domain, &domains, list) { >> - dom_valid = get_domain_info(domain->domid, &dominfo); >> - if (!domain->introduced) { >> - if (!dom_valid) { >> - talloc_free(domain); >> - goto again; >> - } >> - continue; >> - } >> - if (dom_valid) { >> - if ((dominfo.crashed || dominfo.shutdown) >> - && !domain->shutdown) { >> - domain->shutdown = true; >> - notify = 1; >> - /* >> - * Avoid triggering watch events when the >> - * domain's nodes are being deleted. >> - */ >> - if (domain->conn) >> - conn_delete_all_watches(domain->conn); >> - } >> - if (!dominfo.dying) >> - continue; >> + dom_valid = get_domain_info(domain->domid, &dominfo); >> + if (!domain->introduced) { >> + if (!dom_valid) { >> + talloc_free(domain); >> + return 1; > > You are only freeing one domain here. So shouldn't we return 0? If not please > explain in a comment. > >> } >> - if (domain->conn) { >> - /* domain is a talloc child of domain->conn. */ >> - conn = domain->conn; >> - domain->conn = NULL; >> - talloc_unlink(talloc_autofree_context(), conn); >> - notify = 0; /* destroy_domain() fires the watch */ >> - goto again; >> + return 0; >> + } >> + if (dom_valid) { >> + if ((dominfo.crashed || dominfo.shutdown) >> + && !domain->shutdown) { >> + domain->shutdown = true; >> + *notify = true; >> + /* >> + * Avoid triggering watch events when the domain's >> + * nodes are being deleted. >> + */ >> + if (domain->conn) >> + conn_delete_all_watches(domain->conn); >> } >> + if (!dominfo.dying) >> + return 0; >> + } >> + if (domain->conn) { >> + /* domain is a talloc child of domain->conn. */ >> + conn = domain->conn; >> + domain->conn = NULL; >> + talloc_unlink(talloc_autofree_context(), conn); >> + *notify = false; /* destroy_domain() fires the watch */ >> + return 1; > > Can you add a comment explaining why 1 is returned here? Is this because we may > free two domains? Oh, just detected you are right: this might free 2 domains, so a return value other than 0 is required. Will add a comment. Juergen
diff --git a/tools/xenstore/xenstored_domain.c b/tools/xenstore/xenstored_domain.c index 1516df71d8..f6797e53c5 100644 --- a/tools/xenstore/xenstored_domain.c +++ b/tools/xenstore/xenstored_domain.c @@ -48,8 +48,6 @@ static struct node_perms dom_introduce_perms; struct domain { - struct list_head list; - /* The id of this domain */ unsigned int domid; @@ -96,7 +94,7 @@ struct domain bool wrl_delay_logged; }; -static LIST_HEAD(domains); +static struct hashtable *domhash; static bool check_indexes(XENSTORE_RING_IDX cons, XENSTORE_RING_IDX prod) { @@ -309,7 +307,7 @@ static int destroy_domain(void *_domain) domain_tree_remove(domain); - list_del(&domain->list); + hashtable_remove(domhash, &domain->domid); if (!domain->introduced) return 0; @@ -341,49 +339,56 @@ static bool get_domain_info(unsigned int domid, xc_dominfo_t *dominfo) dominfo->domid == domid; } -void check_domains(void) +static int check_domain(void *k, void *v, void *arg) { xc_dominfo_t dominfo; - struct domain *domain; struct connection *conn; - int notify = 0; bool dom_valid; + struct domain *domain = v; + bool *notify = arg; - again: - list_for_each_entry(domain, &domains, list) { - dom_valid = get_domain_info(domain->domid, &dominfo); - if (!domain->introduced) { - if (!dom_valid) { - talloc_free(domain); - goto again; - } - continue; - } - if (dom_valid) { - if ((dominfo.crashed || dominfo.shutdown) - && !domain->shutdown) { - domain->shutdown = true; - notify = 1; - /* - * Avoid triggering watch events when the - * domain's nodes are being deleted. - */ - if (domain->conn) - conn_delete_all_watches(domain->conn); - } - if (!dominfo.dying) - continue; + dom_valid = get_domain_info(domain->domid, &dominfo); + if (!domain->introduced) { + if (!dom_valid) { + talloc_free(domain); + return 1; } - if (domain->conn) { - /* domain is a talloc child of domain->conn. */ - conn = domain->conn; - domain->conn = NULL; - talloc_unlink(talloc_autofree_context(), conn); - notify = 0; /* destroy_domain() fires the watch */ - goto again; + return 0; + } + if (dom_valid) { + if ((dominfo.crashed || dominfo.shutdown) + && !domain->shutdown) { + domain->shutdown = true; + *notify = true; + /* + * Avoid triggering watch events when the domain's + * nodes are being deleted. + */ + if (domain->conn) + conn_delete_all_watches(domain->conn); } + if (!dominfo.dying) + return 0; + } + if (domain->conn) { + /* domain is a talloc child of domain->conn. */ + conn = domain->conn; + domain->conn = NULL; + talloc_unlink(talloc_autofree_context(), conn); + *notify = false; /* destroy_domain() fires the watch */ + return 1; } + return 0; +} + +void check_domains(void) +{ + bool notify = false; + + while (hashtable_iterate(domhash, check_domain, ¬ify)) + ; + if (notify) fire_watches(NULL, NULL, "@releaseDomain", NULL, true, NULL); } @@ -421,13 +426,7 @@ static char *talloc_domain_path(const void *context, unsigned int domid) static struct domain *find_domain_struct(unsigned int domid) { - struct domain *i; - - list_for_each_entry(i, &domains, list) { - if (i->domid == domid) - return i; - } - return NULL; + return hashtable_search(domhash, &domid); } int domain_get_quota(const void *ctx, struct connection *conn, @@ -476,9 +475,13 @@ static struct domain *alloc_domain(const void *context, unsigned int domid) domain->generation = generation; domain->introduced = false; - talloc_set_destructor(domain, destroy_domain); + if (!hashtable_insert(domhash, &domain->domid, domain)) { + talloc_free(domain); + errno = ENOMEM; + return NULL; + } - list_add(&domain->list, &domains); + talloc_set_destructor(domain, destroy_domain); return domain; } @@ -909,10 +912,25 @@ void dom0_init(void) xenevtchn_notify(xce_handle, dom0->port); } +static unsigned int domhash_fn(void *k) +{ + return *(unsigned int *)k; +} + +static int domeq_fn(void *key1, void *key2) +{ + return *(unsigned int *)key1 == *(unsigned int *)key2; +} + void domain_init(int evtfd) { int rc; + /* Start with a random rather low domain count for the hashtable. */ + domhash = create_hashtable(8, domhash_fn, domeq_fn, 0); + if (!domhash) + barf_perror("Failed to allocate domain hashtable"); + xc_handle = talloc(talloc_autofree_context(), xc_interface*); if (!xc_handle) barf_perror("Failed to allocate domain handle");
Today finding a struct domain by its domain id requires to scan the list of domains until finding the correct domid. Add a hashlist for being able to speed this up. This allows to remove the linking of struct domain in a list. Note that the list of changed domains per transaction is kept as a list, as there are no known use cases with more than 4 domains being touched in a single transaction (this would be a device handled by a driver domain and being assigned to a HVM domain with device model in a stubdom, plus the control domain). Some simple performance tests comparing the scanning and hashlist have shown that the hashlist will win as soon as more than 6 entries need to be scanned. Signed-off-by: Juergen Gross <jgross@suse.com> --- tools/xenstore/xenstored_domain.c | 112 +++++++++++++++++------------- 1 file changed, 65 insertions(+), 47 deletions(-)