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.
>
>>>
>>>> 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.
