Re: [PATCH v2] powerpc/pseries/vas: Use usleep_range() to support HCALL delay

[Haren Myneni <haren@linux.ibm.com>]

On 11/29/23 6:07 PM, Michael Ellerman wrote:
> Haren Myneni <haren@linux.ibm.com> writes:
>> VAS allocate, modify and deallocate HCALLs returns
>> H_LONG_BUSY_ORDER_1_MSEC or H_LONG_BUSY_ORDER_10_MSEC for busy
>> delay and expects OS to reissue HCALL after that delay. But using
>> msleep() will often sleep at least 20 msecs even though the
>> hypervisor expects to reissue these HCALLs after 1 or 10msecs.
>> It might cause these HCALLs takes longer when multiple threads
>> issue open or close VAS windows simultaneously.
>>
>> So instead of msleep(), use usleep_range() to ensure sleep with
>> the expected value before issuing HCALL again.
>>
>> Signed-off-by: Haren Myneni <haren@linux.ibm.com>
>> Suggested-by: Nathan Lynch <nathanl@linux.ibm.com>
>>
>> ---
>> v1 -> v2:
>> - Use usleep_range instead of using RTAS sleep routine as
>>    suggested by Nathan
>> ---
>>   arch/powerpc/platforms/pseries/vas.c | 24 +++++++++++++++++++++++-
>>   1 file changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/powerpc/platforms/pseries/vas.c b/arch/powerpc/platforms/pseries/vas.c
>> index 71d52a670d95..bade4402741f 100644
>> --- a/arch/powerpc/platforms/pseries/vas.c
>> +++ b/arch/powerpc/platforms/pseries/vas.c
>> @@ -36,9 +36,31 @@ static bool migration_in_progress;
>>   
>>   static long hcall_return_busy_check(long rc)
>>   {
>> +	unsigned int ms;
>> +
>>   	/* Check if we are stalled for some time */
>>   	if (H_IS_LONG_BUSY(rc)) {
>> -		msleep(get_longbusy_msecs(rc));
>> +		ms = get_longbusy_msecs(rc);
>> +		/*
>> +		 * Allocate, Modify and Deallocate HCALLs returns
>> +		 * H_LONG_BUSY_ORDER_1_MSEC or H_LONG_BUSY_ORDER_10_MSEC
>> +		 * for the long delay. So the delay should always be 1
>> +		 * or 10msecs, but sleeps 1msec in case if the long
>> +		 * delay is > H_LONG_BUSY_ORDER_10_MSEC.
>> +		 */
>> +		if (ms > 10)
>> +			ms = 1;
>   
> I don't understand this. The hypervisor asked you to sleep for more than
> 10 milliseconds, so instead you sleep for 1?
> 
> I can understand that we don't want to usleep() for the longer durations
> that could be returned, but so shouldn't the code be using msleep() for
> those values?
> 
> Sleeping for a very short duration definitely seems wrong.

Allocate, modify and deallocate HCALLs return only 1MSECS and 10MSECS 
for long delay. we should not expect > 10MSECS for these HCALLs. Hence 
ms = 1 if ms > 10

But it is confusing. So will use ms = 10 for ms >= 10 as Nathan suggested.

> 
> 
>> +		/*
>> +		 * msleep() will often sleep at least 20 msecs even
>> +		 * though the hypervisor expects to reissue these
>   
> That makes it sound like the hypervisor is reissuing the hcalls.
> 
> Better would be "the hypervisor suggests the kernel should reissue the
> hcall after ...".
> 
>> +		 * HCALLs after 1 or 10msecs. So use usleep_range()
>> +		 * to sleep with the expected value.
>> +		 *
>> +		 * See Documentation/timers/timers-howto.rst on using
>> +		 * the value range in usleep_range().
>> +		 */
>> +		usleep_range(ms * 100, ms * 1000);
> 
> If ms == 1, then that's 100 usecs, which is not 1 millisecond?
> 
> Please use USEC_PER_MSEC.

Using usleep_range() same way as mentioned in  rtas_busy_delay().


Thanks
Haren

> 
>>   		rc = H_BUSY;
>>   	} else if (rc == H_BUSY) {
>>   		cond_resched();
> 
> cheers
>