| Message ID | 20200327163355.24524-1-borntraeger@de.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/1] s390x/smp: fix detection of "running" | expand |
[1/1] was supposed to be kvm-unit-tests.... On 27.03.20 17:33, Christian Borntraeger wrote: > On s390x hosts with a single CPU, the smp test case hangs (loops). > The check is our restart has finished is wrong. > Sigp sense running status checks if the CPU is currently backed by a > real CPU. This means that on single CPU hosts a sigp sense running > will never claim that a target is running. We need to check for not > being stopped instead. > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > lib/s390x/smp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c > index 2555bf4..5ed8b7b 100644 > --- a/lib/s390x/smp.c > +++ b/lib/s390x/smp.c > @@ -128,7 +128,7 @@ static int smp_cpu_restart_nolock(uint16_t addr, struct psw *psw) > * The order has been accepted, but the actual restart may not > * have been performed yet, so wait until the cpu is running. > */ > - while (!smp_cpu_running(addr)) > + while (smp_cpu_stopped(addr)) > mb(); > cpu->active = true; > return 0; >
On 27.03.20 17:33, Christian Borntraeger wrote: > On s390x hosts with a single CPU, the smp test case hangs (loops). > The check is our restart has finished is wrong. > Sigp sense running status checks if the CPU is currently backed by a > real CPU. This means that on single CPU hosts a sigp sense running > will never claim that a target is running. We need to check for not > being stopped instead. > > Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> > --- > lib/s390x/smp.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c > index 2555bf4..5ed8b7b 100644 > --- a/lib/s390x/smp.c > +++ b/lib/s390x/smp.c > @@ -128,7 +128,7 @@ static int smp_cpu_restart_nolock(uint16_t addr, struct psw *psw) > * The order has been accepted, but the actual restart may not > * have been performed yet, so wait until the cpu is running. > */ > - while (!smp_cpu_running(addr)) > + while (smp_cpu_stopped(addr)) > mb(); > cpu->active = true; > return 0; > Yeah, same as the other issue we fixed before. There is no trusting on SIGP SENSE RUNNING STATUS. Can you please get rid of smp_cpu_running()? (looks like this was the last user)
On 27.03.20 17:55, David Hildenbrand wrote: > On 27.03.20 17:33, Christian Borntraeger wrote: >> On s390x hosts with a single CPU, the smp test case hangs (loops). >> The check is our restart has finished is wrong. >> Sigp sense running status checks if the CPU is currently backed by a >> real CPU. This means that on single CPU hosts a sigp sense running >> will never claim that a target is running. We need to check for not >> being stopped instead. >> >> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >> --- >> lib/s390x/smp.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c >> index 2555bf4..5ed8b7b 100644 >> --- a/lib/s390x/smp.c >> +++ b/lib/s390x/smp.c >> @@ -128,7 +128,7 @@ static int smp_cpu_restart_nolock(uint16_t addr, struct psw *psw) >> * The order has been accepted, but the actual restart may not >> * have been performed yet, so wait until the cpu is running. >> */ >> - while (!smp_cpu_running(addr)) >> + while (smp_cpu_stopped(addr)) >> mb(); >> cpu->active = true; >> return 0; >> > > Yeah, same as the other issue we fixed before. There is no trusting on > SIGP SENSE RUNNING STATUS. > > Can you please get rid of smp_cpu_running()? (looks like this was the > last user) I think we should keep it and rename it to smp_cpu_running_status. This bug actually showed that we should be able to test this feature (which is used for spinlocks in Linux) somehow.
On 27.03.20 19:01, Christian Borntraeger wrote: > > > On 27.03.20 17:55, David Hildenbrand wrote: >> On 27.03.20 17:33, Christian Borntraeger wrote: >>> On s390x hosts with a single CPU, the smp test case hangs (loops). >>> The check is our restart has finished is wrong. >>> Sigp sense running status checks if the CPU is currently backed by a >>> real CPU. This means that on single CPU hosts a sigp sense running >>> will never claim that a target is running. We need to check for not >>> being stopped instead. >>> >>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >>> --- >>> lib/s390x/smp.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c >>> index 2555bf4..5ed8b7b 100644 >>> --- a/lib/s390x/smp.c >>> +++ b/lib/s390x/smp.c >>> @@ -128,7 +128,7 @@ static int smp_cpu_restart_nolock(uint16_t addr, struct psw *psw) >>> * The order has been accepted, but the actual restart may not >>> * have been performed yet, so wait until the cpu is running. >>> */ >>> - while (!smp_cpu_running(addr)) >>> + while (smp_cpu_stopped(addr)) >>> mb(); >>> cpu->active = true; >>> return 0; >>> >> >> Yeah, same as the other issue we fixed before. There is no trusting on >> SIGP SENSE RUNNING STATUS. >> >> Can you please get rid of smp_cpu_running()? (looks like this was the >> last user) > > I think we should keep it and rename it to smp_cpu_running_status. This bug > actually showed that we should be able to test this feature (which is used > for spinlocks in Linux) somehow. AFAIK, there is no trusting on SIGP SENSE RUNNING STATUS at all (I discussed this with Janosch back then). And I don't see a way for a reasonable test either. But if you have plans to add a test, then yes, we can keep it.
On 3/27/20 7:01 PM, Christian Borntraeger wrote: > > > On 27.03.20 17:55, David Hildenbrand wrote: >> On 27.03.20 17:33, Christian Borntraeger wrote: >>> On s390x hosts with a single CPU, the smp test case hangs (loops). >>> The check is our restart has finished is wrong. >>> Sigp sense running status checks if the CPU is currently backed by a >>> real CPU. This means that on single CPU hosts a sigp sense running >>> will never claim that a target is running. We need to check for not >>> being stopped instead. >>> >>> Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> >>> --- >>> lib/s390x/smp.c | 2 +- >>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c >>> index 2555bf4..5ed8b7b 100644 >>> --- a/lib/s390x/smp.c >>> +++ b/lib/s390x/smp.c >>> @@ -128,7 +128,7 @@ static int smp_cpu_restart_nolock(uint16_t addr, struct psw *psw) >>> * The order has been accepted, but the actual restart may not >>> * have been performed yet, so wait until the cpu is running. >>> */ >>> - while (!smp_cpu_running(addr)) >>> + while (smp_cpu_stopped(addr)) >>> mb(); >>> cpu->active = true; >>> return 0; >>> >> >> Yeah, same as the other issue we fixed before. There is no trusting on >> SIGP SENSE RUNNING STATUS. >> >> Can you please get rid of smp_cpu_running()? (looks like this was the >> last user) > > I think we should keep it and rename it to smp_cpu_running_status. This bug > actually showed that we should be able to test this feature (which is used > for spinlocks in Linux) somehow. > We need to move it to the smp test case and at least test it but not use it for any library stuff. When I do firmware testing with the unit tests I need to be able to exercise as much sigp orders as possible.
diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c index 2555bf4..5ed8b7b 100644 --- a/lib/s390x/smp.c +++ b/lib/s390x/smp.c @@ -128,7 +128,7 @@ static int smp_cpu_restart_nolock(uint16_t addr, struct psw *psw) * The order has been accepted, but the actual restart may not * have been performed yet, so wait until the cpu is running. */ - while (!smp_cpu_running(addr)) + while (smp_cpu_stopped(addr)) mb(); cpu->active = true; return 0;
On s390x hosts with a single CPU, the smp test case hangs (loops). The check is our restart has finished is wrong. Sigp sense running status checks if the CPU is currently backed by a real CPU. This means that on single CPU hosts a sigp sense running will never claim that a target is running. We need to check for not being stopped instead. Signed-off-by: Christian Borntraeger <borntraeger@de.ibm.com> --- lib/s390x/smp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)