Re: [PATCH v2 04/17] xen/cpupool: switch cpupool id to unsigned

[Jan Beulich <jbeulich@xxxxxxxx>]

On 01.12.2020 09:21, Juergen Gross wrote:
> @@ -243,11 +243,11 @@ void cpupool_put(struct cpupool *pool)
>   * - unknown scheduler
>   */
>  static struct cpupool *cpupool_create(
> -    int poolid, unsigned int sched_id, int *perr)
> +    unsigned int poolid, unsigned int sched_id, int *perr)
>  {
>      struct cpupool *c;
>      struct cpupool **q;
> -    int last = 0;
> +    unsigned int last = 0;
>  
>      *perr = -ENOMEM;
>      if ( (c = alloc_cpupool_struct()) == NULL )
> @@ -256,7 +256,7 @@ static struct cpupool *cpupool_create(
>      /* One reference for caller, one reference for cpupool_destroy(). */
>      atomic_set(&c->refcnt, 2);
>  
> -    debugtrace_printk("cpupool_create(pool=%d,sched=%u)\n", poolid, 
> sched_id);
> +    debugtrace_printk("cpupool_create(pool=%u,sched=%u)\n", poolid, 
> sched_id);
>  
>      spin_lock(&cpupool_lock);

Below from here we have

    c->cpupool_id = (poolid == CPUPOOLID_NONE) ? (last + 1) : poolid;

which I think can (a) wrap to zero and (b) cause a pool with id
CPUPOOLID_NONE to be created. The former is bad in any event, and
the latter will cause confusion at least with cpupool_add_domain()
and cpupool_get_id(). I realize this is a tangential problem, i.e.
may want fixing in a separate change.

> --- a/xen/common/sched/private.h
> +++ b/xen/common/sched/private.h
> @@ -505,8 +505,8 @@ static inline void sched_unit_unpause(const struct 
> sched_unit *unit)
>  
>  struct cpupool
>  {
> -    int              cpupool_id;
> -#define CPUPOOLID_NONE    (-1)
> +    unsigned int     cpupool_id;
> +#define CPUPOOLID_NONE    (~0U)

How about using XEN_SYSCTL_CPUPOOL_PAR_ANY here? Furthermore,
together with the remark above, I think you also want to consider
the case of sizeof(unsigned int) > sizeof(uint32_t).

Jan