Re: [PATCH v2 08/10] s390x: smp: Wait for sigp completion

[David Hildenbrand <david@redhat.com>]

On 29.04.20 14:09, Janosch Frank wrote:
> On 4/29/20 1:47 PM, David Hildenbrand wrote:
>> On 29.04.20 13:21, Janosch Frank wrote:
>>> On 4/29/20 11:55 AM, David Hildenbrand wrote:
>>>> On 29.04.20 11:37, Janosch Frank wrote:
>>>>> On 4/29/20 11:06 AM, David Hildenbrand wrote:
>>>>>> On 29.04.20 10:57, Janosch Frank wrote:
>>>>>>> On 4/24/20 1:40 PM, Janosch Frank wrote:
>>>>>>>> On 4/24/20 12:11 PM, David Hildenbrand wrote:
>>>>>>>>> On 23.04.20 11:10, Janosch Frank wrote:
>>>>>>>>>> Sigp orders are not necessarily finished when the processor finished
>>>>>>>>>> the sigp instruction. We need to poll if the order has been finished
>>>>>>>>>> before we continue.
>>>>>>>>>>
>>>>>>>>>> For (re)start and stop we already use sigp sense running and sigp
>>>>>>>>>> sense loops. But we still lack completion checks for stop and store
>>>>>>>>>> status, as well as the cpu resets.
>>>>>>>>>>
>>>>>>>>>> Let's add them.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Janosch Frank <frankja@linux.ibm.com>
>>>>>>>>>> Reviewed-by: Cornelia Huck <cohuck@redhat.com>
>>>>>>>>>> ---
>>>>>>>>>>  lib/s390x/smp.c | 8 ++++++++
>>>>>>>>>>  lib/s390x/smp.h | 1 +
>>>>>>>>>>  s390x/smp.c     | 4 ++++
>>>>>>>>>>  3 files changed, 13 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/lib/s390x/smp.c b/lib/s390x/smp.c
>>>>>>>>>> index 6ef0335..2555bf4 100644
>>>>>>>>>> --- a/lib/s390x/smp.c
>>>>>>>>>> +++ b/lib/s390x/smp.c
>>>>>>>>>> @@ -154,6 +154,14 @@ int smp_cpu_start(uint16_t addr, struct psw psw)
>>>>>>>>>>  	return rc;
>>>>>>>>>>  }
>>>>>>>>>>  
>>>>>>>>>> +void smp_cpu_wait_for_completion(uint16_t addr)
>>>>>>>>>> +{
>>>>>>>>>> +	uint32_t status;
>>>>>>>>>> +
>>>>>>>>>> +	/* Loops when cc == 2, i.e. when the cpu is busy with a sigp order */
>>>>>>>>>> +	sigp_retry(1, SIGP_SENSE, 0, &status);
>>>>>>>>>> +}
>>>>>>>>>> +
>>>>>>>>>>  int smp_cpu_destroy(uint16_t addr)
>>>>>>>>>>  {
>>>>>>>>>>  	struct cpu *cpu;
>>>>>>>>>> diff --git a/lib/s390x/smp.h b/lib/s390x/smp.h
>>>>>>>>>> index ce63a89..a8b98c0 100644
>>>>>>>>>> --- a/lib/s390x/smp.h
>>>>>>>>>> +++ b/lib/s390x/smp.h
>>>>>>>>>> @@ -45,6 +45,7 @@ int smp_cpu_restart(uint16_t addr);
>>>>>>>>>>  int smp_cpu_start(uint16_t addr, struct psw psw);
>>>>>>>>>>  int smp_cpu_stop(uint16_t addr);
>>>>>>>>>>  int smp_cpu_stop_store_status(uint16_t addr);
>>>>>>>>>> +void smp_cpu_wait_for_completion(uint16_t addr);
>>>>>>>>>>  int smp_cpu_destroy(uint16_t addr);
>>>>>>>>>>  int smp_cpu_setup(uint16_t addr, struct psw psw);
>>>>>>>>>>  void smp_teardown(void);
>>>>>>>>>> diff --git a/s390x/smp.c b/s390x/smp.c
>>>>>>>>>> index 7462211..48321f4 100644
>>>>>>>>>> --- a/s390x/smp.c
>>>>>>>>>> +++ b/s390x/smp.c
>>>>>>>>>> @@ -75,6 +75,7 @@ static void test_stop_store_status(void)
>>>>>>>>>>  	lc->prefix_sa = 0;
>>>>>>>>>>  	lc->grs_sa[15] = 0;
>>>>>>>>>>  	smp_cpu_stop_store_status(1);
>>>>>>>>>> +	smp_cpu_wait_for_completion(1);
>>>>>>>>>>  	mb();
>>>>>>>>>>  	report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix");
>>>>>>>>>>  	report(lc->grs_sa[15], "stack");
>>>>>>>>>> @@ -85,6 +86,7 @@ static void test_stop_store_status(void)
>>>>>>>>>>  	lc->prefix_sa = 0;
>>>>>>>>>>  	lc->grs_sa[15] = 0;
>>>>>>>>>>  	smp_cpu_stop_store_status(1);
>>>>>>>>>
>>>>>>>>> Just curious: Would it make sense to add that inside
>>>>>>>>> smp_cpu_stop_store_status() instead?
>>>>>>>>>
>>>>>>>>
>>>>>>>> I think so, we also wait for stop and start to finish, so why not for
>>>>>>>> this order code.
>>>>>>>>
>>>>>>>
>>>>>>> I've moved the waiting into the smp library and now the prefix check for
>>>>>>> stop and store status fails every so often if executed repeatedly.
>>>>>>>
>>>>>>> I've tried making the lc ptr volatile, a print of the prefix before the
>>>>>>> report seems to fix the issue, a print after the report still shows the
>>>>>>> issue but according to the print both values are the same.
>>>>>>>
>>>>>>> I'm currently at a loss...
>>>>>>
>>>>>> Are you missing a barrier() somewhere?
>>>>>>
>>>>>
>>>>> Maybe, but the question is where?
>>>>>
>>>>> There's already one before the report:
>>>>> smp_cpu_stop_store_status(1);
>>>>> mb();
>>>>> report(lc->prefix_sa == (uint32_t)(uintptr_t)cpu->lowcore, "prefix");
>>>>
>>>> The issue here is:
>>>>
>>>> SIGP_SENSE is always handled in the kernel for KVM. Meaning, it will
>>>> complete even before the target CPU executed the stop and store (in QEMU).
>>>>
>>>> Reading the PoP:
>>>>
>>>> "One of the following conditions exists at the
>>>> addressed CPU: ... A previously issued stop-
>>>> and-store-status ... has been accepted by the
>>>> addressed CPU, and execution of the func-
>>>> tion requested by the order has not yet been
>>>> completed.
>>>>
>>>> "If the currently specified order is sense ... then the order
>>>> is rejected, and condition code 2 is set."
>>>>
>>>> So, in case of KVM, SENSE does not wait for completion of the previous
>>>> order. I remember that was a performance improvements, because we wanted
>>>> to avoid going to user space just to sense if another CPU is running.
>>>> (and I remember that the documentation was inconsistent)
>>>
>>> So, KVM is not architectural compliant when it comes to SIGP SENSE?
>>> I guess I need to go back to looping until the prefix is > 0
>>
>> Yeah, or fix SIGP_SENSE in KVM. Would need QEMU and KVM changes. I
>> remember that a tricky part was checking if external calls are pending
>> for a CPU from user space.
>>
>> We could pass that information along with the intercept to QEMU.
>>
>> AFAIKs, SIGP SENSE is not used on a hot path in Linux.
>>
> 
> For now I'd rather have a workaround in the test until I can find cycles
> to find a solution in KVM/QEMU.
> 
> SIGP SENSE has been working quite well for Linux for the last few years,
> so I won't start running around now frantically fixing stuff.

Huh. I thought that's why we have the SMP tests after all ;)


-- 
Thanks,

David / dhildenb