Re: [PATCH 2/5] stop_machine: yield CPU during stop machine

Re: [PATCH 2/5] stop_machine: yield CPU during stop machine

[Peter Zijlstra]

On Fri, Oct 21, 2016 at 01:58:55PM +0200, Christian Borntraeger wrote:
> stop_machine can take a very long time if the hypervisor does
> overcommitment for guest CPUs. When waiting for "the one", lets
> give up our CPU by using the new cpu_relax_yield.

This seems something that would apply to most other virt stuff. Lets Cc
a few more lists for that.

> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> ---
>  kernel/stop_machine.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
> index ec9ab2f..1eb8266 100644
> --- a/kernel/stop_machine.c
> +++ b/kernel/stop_machine.c
> @@ -194,7 +194,7 @@ static int multi_cpu_stop(void *data)
>  	/* Simple state machine */
>  	do {
>  		/* Chill out and ensure we re-read multi_stop_state. */
> -		cpu_relax();
> +		cpu_relax_yield();
>  		if (msdata->state != curstate) {
>  			curstate = msdata->state;
>  			switch (curstate) {
> -- 
> 2.5.5
> 

Re: [PATCH 2/5] stop_machine: yield CPU during stop machine

[Juergen Gross]

On 21/10/16 14:05, Peter Zijlstra wrote:
> On Fri, Oct 21, 2016 at 01:58:55PM +0200, Christian Borntraeger wrote:
>> stop_machine can take a very long time if the hypervisor does
>> overcommitment for guest CPUs. When waiting for "the one", lets
>> give up our CPU by using the new cpu_relax_yield.
> 
> This seems something that would apply to most other virt stuff. Lets Cc
> a few more lists for that.

Corrected xen-devel mail address.


Juergen

> 
>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>> ---
>>  kernel/stop_machine.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
>> index ec9ab2f..1eb8266 100644
>> --- a/kernel/stop_machine.c
>> +++ b/kernel/stop_machine.c
>> @@ -194,7 +194,7 @@ static int multi_cpu_stop(void *data)
>>  	/* Simple state machine */
>>  	do {
>>  		/* Chill out and ensure we re-read multi_stop_state. */
>> -		cpu_relax();
>> +		cpu_relax_yield();
>>  		if (msdata->state != curstate) {
>>  			curstate = msdata->state;
>>  			switch (curstate) {
>> -- 
>> 2.5.5
>>
> _______________________________________________
> Virtualization mailing list
> Virtualization@lists.linux-foundation.org
> https://lists.linuxfoundation.org/mailman/listinfo/virtualization
> 

Re: [PATCH 2/5] stop_machine: yield CPU during stop machine

[Nicholas Piggin]

On Fri, 21 Oct 2016 14:05:36 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, Oct 21, 2016 at 01:58:55PM +0200, Christian Borntraeger wrote:
> > stop_machine can take a very long time if the hypervisor does
> > overcommitment for guest CPUs. When waiting for "the one", lets
> > give up our CPU by using the new cpu_relax_yield.  
> 
> This seems something that would apply to most other virt stuff. Lets Cc
> a few more lists for that.
> 
> > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
> > ---
> >  kernel/stop_machine.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
> > index ec9ab2f..1eb8266 100644
> > --- a/kernel/stop_machine.c
> > +++ b/kernel/stop_machine.c
> > @@ -194,7 +194,7 @@ static int multi_cpu_stop(void *data)
> >  	/* Simple state machine */
> >  	do {
> >  		/* Chill out and ensure we re-read multi_stop_state. */
> > -		cpu_relax();
> > +		cpu_relax_yield();
> >  		if (msdata->state != curstate) {
> >  			curstate = msdata->state;
> >  			switch (curstate) {
> > -- 
> > 2.5.5
> >   

This is the only caller of cpu_relax_yield()?

As a step to removing cpu_yield_lowlatency this series is nice so I
have no objection. But "general" kernel coders still have basically
no chance of using this properly.

I wonder what can be done about that. I've got that spin_do/while
series I'll rebase on top of this, but a spin_yield variant of them
is of no more help to the caller.

What makes this unique? Long latency and not performance critical?
Most places where we spin and maybe yield have been moved to arch
code, but I wonder whether we can make an easier to use architecture
independent API?

Thanks,
Nick

Re: [PATCH 2/5] stop_machine: yield CPU during stop machine

[Christian Borntraeger]

On 10/22/2016 02:06 AM, Nicholas Piggin wrote:
> On Fri, 21 Oct 2016 14:05:36 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
>> On Fri, Oct 21, 2016 at 01:58:55PM +0200, Christian Borntraeger wrote:
>>> stop_machine can take a very long time if the hypervisor does
>>> overcommitment for guest CPUs. When waiting for "the one", lets
>>> give up our CPU by using the new cpu_relax_yield.  
>>
>> This seems something that would apply to most other virt stuff. Lets Cc
>> a few more lists for that.
>>
>>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com>
>>> ---
>>>  kernel/stop_machine.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/kernel/stop_machine.c b/kernel/stop_machine.c
>>> index ec9ab2f..1eb8266 100644
>>> --- a/kernel/stop_machine.c
>>> +++ b/kernel/stop_machine.c
>>> @@ -194,7 +194,7 @@ static int multi_cpu_stop(void *data)
>>>  	/* Simple state machine */
>>>  	do {
>>>  		/* Chill out and ensure we re-read multi_stop_state. */
>>> -		cpu_relax();
>>> +		cpu_relax_yield();
>>>  		if (msdata->state != curstate) {
>>>  			curstate = msdata->state;
>>>  			switch (curstate) {
>>> -- 
>>> 2.5.5
>>>   
> 
> This is the only caller of cpu_relax_yield()?

As of today yes. Right now the yielding (call to hypervisor) in 
cpu_relax is only done for s390. Some time ago Heiko did remove 
that also from s390 with commit 57f2ffe14fd125c2 ("s390: remove 
diag 44 calls from cpu_relax()")

As it turns out this make stop_machine run really slow on virtualized
systems. For example the kprobes test during bootup took several seconds 
instead of just running unnoticed with large guests. Therefore, we 
reintroduced that with commit 4d92f50249eb ("s390: reintroduce diag 44
calls for cpu_relax()"), but the only place where we noticed the missing
yield was in the stop_machine code.

I would assume that we might find some other places where this makes
sense in the future, but I expect that we have much less places for 
yield than we need for lowlatency.

PS: We do something similar for our arch implementation for spinlocks,
but there  we use the directed yield as we know which CPU holds the lock.


> 
> As a step to removing cpu_yield_lowlatency this series is nice so I
> have no objection. But "general" kernel coders still have basically
> no chance of using this properly.
> 
> I wonder what can be done about that. I've got that spin_do/while
> series I'll rebase on top of this, but a spin_yield variant of them
> is of no more help to the caller.
> 
> What makes this unique? Long latency and not performance critical?

I think what makes this unique is that ALL cpus spin and wait for one.
It was really the only place that I noticed a regression with Heikos
first patch.

> Most places where we spin and maybe yield have been moved to arch
> code, but I wonder whether we can make an easier to use architecture
> independent API?


Peter, I will fixup the patch set (I forgot to remove the lowlatency
in 2 places) and push it on my tree for linux-next. Lets see what happens.
Would the tip tree be the right place if things work out ok?

Re: [PATCH 2/5] stop_machine: yield CPU during stop machine

[Peter Zijlstra]

On Mon, Oct 24, 2016 at 09:52:31AM +0200, Christian Borntraeger wrote:
> Peter, I will fixup the patch set (I forgot to remove the lowlatency
> in 2 places) and push it on my tree for linux-next. Lets see what happens.
> Would the tip tree be the right place if things work out ok?

I think so, you're touching a fair bit of kernel/locking/ and there's
bound to be some conflicts with work there. So carrying it in the
locking tree might be best.

Re: [PATCH/RFC 0/5] cpu_relax: introduce yield, remove lowlatency

[Christian Borntraeger]

On 10/21/2016 04:57 PM, David Miller wrote:
> From: Christian Borntraeger <borntraeger@de.ibm.com>
> Date: Fri, 21 Oct 2016 13:58:53 +0200
> 
>> For spinning loops people did often use barrier() or cpu_relax().
>> For most architectures cpu_relax and barrier are the same, but on
>> some architectures cpu_relax can add some latency. For example on s390
>> cpu_relax gives up the time slice to the hypervisor. On power cpu_relax
>> tries to give some of the CPU to the neighbor threads. To reduce the
>> latency another variant cpu_relax_lowlatency was introduced. Before this
>> is used in more and more places, lets revert the logic of provide a new
>> function cpu_relax_yield that can spend some time and for s390 yields
>> the guest CPU.
> 
> Sparc64, fwiw, behaves similarly to powerpc.

As sparc currently defines cpu_relax_lowlatency to cpu_relax, this patch set
should be a no-op then for sparc, correct?

My intend was that cpu_relax should not add a huge latency but can certainly
push some cpu power to hardware threads of the same core. This seems to be
the case for sparc/power and some arc variants. 

Christian

Re: [PATCH/RFC 0/5] cpu_relax: introduce yield, remove lowlatency

[David Miller]

From: Christian Borntraeger <borntraeger@de.ibm.com>
Date: Fri, 21 Oct 2016 17:08:54 +0200

> On 10/21/2016 04:57 PM, David Miller wrote:
>> From: Christian Borntraeger <borntraeger@de.ibm.com>
>> Date: Fri, 21 Oct 2016 13:58:53 +0200
>> 
>>> For spinning loops people did often use barrier() or cpu_relax().
>>> For most architectures cpu_relax and barrier are the same, but on
>>> some architectures cpu_relax can add some latency. For example on s390
>>> cpu_relax gives up the time slice to the hypervisor. On power cpu_relax
>>> tries to give some of the CPU to the neighbor threads. To reduce the
>>> latency another variant cpu_relax_lowlatency was introduced. Before this
>>> is used in more and more places, lets revert the logic of provide a new
>>> function cpu_relax_yield that can spend some time and for s390 yields
>>> the guest CPU.
>> 
>> Sparc64, fwiw, behaves similarly to powerpc.
> 
> As sparc currently defines cpu_relax_lowlatency to cpu_relax, this patch set
> should be a no-op then for sparc, correct?
> 
> My intend was that cpu_relax should not add a huge latency but can certainly
> push some cpu power to hardware threads of the same core. This seems to be
> the case for sparc/power and some arc variants. 

Agreed.