xen/common/domain.c | 80 +++++++++++++++++++++++++++++++-------------- 1 file changed, 55 insertions(+), 25 deletions(-)
These are opencoded linked list and hashtable manipulations. Factor them out
into static inline helpers, and discuss the consequence for the domain.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
Calling domain_hash a hash is rather disingenuous... It's technically true,
but it has terrible scaling.
---
xen/common/domain.c | 80 +++++++++++++++++++++++++++++++--------------
1 file changed, 55 insertions(+), 25 deletions(-)
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 256660473861..fc0a56673449 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -64,6 +64,57 @@ DEFINE_RCU_READ_LOCK(domlist_read_lock);
static struct domain *domain_hash[DOMAIN_HASH_SIZE];
struct domain *domain_list;
+/*
+ * Insert a domain into the domlist/hash. This allows the domain to be looked
+ * up by domid, and therefore to be the subject of hypercalls/etc.
+ */
+static void domlist_insert(struct domain *d)
+{
+ struct domain **pd;
+
+ spin_lock(&domlist_update_lock);
+
+ /* domain_list is maintained in domid order. */
+ pd = &domain_list;
+ for ( ; *pd != NULL; pd = &(*pd)->next_in_list )
+ if ( (*pd)->domain_id > d->domain_id )
+ break;
+
+ d->next_in_list = *pd;
+ d->next_in_hashbucket = domain_hash[DOMAIN_HASH(d->domain_id)];
+ rcu_assign_pointer(*pd, d);
+ rcu_assign_pointer(domain_hash[DOMAIN_HASH(d->domain_id)], d);
+
+ spin_unlock(&domlist_update_lock);
+}
+
+/*
+ * Remove a domain from the domlist/hash. This means the domain can no longer
+ * be looked up by domid, and therefore can no longer be the subject of
+ * *subsequent* hypercalls/etc. In-progress hypercalls/etc can still operate
+ * on the domain.
+ */
+static void domlist_remove(struct domain *d)
+{
+ struct domain **pd = &domain_list;
+
+ spin_lock(&domlist_update_lock);
+
+ pd = &domain_list;
+ while ( *pd != d )
+ pd = &(*pd)->next_in_list;
+
+ rcu_assign_pointer(*pd, d->next_in_list);
+
+ pd = &domain_hash[DOMAIN_HASH(d->domain_id)];
+ while ( *pd != d )
+ pd = &(*pd)->next_in_hashbucket;
+
+ rcu_assign_pointer(*pd, d->next_in_hashbucket);
+
+ spin_unlock(&domlist_update_lock);
+}
+
struct domain *hardware_domain __read_mostly;
#ifdef CONFIG_LATE_HWDOM
@@ -589,7 +640,7 @@ struct domain *domain_create(domid_t domid,
struct xen_domctl_createdomain *config,
unsigned int flags)
{
- struct domain *d, **pd, *old_hwdom = NULL;
+ struct domain *d, *old_hwdom = NULL;
enum { INIT_watchdog = 1u<<1,
INIT_evtchn = 1u<<3, INIT_gnttab = 1u<<4, INIT_arch = 1u<<5 };
int err, init_status = 0;
@@ -758,17 +809,7 @@ struct domain *domain_create(domid_t domid,
* Must not fail beyond this point, as our caller doesn't know whether
* the domain has been entered into domain_list or not.
*/
-
- spin_lock(&domlist_update_lock);
- pd = &domain_list; /* NB. domain_list maintained in order of domid. */
- for ( pd = &domain_list; *pd != NULL; pd = &(*pd)->next_in_list )
- if ( (*pd)->domain_id > d->domain_id )
- break;
- d->next_in_list = *pd;
- d->next_in_hashbucket = domain_hash[DOMAIN_HASH(domid)];
- rcu_assign_pointer(*pd, d);
- rcu_assign_pointer(domain_hash[DOMAIN_HASH(domid)], d);
- spin_unlock(&domlist_update_lock);
+ domlist_insert(d);
memcpy(d->handle, config->handle, sizeof(d->handle));
@@ -1232,8 +1273,6 @@ static void cf_check complete_domain_destroy(struct rcu_head *head)
/* Release resources belonging to task @p. */
void domain_destroy(struct domain *d)
{
- struct domain **pd;
-
BUG_ON(!d->is_dying);
/* May be already destroyed, or get_domain() can race us. */
@@ -1242,17 +1281,8 @@ void domain_destroy(struct domain *d)
TRACE_TIME(TRC_DOM0_DOM_REM, d->domain_id);
- /* Delete from task list and task hashtable. */
- spin_lock(&domlist_update_lock);
- pd = &domain_list;
- while ( *pd != d )
- pd = &(*pd)->next_in_list;
- rcu_assign_pointer(*pd, d->next_in_list);
- pd = &domain_hash[DOMAIN_HASH(d->domain_id)];
- while ( *pd != d )
- pd = &(*pd)->next_in_hashbucket;
- rcu_assign_pointer(*pd, d->next_in_hashbucket);
- spin_unlock(&domlist_update_lock);
+ /* Remove from the domlist/hash. */
+ domlist_remove(d);
/* Schedule RCU asynchronous completion of domain destroy. */
call_rcu(&d->rcu, complete_domain_destroy);
base-commit: 561cba38ff551383a628dc93e64ab0691cfc92bf
--
2.39.2
On 31.07.2024 14:04, Andrew Cooper wrote: > --- a/xen/common/domain.c > +++ b/xen/common/domain.c > @@ -64,6 +64,57 @@ DEFINE_RCU_READ_LOCK(domlist_read_lock); > static struct domain *domain_hash[DOMAIN_HASH_SIZE]; > struct domain *domain_list; > > +/* > + * Insert a domain into the domlist/hash. This allows the domain to be looked > + * up by domid, and therefore to be the subject of hypercalls/etc. > + */ > +static void domlist_insert(struct domain *d) > +{ > + struct domain **pd; > + > + spin_lock(&domlist_update_lock); > + > + /* domain_list is maintained in domid order. */ > + pd = &domain_list; Make this the initializer of the variable, if ... > + for ( ; *pd != NULL; pd = &(*pd)->next_in_list ) ... isn't to be the starting clause of the for()? > + if ( (*pd)->domain_id > d->domain_id ) > + break; > + > + d->next_in_list = *pd; > + d->next_in_hashbucket = domain_hash[DOMAIN_HASH(d->domain_id)]; > + rcu_assign_pointer(*pd, d); > + rcu_assign_pointer(domain_hash[DOMAIN_HASH(d->domain_id)], d); Maybe worth putting the hash in a local variable? > + spin_unlock(&domlist_update_lock); > +} > + > +/* > + * Remove a domain from the domlist/hash. This means the domain can no longer > + * be looked up by domid, and therefore can no longer be the subject of > + * *subsequent* hypercalls/etc. In-progress hypercalls/etc can still operate > + * on the domain. > + */ > +static void domlist_remove(struct domain *d) > +{ > + struct domain **pd = &domain_list; > + > + spin_lock(&domlist_update_lock); > + > + pd = &domain_list; pd already has an initializer. With at least the pd related adjustments Reviewed-by: Jan Beulich <jbeulich@suse.com> Jan
On 31/07/2024 1:14 pm, Jan Beulich wrote: > On 31.07.2024 14:04, Andrew Cooper wrote: >> --- a/xen/common/domain.c >> +++ b/xen/common/domain.c >> @@ -64,6 +64,57 @@ DEFINE_RCU_READ_LOCK(domlist_read_lock); >> static struct domain *domain_hash[DOMAIN_HASH_SIZE]; >> struct domain *domain_list; >> >> +/* >> + * Insert a domain into the domlist/hash. This allows the domain to be looked >> + * up by domid, and therefore to be the subject of hypercalls/etc. >> + */ >> +static void domlist_insert(struct domain *d) >> +{ >> + struct domain **pd; >> + >> + spin_lock(&domlist_update_lock); >> + >> + /* domain_list is maintained in domid order. */ >> + pd = &domain_list; > Make this the initializer of the variable, if ... > >> + for ( ; *pd != NULL; pd = &(*pd)->next_in_list ) > ... isn't to be the starting clause of the for()? Ok. > >> + if ( (*pd)->domain_id > d->domain_id ) >> + break; >> + >> + d->next_in_list = *pd; >> + d->next_in_hashbucket = domain_hash[DOMAIN_HASH(d->domain_id)]; >> + rcu_assign_pointer(*pd, d); >> + rcu_assign_pointer(domain_hash[DOMAIN_HASH(d->domain_id)], d); > Maybe worth putting the hash in a local variable? Ok. > >> + spin_unlock(&domlist_update_lock); >> +} >> + >> +/* >> + * Remove a domain from the domlist/hash. This means the domain can no longer >> + * be looked up by domid, and therefore can no longer be the subject of >> + * *subsequent* hypercalls/etc. In-progress hypercalls/etc can still operate >> + * on the domain. >> + */ >> +static void domlist_remove(struct domain *d) >> +{ >> + struct domain **pd = &domain_list; >> + >> + spin_lock(&domlist_update_lock); >> + >> + pd = &domain_list; > pd already has an initializer. Ah - that was a copy&paste error of mine. I'll drop the initialiser. The code I copied from strictly initialises *pd with the update lock held. As we're only taking the address of pointer, I think it's safe to be outside, but its also just an LEA so also not interesting to initialise outside. > With at least the pd related adjustments > Reviewed-by: Jan Beulich <jbeulich@suse.com> Thanks. ~Andrew
On 31.07.2024 16:07, Andrew Cooper wrote: > On 31/07/2024 1:14 pm, Jan Beulich wrote: >> On 31.07.2024 14:04, Andrew Cooper wrote: >>> --- a/xen/common/domain.c >>> +++ b/xen/common/domain.c >>> @@ -64,6 +64,57 @@ DEFINE_RCU_READ_LOCK(domlist_read_lock); >>> static struct domain *domain_hash[DOMAIN_HASH_SIZE]; >>> struct domain *domain_list; >>> >>> +/* >>> + * Insert a domain into the domlist/hash. This allows the domain to be looked >>> + * up by domid, and therefore to be the subject of hypercalls/etc. >>> + */ >>> +static void domlist_insert(struct domain *d) >>> +{ >>> + struct domain **pd; >>> + >>> + spin_lock(&domlist_update_lock); >>> + >>> + /* domain_list is maintained in domid order. */ >>> + pd = &domain_list; >> Make this the initializer of the variable, if ... >> >>> + for ( ; *pd != NULL; pd = &(*pd)->next_in_list ) >> ... isn't to be the starting clause of the for()? > > Ok. > >> >>> + if ( (*pd)->domain_id > d->domain_id ) >>> + break; >>> + >>> + d->next_in_list = *pd; >>> + d->next_in_hashbucket = domain_hash[DOMAIN_HASH(d->domain_id)]; >>> + rcu_assign_pointer(*pd, d); >>> + rcu_assign_pointer(domain_hash[DOMAIN_HASH(d->domain_id)], d); >> Maybe worth putting the hash in a local variable? > > Ok. > >> >>> + spin_unlock(&domlist_update_lock); >>> +} >>> + >>> +/* >>> + * Remove a domain from the domlist/hash. This means the domain can no longer >>> + * be looked up by domid, and therefore can no longer be the subject of >>> + * *subsequent* hypercalls/etc. In-progress hypercalls/etc can still operate >>> + * on the domain. >>> + */ >>> +static void domlist_remove(struct domain *d) >>> +{ >>> + struct domain **pd = &domain_list; >>> + >>> + spin_lock(&domlist_update_lock); >>> + >>> + pd = &domain_list; >> pd already has an initializer. > > Ah - that was a copy&paste error of mine. I'll drop the initialiser. The initializer? Then please also don't switch to making it the initializer in domlist_insert(), but rather move it into the for(). I think the two functions want to remain in sync in this regard. > The code I copied from strictly initialises *pd with the update lock > held. As we're only taking the address of pointer, I think it's safe to > be outside, but its also just an LEA so also not interesting to > initialise outside. One can really view it both ways, I suppose. The compiler is also free to move it, even across the spin_lock(), aiui. Jan
These are opencoded linked list and hashtable manipulations. Factor them out
into static inline helpers, and discuss the consequence for the domain.
No functional change.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
v2:
* Factor out the hash bucket calculation
* Drop double initilaisation of pd in domlist_remove()
---
xen/common/domain.c | 81 +++++++++++++++++++++++++++++++--------------
1 file changed, 56 insertions(+), 25 deletions(-)
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 256660473861..8d8f40ccb245 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -64,6 +64,58 @@ DEFINE_RCU_READ_LOCK(domlist_read_lock);
static struct domain *domain_hash[DOMAIN_HASH_SIZE];
struct domain *domain_list;
+/*
+ * Insert a domain into the domlist/hash. This allows the domain to be looked
+ * up by domid, and therefore to be the subject of hypercalls/etc.
+ */
+static void domlist_insert(struct domain *d)
+{
+ struct domain **pd, *bucket;
+
+ spin_lock(&domlist_update_lock);
+
+ /* domain_list is maintained in domid order. */
+ for ( pd = &domain_list; *pd != NULL; pd = &(*pd)->next_in_list )
+ if ( (*pd)->domain_id > d->domain_id )
+ break;
+
+ bucket = domain_hash[DOMAIN_HASH(d->domain_id)];
+
+ d->next_in_list = *pd;
+ d->next_in_hashbucket = bucket;
+ rcu_assign_pointer(*pd, d);
+ rcu_assign_pointer(bucket, d);
+
+ spin_unlock(&domlist_update_lock);
+}
+
+/*
+ * Remove a domain from the domlist/hash. This means the domain can no longer
+ * be looked up by domid, and therefore can no longer be the subject of
+ * *subsequent* hypercalls/etc. In-progress hypercalls/etc can still operate
+ * on the domain.
+ */
+static void domlist_remove(struct domain *d)
+{
+ struct domain **pd;
+
+ spin_lock(&domlist_update_lock);
+
+ pd = &domain_list;
+ while ( *pd != d )
+ pd = &(*pd)->next_in_list;
+
+ rcu_assign_pointer(*pd, d->next_in_list);
+
+ pd = &domain_hash[DOMAIN_HASH(d->domain_id)];
+ while ( *pd != d )
+ pd = &(*pd)->next_in_hashbucket;
+
+ rcu_assign_pointer(*pd, d->next_in_hashbucket);
+
+ spin_unlock(&domlist_update_lock);
+}
+
struct domain *hardware_domain __read_mostly;
#ifdef CONFIG_LATE_HWDOM
@@ -589,7 +641,7 @@ struct domain *domain_create(domid_t domid,
struct xen_domctl_createdomain *config,
unsigned int flags)
{
- struct domain *d, **pd, *old_hwdom = NULL;
+ struct domain *d, *old_hwdom = NULL;
enum { INIT_watchdog = 1u<<1,
INIT_evtchn = 1u<<3, INIT_gnttab = 1u<<4, INIT_arch = 1u<<5 };
int err, init_status = 0;
@@ -758,17 +810,7 @@ struct domain *domain_create(domid_t domid,
* Must not fail beyond this point, as our caller doesn't know whether
* the domain has been entered into domain_list or not.
*/
-
- spin_lock(&domlist_update_lock);
- pd = &domain_list; /* NB. domain_list maintained in order of domid. */
- for ( pd = &domain_list; *pd != NULL; pd = &(*pd)->next_in_list )
- if ( (*pd)->domain_id > d->domain_id )
- break;
- d->next_in_list = *pd;
- d->next_in_hashbucket = domain_hash[DOMAIN_HASH(domid)];
- rcu_assign_pointer(*pd, d);
- rcu_assign_pointer(domain_hash[DOMAIN_HASH(domid)], d);
- spin_unlock(&domlist_update_lock);
+ domlist_insert(d);
memcpy(d->handle, config->handle, sizeof(d->handle));
@@ -1232,8 +1274,6 @@ static void cf_check complete_domain_destroy(struct rcu_head *head)
/* Release resources belonging to task @p. */
void domain_destroy(struct domain *d)
{
- struct domain **pd;
-
BUG_ON(!d->is_dying);
/* May be already destroyed, or get_domain() can race us. */
@@ -1242,17 +1282,8 @@ void domain_destroy(struct domain *d)
TRACE_TIME(TRC_DOM0_DOM_REM, d->domain_id);
- /* Delete from task list and task hashtable. */
- spin_lock(&domlist_update_lock);
- pd = &domain_list;
- while ( *pd != d )
- pd = &(*pd)->next_in_list;
- rcu_assign_pointer(*pd, d->next_in_list);
- pd = &domain_hash[DOMAIN_HASH(d->domain_id)];
- while ( *pd != d )
- pd = &(*pd)->next_in_hashbucket;
- rcu_assign_pointer(*pd, d->next_in_hashbucket);
- spin_unlock(&domlist_update_lock);
+ /* Remove from the domlist/hash. */
+ domlist_remove(d);
/* Schedule RCU asynchronous completion of domain destroy. */
call_rcu(&d->rcu, complete_domain_destroy);
base-commit: d2b91faec4d905acebbc80aaec4def825e2f7280
--
2.39.2
© 2016 - 2024 Red Hat, Inc.