Re: [PATCH RFC 09/35] Add cpumask_next_zero set_cpu_present and possible

[Julien Grall <julien.grall@linaro.org>]

Hi Parth,

On 04/02/2015 14:01, parth.dixit@linaro.org wrote:
> From: Naresh Bhat <naresh.bhat@linaro.org>
>
> Introduce and use cpumask_next_zero, set_cpu_present and set_cpu_possible.

Why don't you use cpu_possible_map, cpu_present_map?

> Signed-off-by: Naresh Bhat <naresh.bhat@linaro.org>
> ---
>   xen/common/cpu.c          | 18 ++++++++++++++++++
>   xen/include/xen/cpumask.h | 40 ++++++++++++++++++++++++++++++++++++++++
>   2 files changed, 58 insertions(+)
>
> diff --git a/xen/common/cpu.c b/xen/common/cpu.c
> index 630881e..da399c9 100644
> --- a/xen/common/cpu.c
> +++ b/xen/common/cpu.c
> @@ -216,3 +216,21 @@ void enable_nonboot_cpus(void)
>
>       cpumask_clear(&frozen_cpus);
>   }
> +
> +static DECLARE_BITMAP(cpu_present_bits, NR_CPUS) __read_mostly;
> +static DECLARE_BITMAP(cpu_possible_bits, NR_CPUS) __read_mostly;
> +void set_cpu_possible(unsigned int cpu, bool possible)
> +{
> +    if ( possible )
> +         cpumask_set_cpu(cpu, to_cpumask(cpu_possible_bits));
> +    else
> +        cpumask_clear_cpu(cpu, to_cpumask(cpu_possible_bits));
> +}
> +
> +void set_cpu_present(unsigned int cpu, bool present)
> +{
> +    if ( present )
> +        cpumask_set_cpu(cpu, to_cpumask(cpu_present_bits));
> +    else
> +        cpumask_clear_cpu(cpu, to_cpumask(cpu_present_bits));
> +}

What's the purpose of declaring two bitmaps but never use them?

> diff --git a/xen/include/xen/cpumask.h b/xen/include/xen/cpumask.h
> index 850b4a2..209483e 100644
> --- a/xen/include/xen/cpumask.h
> +++ b/xen/include/xen/cpumask.h
> @@ -78,6 +78,7 @@
>   #include <xen/bitmap.h>
>   #include <xen/kernel.h>
>   #include <xen/random.h>
> +#include <xen/stdbool.h>

Please don't include stdbool.h and use bool_t instead (defined in types.h)

>
>   typedef struct cpumask{ DECLARE_BITMAP(bits, NR_CPUS); } cpumask_t;
>
> @@ -295,6 +296,22 @@ static inline int cpulist_scnprintf(char *buf, int len,
>   }
>
>   /*
> + * cpumask_next_zero - get the next unset cpu in a cpumask
> + * @n: the cpu prior to the place to search (ie. return will be > @n)
> + * @srcp: the cpumask pointer
> + *
> + * Returns >= nr_cpu_ids if no further cpus unset.
> + */
> +static inline unsigned int cpumask_next_zero(int n, const cpumask_t *srcp)
> +{
> +    /* -1 is a legal arg here. */
> +    if (n != -1)
> +        cpumask_check(n);
> +
> +    return find_next_zero_bit(srcp->bits, nr_cpu_ids, n+1);
> +}
> +
> +/*
>    * cpumask_var_t: struct cpumask for stack usage.
>    *
>    * Oh, the wicked games we play!  In order to make kernel coding a
> @@ -440,6 +457,29 @@ extern cpumask_t cpu_present_map;
>   #define for_each_online_cpu(cpu)   for_each_cpu(cpu, &cpu_online_map)
>   #define for_each_present_cpu(cpu)  for_each_cpu(cpu, &cpu_present_map)
>
> +/* Wrappers for arch boot code to manipulate normally-constant masks */
> +void set_cpu_possible(unsigned int cpu, bool possible);
> +void set_cpu_present(unsigned int cpu, bool present);
> +
> +/*
> + * to_cpumask - convert an NR_CPUS bitmap to a struct cpumask *
> + * @bitmap: the bitmap
> + *
> + * There are a few places where cpumask_var_t isn't appropriate and
> + * static cpumasks must be used (eg. very early boot), yet we don't
> + * expose the definition of 'struct cpumask'.
> + *
> + * This does the conversion, and can be used as a constant initializer.
> + */
> +#define to_cpumask(bitmap)                                              \
> +        ((struct cpumask *)(1 ? (bitmap)                                \
> +                            : (void *)sizeof(__check_is_bitmap(bitmap))))
> +
> +static inline int __check_is_bitmap(const unsigned long *bitmap)
> +{
> +    return 1;
> +}
> +
>   /* Copy to/from cpumap provided by control tools. */
>   struct xenctl_bitmap;
>   int cpumask_to_xenctl_bitmap(struct xenctl_bitmap *, const cpumask_t *);
>

Regards,

-- 
Julien Grall