The "allbutself" cpumask in stop_machine_run() is not needed. Instead
of allocating it on the stack it can easily be avoided.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 xen/common/stop_machine.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/xen/common/stop_machine.c b/xen/common/stop_machine.c
index ce6f5624c4..ccda2efa3e 100644
--- 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();
     int ret;
 
     BUG_ON(!local_irq_is_enabled());
@@ -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;
 
     /* Must not spin here as the holder will expect us to be descheduled. */
     if ( !spin_trylock(&stopmachine_lock) )
@@ -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 )
+        if ( i != my_cpu )
+            tasklet_schedule_on_cpu(&per_cpu(stopmachine_tasklet, i), i);
 
     stopmachine_set_state(STOPMACHINE_PREPARE);
     stopmachine_wait_state();
-- 
2.16.4


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

>>> On 28.05.19 at 15:08, <jgross@suse.com> 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@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

On 28/05/2019 16:32, Jan Beulich wrote:
>>>> On 28.05.19 at 15:08, <jgross@suse.com> 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.

Okay.

> 
>> @@ -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.

Yes, I have thought about it. If you like it better I test for the
running cpu to be in cpu_online_map.

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

Ah, of course!

> 
>> @@ -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().

Yes.


Juergen

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