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.2On 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 - 2026 Red Hat, Inc.