| Message ID | 20220303210425.1693486-7-farman@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | s390x: SIGP fixes | expand |
On 3/3/22 22:04, Eric Farman wrote: > A SIGP SENSE is used to determine if a CPU is stopped or operating, > and thus has a vested interest in ensuring it received a CC0 or CC1, > instead of a CC2 (BUSY). But, any order could receive a CC2 response, > and is probably ill-equipped to respond to it. sigp sense running status doesn't return a cc2, only sigp sense does afaik. Looking at the KVM implementation tells me that it's not doing more than looking at the R bit in the sblk. > > In practice, the order is likely to only encounter this when racing > with a SIGP STOP (AND STORE STATUS) or SIGP RESTART order, which are > unlikely. But, since it's not impossible, let's convert the library > calls that issue a SIGP to loop on CC2 so the callers do not need > to react to that possible outcome. > > Signed-off-by: Eric Farman <farman@linux.ibm.com> > --- > lib/s390x/smp.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c > index 85b046a5..2e476264 100644 > --- a/lib/s390x/smp.c > +++ b/lib/s390x/smp.c > @@ -85,7 +85,7 @@ bool smp_cpu_stopped(uint16_t idx) > > bool smp_sense_running_status(uint16_t idx) > { > - if (smp_sigp(idx, SIGP_SENSE_RUNNING, 0, NULL) != SIGP_CC_STATUS_STORED) > + if (smp_sigp_retry(idx, SIGP_SENSE_RUNNING, 0, NULL) != SIGP_CC_STATUS_STORED) > return true; > /* Status stored condition code is equivalent to cpu not running. */ > return false; > @@ -169,7 +169,7 @@ static int smp_cpu_restart_nolock(uint16_t idx, struct psw *psw) > * running after the restart. > */ > smp_cpu_stop_nolock(idx, false); > - rc = smp_sigp(idx, SIGP_RESTART, 0, NULL); > + rc = smp_sigp_retry(idx, SIGP_RESTART, 0, NULL); > if (rc) > return rc; > /*
On Fri, 2022-03-04 at 11:56 +0100, Janosch Frank wrote: > On 3/3/22 22:04, Eric Farman wrote: > > A SIGP SENSE is used to determine if a CPU is stopped or operating, > > and thus has a vested interest in ensuring it received a CC0 or > > CC1, > > instead of a CC2 (BUSY). But, any order could receive a CC2 > > response, > > and is probably ill-equipped to respond to it. > > sigp sense running status doesn't return a cc2, only sigp sense does > afaik. The KVM routine handle_sigp_dst() returns the CC2 if a STOP/RESTART IRQ is pending for any non-reset order, before it gets to the switch statement that would route to the SIGP SENSE RUNNING handler. > Looking at the KVM implementation tells me that it's not doing more > than > looking at the R bit in the sblk. > > > In practice, the order is likely to only encounter this when racing > > with a SIGP STOP (AND STORE STATUS) or SIGP RESTART order, which > > are > > unlikely. But, since it's not impossible, let's convert the library > > calls that issue a SIGP to loop on CC2 so the callers do not need > > to react to that possible outcome. > > > > Signed-off-by: Eric Farman <farman@linux.ibm.com> > > --- > > lib/s390x/smp.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c > > index 85b046a5..2e476264 100644 > > --- a/lib/s390x/smp.c > > +++ b/lib/s390x/smp.c > > @@ -85,7 +85,7 @@ bool smp_cpu_stopped(uint16_t idx) > > > > bool smp_sense_running_status(uint16_t idx) > > { > > - if (smp_sigp(idx, SIGP_SENSE_RUNNING, 0, NULL) != > > SIGP_CC_STATUS_STORED) > > + if (smp_sigp_retry(idx, SIGP_SENSE_RUNNING, 0, NULL) != > > SIGP_CC_STATUS_STORED) > > return true; > > /* Status stored condition code is equivalent to cpu not > > running. */ > > return false; > > @@ -169,7 +169,7 @@ static int smp_cpu_restart_nolock(uint16_t idx, > > struct psw *psw) > > * running after the restart. > > */ > > smp_cpu_stop_nolock(idx, false); > > - rc = smp_sigp(idx, SIGP_RESTART, 0, NULL); > > + rc = smp_sigp_retry(idx, SIGP_RESTART, 0, NULL); > > if (rc) > > return rc; > > /*
On Fri, 2022-03-04 at 11:56 +0100, Janosch Frank wrote: > On 3/3/22 22:04, Eric Farman wrote: > > A SIGP SENSE is used to determine if a CPU is stopped or operating, > > and thus has a vested interest in ensuring it received a CC0 or > > CC1, > > instead of a CC2 (BUSY). But, any order could receive a CC2 > > response, > > and is probably ill-equipped to respond to it. > > sigp sense running status doesn't return a cc2, only sigp sense does > afaik. > Looking at the KVM implementation tells me that it's not doing more > than > looking at the R bit in the sblk. From the POP I read _all_ orders may indeed return CC=2: case 1 under "Conditions precluding Interpretation of the Order Code". That being said, there are a few more users of smp_sigp (no retry) in smp.c (the test, not the lib). Does it make sense to fix them aswell?
On Mon, 2022-03-07 at 15:42 +0100, Nico Boehr wrote: > On Fri, 2022-03-04 at 11:56 +0100, Janosch Frank wrote: > > On 3/3/22 22:04, Eric Farman wrote: > > > A SIGP SENSE is used to determine if a CPU is stopped or > > > operating, > > > and thus has a vested interest in ensuring it received a CC0 or > > > CC1, > > > instead of a CC2 (BUSY). But, any order could receive a CC2 > > > response, > > > and is probably ill-equipped to respond to it. > > > > sigp sense running status doesn't return a cc2, only sigp sense > > does > > afaik. > > Looking at the KVM implementation tells me that it's not doing more > > than > > looking at the R bit in the sblk. > > From the POP I read _all_ orders may indeed return CC=2: case 1 under > "Conditions precluding Interpretation of the Order Code". > > That being said, there are a few more users of smp_sigp (no retry) in > smp.c (the test, not the lib). > > Does it make sense to fix them aswell? I thought it made sense to do the lib, since other places expect those things to "just work." But for the tests themselves, I struggle to convince myself with one path over another. The only way KVM returns a CC2 is because of a concurrent STOP/RESTART, which isn't a possibility because of the waiting the lib itself does when invoking the STOP/RESTART. So should the tests be looking for an unexpected CC2? Or just loop when they occur? If the latter, shouldn't the lib itself do that? I'm happy to make changes, I just can't decide which it should be. Any opinions? Eric
On 3/7/22 21:15, Eric Farman wrote: > On Mon, 2022-03-07 at 15:42 +0100, Nico Boehr wrote: >> On Fri, 2022-03-04 at 11:56 +0100, Janosch Frank wrote: >>> On 3/3/22 22:04, Eric Farman wrote: >>>> A SIGP SENSE is used to determine if a CPU is stopped or >>>> operating, >>>> and thus has a vested interest in ensuring it received a CC0 or >>>> CC1, >>>> instead of a CC2 (BUSY). But, any order could receive a CC2 >>>> response, >>>> and is probably ill-equipped to respond to it. >>> >>> sigp sense running status doesn't return a cc2, only sigp sense >>> does >>> afaik. >>> Looking at the KVM implementation tells me that it's not doing more >>> than >>> looking at the R bit in the sblk. >> >> From the POP I read _all_ orders may indeed return CC=2: case 1 under >> "Conditions precluding Interpretation of the Order Code". >> >> That being said, there are a few more users of smp_sigp (no retry) in >> smp.c (the test, not the lib). >> >> Does it make sense to fix them aswell? > > I thought it made sense to do the lib, since other places expect those > things to "just work." > > But for the tests themselves, I struggle to convince myself with one > path over another. The only way KVM returns a CC2 is because of a > concurrent STOP/RESTART, which isn't a possibility because of the > waiting the lib itself does when invoking the STOP/RESTART. So should > the tests be looking for an unexpected CC2? Or just loop when they > occur? If the latter, shouldn't the lib itself do that? > > I'm happy to make changes, I just can't decide which it should be. Any > opinions? Before we continue bikeshedding, let's add the cc2 retry. If it never returns cc2 we'll never loop on it but the dead code won't kill us either.
diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c index 85b046a5..2e476264 100644 --- a/lib/s390x/smp.c +++ b/lib/s390x/smp.c @@ -85,7 +85,7 @@ bool smp_cpu_stopped(uint16_t idx) bool smp_sense_running_status(uint16_t idx) { - if (smp_sigp(idx, SIGP_SENSE_RUNNING, 0, NULL) != SIGP_CC_STATUS_STORED) + if (smp_sigp_retry(idx, SIGP_SENSE_RUNNING, 0, NULL) != SIGP_CC_STATUS_STORED) return true; /* Status stored condition code is equivalent to cpu not running. */ return false; @@ -169,7 +169,7 @@ static int smp_cpu_restart_nolock(uint16_t idx, struct psw *psw) * running after the restart. */ smp_cpu_stop_nolock(idx, false); - rc = smp_sigp(idx, SIGP_RESTART, 0, NULL); + rc = smp_sigp_retry(idx, SIGP_RESTART, 0, NULL); if (rc) return rc; /*
A SIGP SENSE is used to determine if a CPU is stopped or operating, and thus has a vested interest in ensuring it received a CC0 or CC1, instead of a CC2 (BUSY). But, any order could receive a CC2 response, and is probably ill-equipped to respond to it. In practice, the order is likely to only encounter this when racing with a SIGP STOP (AND STORE STATUS) or SIGP RESTART order, which are unlikely. But, since it's not impossible, let's convert the library calls that issue a SIGP to loop on CC2 so the callers do not need to react to that possible outcome. Signed-off-by: Eric Farman <farman@linux.ibm.com> --- lib/s390x/smp.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)