[v1] s390x: sigp: Fix sense running reporting

[PATCH] s390x: sigp: Fix sense running reporting

Posted by Janosch Frank 5 years, 9 months ago

The logic was inversed and reported running if the cpu was stopped.
Let's fix that.

Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
---
 target/s390x/sigp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
index 727875bb4a..286c0d6c9c 100644
--- a/target/s390x/sigp.c
+++ b/target/s390x/sigp.c
@@ -347,7 +347,7 @@ static void sigp_sense_running(S390CPU *dst_cpu, SigpInfo *si)
     }
 
     /* If halted (which includes also STOPPED), it is not running */
-    if (CPU(dst_cpu)->halted) {
+    if (!CPU(dst_cpu)->halted) {
         si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
     } else {
         set_sigp_status(si, SIGP_STAT_NOT_RUNNING);
-- 
2.20.1

Re: [PATCH] s390x: sigp: Fix sense running reporting

Posted by Cornelia Huck 5 years, 9 months ago

On Fri, 24 Jan 2020 05:01:37 -0500
Janosch Frank <frankja@linux.ibm.com> wrote:

> The logic was inversed and reported running if the cpu was stopped.

s/inversed/inverted/ ?

> Let's fix that.
>

Fixes: d1b468bc8869 ("s390x/tcg: implement SIGP SENSE RUNNING STATUS")

> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
> ---
>  target/s390x/sigp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
> index 727875bb4a..286c0d6c9c 100644
> --- a/target/s390x/sigp.c
> +++ b/target/s390x/sigp.c
> @@ -347,7 +347,7 @@ static void sigp_sense_running(S390CPU *dst_cpu, SigpInfo *si)
>      }
>  
>      /* If halted (which includes also STOPPED), it is not running */
> -    if (CPU(dst_cpu)->halted) {
> +    if (!CPU(dst_cpu)->halted) {
>          si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
>      } else {
>          set_sigp_status(si, SIGP_STAT_NOT_RUNNING);

I'm wondering why nobody noticed this before...

Re: [PATCH] s390x: sigp: Fix sense running reporting

Posted by David Hildenbrand 5 years, 9 months ago

On 24.01.20 11:05, Cornelia Huck wrote:
> On Fri, 24 Jan 2020 05:01:37 -0500
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> The logic was inversed and reported running if the cpu was stopped.
> 
> s/inversed/inverted/ ?
> 
>> Let's fix that.
>>
> 
> Fixes: d1b468bc8869 ("s390x/tcg: implement SIGP SENSE RUNNING STATUS")
> 
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  target/s390x/sigp.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
>> index 727875bb4a..286c0d6c9c 100644
>> --- a/target/s390x/sigp.c
>> +++ b/target/s390x/sigp.c
>> @@ -347,7 +347,7 @@ static void sigp_sense_running(S390CPU *dst_cpu, SigpInfo *si)
>>      }
>>  
>>      /* If halted (which includes also STOPPED), it is not running */
>> -    if (CPU(dst_cpu)->halted) {
>> +    if (!CPU(dst_cpu)->halted) {
>>          si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
>>      } else {
>>          set_sigp_status(si, SIGP_STAT_NOT_RUNNING);
> 
> I'm wondering why nobody noticed this before...

AFAIR, it "SENSE RUNNING" allows you to test if the target CPU is
scheduled by the hypervisor. So it is used for performance optimization,
but correctness of a program barely depends on it.

a) You can always return "not running" and it would be totally fine

b) Return "running" would also most probably always valid (although it
does not make too much sense for STOPPED CPUs).

E.g., in KVM we set CPUSTAT_RUNNING whenever we load the CPU. This can
also happen (AFAIR) when the CPU state is already stopped (e.g.,
currently getting stopped) or still sleeping (e.g., about to wake up, or
in kvm_vcpu_block()).

Long story short: There is no trusting on these values.

But yeah, the heuristic we are using is sub-optimal. :)

Reviewed-by: David Hildenbrand <david@redhat.com>

-- 
Thanks,

David / dhildenb

Re: [PATCH] s390x: sigp: Fix sense running reporting

Posted by Janosch Frank 5 years, 9 months ago

On 1/24/20 1:03 PM, David Hildenbrand wrote:
> On 24.01.20 11:05, Cornelia Huck wrote:
>> On Fri, 24 Jan 2020 05:01:37 -0500
>> Janosch Frank <frankja@linux.ibm.com> wrote:
>>
>>> The logic was inversed and reported running if the cpu was stopped.
>>
>> s/inversed/inverted/ ?
>>
>>> Let's fix that.
>>>
>>
>> Fixes: d1b468bc8869 ("s390x/tcg: implement SIGP SENSE RUNNING STATUS")
>>
>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>> ---
>>>  target/s390x/sigp.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
>>> index 727875bb4a..286c0d6c9c 100644
>>> --- a/target/s390x/sigp.c
>>> +++ b/target/s390x/sigp.c
>>> @@ -347,7 +347,7 @@ static void sigp_sense_running(S390CPU *dst_cpu, SigpInfo *si)
>>>      }
>>>  
>>>      /* If halted (which includes also STOPPED), it is not running */
>>> -    if (CPU(dst_cpu)->halted) {
>>> +    if (!CPU(dst_cpu)->halted) {
>>>          si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
>>>      } else {
>>>          set_sigp_status(si, SIGP_STAT_NOT_RUNNING);
>>
>> I'm wondering why nobody noticed this before...
> 
> AFAIR, it "SENSE RUNNING" allows you to test if the target CPU is
> scheduled by the hypervisor. So it is used for performance optimization,
> but correctness of a program barely depends on it.
> 
> a) You can always return "not running" and it would be totally fine
> 
> b) Return "running" would also most probably always valid (although it
> does not make too much sense for STOPPED CPUs).
> 
> E.g., in KVM we set CPUSTAT_RUNNING whenever we load the CPU. This can
> also happen (AFAIR) when the CPU state is already stopped (e.g.,
> currently getting stopped) or still sleeping (e.g., about to wake up, or
> in kvm_vcpu_block()).
> 
> 
> Long story short: There is no trusting on these values.

