* [PATCH] xen: remove on-stack cpumask from stop_machine_run()
@ 2019-05-28 13:08 ` Juergen Gross
0 siblings, 0 replies; 6+ messages in thread
From: Juergen Gross @ 2019-05-28 13:08 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Julien Grall, Jan Beulich
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [Xen-devel] [PATCH] xen: remove on-stack cpumask from stop_machine_run()
@ 2019-05-28 13:08 ` Juergen Gross
0 siblings, 0 replies; 6+ messages in thread
From: Juergen Gross @ 2019-05-28 13:08 UTC (permalink / raw)
To: xen-devel
Cc: Juergen Gross, Stefano Stabellini, Wei Liu,
Konrad Rzeszutek Wilk, George Dunlap, Andrew Cooper, Ian Jackson,
Tim Deegan, Julien Grall, Jan Beulich
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] xen: remove on-stack cpumask from stop_machine_run()
@ 2019-05-28 14:32 ` Jan Beulich
0 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2019-05-28 14:32 UTC (permalink / raw)
To: Juergen Gross
Cc: Stefano Stabellini, WeiLiu, Konrad Rzeszutek Wilk, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH] xen: remove on-stack cpumask from stop_machine_run()
@ 2019-05-28 14:32 ` Jan Beulich
0 siblings, 0 replies; 6+ messages in thread
From: Jan Beulich @ 2019-05-28 14:32 UTC (permalink / raw)
To: Juergen Gross
Cc: Stefano Stabellini, WeiLiu, Konrad Rzeszutek Wilk, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xen: remove on-stack cpumask from stop_machine_run()
@ 2019-05-28 14:41 ` Juergen Gross
0 siblings, 0 replies; 6+ messages in thread
From: Juergen Gross @ 2019-05-28 14:41 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, WeiLiu, Konrad Rzeszutek Wilk, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [Xen-devel] [PATCH] xen: remove on-stack cpumask from stop_machine_run()
@ 2019-05-28 14:41 ` Juergen Gross
0 siblings, 0 replies; 6+ messages in thread
From: Juergen Gross @ 2019-05-28 14:41 UTC (permalink / raw)
To: Jan Beulich
Cc: Stefano Stabellini, WeiLiu, Konrad Rzeszutek Wilk, George Dunlap,
Andrew Cooper, Ian Jackson, Tim Deegan, Julien Grall, 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-05-28 14:42 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-28 13:08 [PATCH] xen: remove on-stack cpumask from stop_machine_run() Juergen Gross
2019-05-28 13:08 ` [Xen-devel] " Juergen Gross
2019-05-28 14:32 ` Jan Beulich
2019-05-28 14:32 ` [Xen-devel] " Jan Beulich
2019-05-28 14:41 ` Juergen Gross
2019-05-28 14:41 ` [Xen-devel] " Juergen Gross
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.