Re: [Xen-devel] [PATCH] xen: remove on-stack cpumask from stop_machine_run()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 28.05.19 at 15:08, <jgross@xxxxxxxx> wrote:
> --- a/xen/common/stop_machine.c
> +++ b/xen/common/stop_machine.c
> @@ -69,8 +69,8 @@ static void stopmachine_wait_state(void)
>  
>  int stop_machine_run(int (*fn)(void *), void *data, unsigned int cpu)
>  {
> -    cpumask_t allbutself;
>      unsigned int i, nr_cpus;
> +    unsigned int my_cpu = smp_processor_id();

Variables starting with my_ being commonly used in introductory
examples, I'd prefer to avoid such names. Elsewhere we use
"this_cpu", "me", or maybe "this" if plain "cpu" is already taken.

> @@ -79,9 +79,7 @@ int stop_machine_run(int (*fn)(void *), void *data, 
> unsigned int cpu)
>      if ( !get_cpu_maps() )
>          return -EBUSY;
>  
> -    cpumask_andnot(&allbutself, &cpu_online_map,
> -                   cpumask_of(smp_processor_id()));
> -    nr_cpus = cpumask_weight(&allbutself);
> +    nr_cpus = cpumask_weight(&cpu_online_map) - 1;

Having looked at a lot of CPU offlining code recently, I notice this
isn't strictly correct: You imply cpu_online(my_cpu) to produce
"true". I think at present this will always hold, but I'd prefer if we
could avoid gaining such a dependency. And it doesn't look overly
difficult to avoid it.

Also please don't open-code num_online_cpus().

> @@ -100,8 +98,9 @@ int stop_machine_run(int (*fn)(void *), void *data, 
> unsigned int cpu)
>  
>      smp_wmb();
>  
> -    for_each_cpu ( i, &allbutself )
> -        tasklet_schedule_on_cpu(&per_cpu(stopmachine_tasklet, i), i);
> +    for_each_cpu ( i, &cpu_online_map )

Same here for for_each_online_cpu().

Jan



_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel