| Message ID | 20200124100137.28656-1-frankja@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | s390x: sigp: Fix sense running reporting | expand |
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...
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>
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!
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?
> 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)
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);
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(-)