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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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@xxxxxxxx>

Thanks.

~Andrew