On 01.12.20 10:12, Jan Beulich wrote:
> On 01.12.2020 09:21, Juergen Gross wrote:
>> @@ -260,23 +257,42 @@ static struct cpupool *cpupool_create(
>>   
>>       spin_lock(&cpupool_lock);
>>   
>> -    for_each_cpupool(q)
>> +    if ( poolid != CPUPOOLID_NONE )
>>       {
>> -        last = (*q)->cpupool_id;
>> -        if ( (poolid != CPUPOOLID_NONE) && (last >= poolid) )
>> -            break;
>> +        q = __cpupool_find_by_id(poolid, false);
>> +        if ( !q )
>> +            list_add_tail(&c->list, &cpupool_list);
>> +        else
>> +        {
>> +            list_add_tail(&c->list, &q->list);
>> +            if ( q->cpupool_id == poolid )
>> +            {
>> +                *perr = -EEXIST;
>> +                goto err;
>> +            }
> 
> You bail _after_ having added the new entry to the list?

Yes, this is making exit handling easier.

> 
>> +        }
>> +
>> +        c->cpupool_id = poolid;
>>       }
>> -    if ( *q != NULL )
>> +    else
>>       {
>> -        if ( (*q)->cpupool_id == poolid )
>> +        /* Cpupool 0 is created with specified id at boot and never removed. */
>> +        ASSERT(!list_empty(&cpupool_list));
>> +
>> +        q = list_last_entry(&cpupool_list, struct cpupool, list);
>> +        /* In case of wrap search for first free id. */
>> +        if ( q->cpupool_id == CPUPOOLID_NONE - 1 )
>>           {
>> -            *perr = -EEXIST;
>> -            goto err;
>> +            list_for_each_entry(q, &cpupool_list, list)
>> +                if ( q->cpupool_id + 1 != list_next_entry(q, list)->cpupool_id )
>> +                    break;
>>           }
>> -        c->next = *q;
>> +
>> +        list_add(&c->list, &q->list);
>> +
>> +        c->cpupool_id = q->cpupool_id + 1;
> 
> What guarantees that you managed to find an unused ID, other
> than at current CPU speeds it taking too long to create 4
> billion pools? Since you're doing this under lock, wouldn't
> it help anyway to have a global helper variable pointing at
> the lowest pool followed by an unused ID?

An admin doing that would be quite crazy and wouldn't deserve better.

For being usable a cpupool needs to have a cpu assigned to it. And I
don't think we are coming even close to 4 billion supported cpus. :-)

Yes, it would be possible to create 4 billion empty cpupools, but for
what purpose? There are simpler ways to make the system unusable with
dom0 root access.


Juergen