That answer makes me highly uncomfortable...

> 
> 
> But yeah, the heuristic we are using is sub-optimal. :)
> 
> Reviewed-by: David Hildenbrand <david@redhat.com>

Thanks!

Re: [PATCH] s390x: sigp: Fix sense running reporting

Posted by Janosch Frank 5 years, 9 months ago

On 1/24/20 11:05 AM, Cornelia Huck wrote:
> On Fri, 24 Jan 2020 05:01:37 -0500
> Janosch Frank <frankja@linux.ibm.com> wrote:
> 
>> The logic was inversed and reported running if the cpu was stopped.
> 
> s/inversed/inverted/ ?

Ok

> 
>> Let's fix that.
>>
> 
> Fixes: d1b468bc8869 ("s390x/tcg: implement SIGP SENSE RUNNING STATUS")

Added

> 
>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>> ---
>>  target/s390x/sigp.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/target/s390x/sigp.c b/target/s390x/sigp.c
>> index 727875bb4a..286c0d6c9c 100644
>> --- a/target/s390x/sigp.c
>> +++ b/target/s390x/sigp.c
>> @@ -347,7 +347,7 @@ static void sigp_sense_running(S390CPU *dst_cpu, SigpInfo *si)
>>      }
>>  
>>      /* If halted (which includes also STOPPED), it is not running */
>> -    if (CPU(dst_cpu)->halted) {
>> +    if (!CPU(dst_cpu)->halted) {
>>          si->cc = SIGP_CC_ORDER_CODE_ACCEPTED;
>>      } else {
>>          set_sigp_status(si, SIGP_STAT_NOT_RUNNING);
> 
> I'm wondering why nobody noticed this before...

No idea.
How many people use smp with tcg?

Re: [PATCH] s390x: sigp: Fix sense running reporting

Posted by David Hildenbrand 5 years, 9 months ago

> No idea.
> How many people use smp with tcg?
> 

The kernel uses it (pcpu_running()) to decide whether
- To send an emergency call or an external call (external calls are
  faster when the CPU is already running - e.g., SIE HW assist)
- In arch_vcpu_is_preempted(), for an heuristic

So nothing actually breaks (as I explained, only used for performance
optimizations)

-- 
Thanks,

David / dhildenb