On Thu, 2 Apr 2020 16:47:44 +0200
Christian Borntraeger <borntraeger@xxxxxxxxxx> wrote:
> On 02.04.20 15:41, Janosch Frank wrote:
> > On 4/2/20 2:29 PM, Christian Borntraeger wrote:
> >>
> >>
> >> On 02.04.20 14:18, Janosch Frank wrote:
> >>> On 4/2/20 1:02 PM, Christian Borntraeger wrote:
> >>>> make sure that sigp sense running status returns a sane value for
> >>>
> >>> s/m/M/
> >>>
> >>>> stopped CPUs. To avoid potential races with the stop being processed we
> >>>> wait until sense running status is first 0.
> >>>
> >>> ENOPARSE "...is first 0?"
> >>
> >> Yes, what about "....smp_sense_running_status returns false." ?
> >
> > sure, or "returns 0"
> > "is first 0" just doesn't parse :)
> >
> >>
> >>>
> >>>>
> >>>> Signed-off-by: Christian Borntraeger <borntraeger@xxxxxxxxxx>
> >>>> ---
> >>>> lib/s390x/smp.c | 2 +-
> >>>> lib/s390x/smp.h | 2 +-
> >>>> s390x/smp.c | 13 +++++++++++++
> >>>> 3 files changed, 15 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
> >>>> index 5ed8b7b..492cb05 100644
> >>>> --- a/lib/s390x/smp.c
> >>>> +++ b/lib/s390x/smp.c
> >>>> @@ -58,7 +58,7 @@ bool smp_cpu_stopped(uint16_t addr)
> >>>> return !!(status & (SIGP_STATUS_CHECK_STOP|SIGP_STATUS_STOPPED));
> >>>> }
> >>>>
> >>>> -bool smp_cpu_running(uint16_t addr)
> >>>> +bool smp_sense_running_status(uint16_t addr)
> >>>> {
> >>>> if (sigp(addr, SIGP_SENSE_RUNNING, 0, NULL) != SIGP_CC_STATUS_STORED)
> >>>> return true;
> >>>> diff --git a/lib/s390x/smp.h b/lib/s390x/smp.h
> >>>> index a8b98c0..639ec92 100644
> >>>> --- a/lib/s390x/smp.h
> >>>> +++ b/lib/s390x/smp.h
> >>>> @@ -40,7 +40,7 @@ struct cpu_status {
> >>>> int smp_query_num_cpus(void);
> >>>> struct cpu *smp_cpu_from_addr(uint16_t addr);
> >>>> bool smp_cpu_stopped(uint16_t addr);
> >>>> -bool smp_cpu_running(uint16_t addr);
> >>>> +bool smp_sense_running_status(uint16_t addr);
> >>>
> >>> That's completely unrelated to the test
> >>
> >> Right but this name seems to better reflect what the function does. Because this is not
> >> the oppositite of cpu_stopped.
> >
> > I'm pondering if we want to split that out.
>
> A single patch for just 2 lines? I dont know.
I vote for keeping it in the patch and simply mentioning it in the
commit message.
> >
> >>>
> >>>> int smp_cpu_restart(uint16_t addr);
> >>>> int smp_cpu_start(uint16_t addr, struct psw psw);
> >>>> int smp_cpu_stop(uint16_t addr);
> >>>> diff --git a/s390x/smp.c b/s390x/smp.c
> >>>> index 79cdc1f..b4b1ff2 100644
> >>>> --- a/s390x/smp.c
> >>>> +++ b/s390x/smp.c
> >>>> @@ -210,6 +210,18 @@ static void test_emcall(void)
> >>>> report_prefix_pop();
> >>>> }
> >>>>
> >>>> +static void test_sense_running(void)
> >>>> +{
> >>>> + report_prefix_push("sense_running");
> >>>> + /* make sure CPU is stopped */
> >>>> + smp_cpu_stop(1);
> >>>> + /* wait for stop to succeed. */
> >>>> + while(smp_sense_running_status(1));
> >>>> + report(!smp_sense_running_status(1), "CPU1 sense claims not running");
> >>>
> >>> That's basically true anyway after the loop, no?
> >>
> >> Yes, but you get no "positive" message in the more verbose output variants
> >> without a report statement.
> >
> > report(true, "CPU1 sense claims not running");
> > That's also possible, but I leave that up to you.
>
> I do not care, both variants are fine. Whatever you or David prefer.
I'd keep the 'check' for !smp_sense_running_status(1) and add a comment.
