s390x: sigp: Fix sense running reporting
diff mbox series

Message ID 20200124100137.28656-1-frankja@linux.ibm.com
State New
Headers show
Series
  • s390x: sigp: Fix sense running reporting
Related show

Commit Message

Janosch Frank Jan. 24, 2020, 10:01 a.m. UTC
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(-)

Comments

Cornelia Huck Jan. 24, 2020, 10:05 a.m. UTC | #1
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...
David Hildenbrand Jan. 24, 2020, 12:03 p.m. UTC | #2
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>
Janosch Frank Jan. 24, 2020, 1 p.m. UTC | #3
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!
Janosch Frank Jan. 24, 2020, 1:01 p.m. UTC | #4
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?
David Hildenbrand Jan. 24, 2020, 1:15 p.m. UTC | #5
> 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)

Patch
diff mbox series

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);