Re: [PATCH v2 05/17] xen/cpupool: switch cpupool list to normal list interface

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

> +        }
> +
> +        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?

Jan