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

[Jürgen Groß <jgross@xxxxxxxx>]

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

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: application/pgp-keys

Attachment: OpenPGP_signature
Description: OpenPGP digital signature