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

[Jan Beulich <jbeulich@xxxxxxxx>]

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