* [PATCH] s390x: sigp: Fix sense running reporting
@ 2020-01-24 10:01 Janosch Frank
2020-01-24 10:05 ` Cornelia Huck
0 siblings, 1 reply; 6+ messages in thread
From: Janosch Frank @ 2020-01-24 10:01 UTC (permalink / raw)
To: qemu-devel; +Cc: borntraeger, thuth, cohuck, qemu-s390x, david
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] s390x: sigp: Fix sense running reporting
2020-01-24 10:01 [PATCH] s390x: sigp: Fix sense running reporting Janosch Frank
@ 2020-01-24 10:05 ` Cornelia Huck
2020-01-24 12:03 ` David Hildenbrand
2020-01-24 13:01 ` Janosch Frank
0 siblings, 2 replies; 6+ messages in thread
From: Cornelia Huck @ 2020-01-24 10:05 UTC (permalink / raw)
To: Janosch Frank; +Cc: qemu-s390x, borntraeger, thuth, qemu-devel, david
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...
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] s390x: sigp: Fix sense running reporting
2020-01-24 10:05 ` Cornelia Huck
@ 2020-01-24 12:03 ` David Hildenbrand
2020-01-24 13:00 ` Janosch Frank
2020-01-24 13:01 ` Janosch Frank
1 sibling, 1 reply; 6+ messages in thread
From: David Hildenbrand @ 2020-01-24 12:03 UTC (permalink / raw)
To: Cornelia Huck, Janosch Frank; +Cc: qemu-s390x, borntraeger, thuth, qemu-devel
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
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] s390x: sigp: Fix sense running reporting
2020-01-24 12:03 ` David Hildenbrand
@ 2020-01-24 13:00 ` Janosch Frank
0 siblings, 0 replies; 6+ messages in thread
From: Janosch Frank @ 2020-01-24 13:00 UTC (permalink / raw)
To: David Hildenbrand, Cornelia Huck
Cc: borntraeger, qemu-s390x, thuth, qemu-devel
[-- Attachment #1.1: Type: text/plain, Size: 2072 bytes --]
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!
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] s390x: sigp: Fix sense running reporting
2020-01-24 10:05 ` Cornelia Huck
2020-01-24 12:03 ` David Hildenbrand
@ 2020-01-24 13:01 ` Janosch Frank
2020-01-24 13:15 ` David Hildenbrand
1 sibling, 1 reply; 6+ messages in thread
From: Janosch Frank @ 2020-01-24 13:01 UTC (permalink / raw)
To: Cornelia Huck; +Cc: qemu-s390x, borntraeger, thuth, qemu-devel, david
[-- Attachment #1.1: Type: text/plain, Size: 1160 bytes --]
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?
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH] s390x: sigp: Fix sense running reporting
2020-01-24 13:01 ` Janosch Frank
@ 2020-01-24 13:15 ` David Hildenbrand
0 siblings, 0 replies; 6+ messages in thread
From: David Hildenbrand @ 2020-01-24 13:15 UTC (permalink / raw)
To: Janosch Frank, Cornelia Huck; +Cc: qemu-s390x, borntraeger, thuth, qemu-devel
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-01-24 13:16 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-01-24 10:01 [PATCH] s390x: sigp: Fix sense running reporting Janosch Frank
2020-01-24 10:05 ` Cornelia Huck
2020-01-24 12:03 ` David Hildenbrand
2020-01-24 13:00 ` Janosch Frank
2020-01-24 13:01 ` Janosch Frank
2020-01-24 13:15 ` David Hildenbrand
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).