diff mbox series

xen/domain: Factor domlist_{insert,remove}() out of domain_{create,destroy}()

Message ID 20240731120406.3429430-1-andrew.cooper3@citrix.com (mailing list archive)
State Superseded
Headers show
Series xen/domain: Factor domlist_{insert,remove}() out of domain_{create,destroy}() | expand

Commit Message

Andrew Cooper July 31, 2024, 12:04 p.m. UTC 
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(-)


base-commit: 561cba38ff551383a628dc93e64ab0691cfc92bf

Comments

Jan Beulich July 31, 2024, 12:14 p.m. UTC | #1 
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
Andrew Cooper July 31, 2024, 2:07 p.m. UTC | #2 
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
Jan Beulich July 31, 2024, 2:12 p.m. UTC | #3 
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
diff mbox series

Patch

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);