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