[Skiboot] [RFC PATCH 4/6] core/flash: Make opal_flash_op() actually asynchronous

[Stewart Smith]

Vasant Hegde <hegdevasant at linux.vnet.ibm.com> writes:
> On 02/28/2019 11:48 AM, Stewart Smith wrote:
>> From: Cyril Bur <cyril.bur at au1.ibm.com>
>> 
>
> .../...
>
>
>> -err:
>> +	if (size - len) {
>> +		/* Work remains */
>> +		schedule_timer(&flash->async.poller, 0);
>
> Why are we calling schedule_time with 0?

The idea is to ask linux to get back to us "immediately", so we can
effectively spin reading things from LPC, but still give the kernel time
to do anything critical before calling back into OPAL.

If we delay, then we end up impacting performance of reading from flash.

>> diff --git a/core/timer.c b/core/timer.c
>> index 1c539517839e..c68f978ccb07 100644
>> --- a/core/timer.c
>> +++ b/core/timer.c
>> @@ -199,6 +199,7 @@ static void __check_poll_timers(uint64_t now)
>>   static void __check_timers(uint64_t now)
>>   {
>>   	struct timer *t;
>> +	uint64_t stop = now + msecs_to_tb(50); /* Run timers for max 5ms */
>
> I'm not sure why we need this change. Can you please elaborate?

it should possibly be a separate commit. The idea was to throw some
warnings if we spend too long running timers, as well as avoid going
around in a loop inside OPAL when we have a timer scheduled
"immediately".

Otherwise, if we schedule something 0 tb ticks away (or close enough
that we'd hit it by the time we get to the end of the timer list), we
just spend ever increasing time in OPAL without giving the kernel a
chance to do something.

>>   	for (;;) {
>>   		t = list_top(&timer_list, struct timer, link);
>> @@ -229,6 +230,12 @@ static void __check_timers(uint64_t now)
>> 
>>   		/* Update time stamp */
>>   		now = mftb();
>> +
>> +		/* Only run timers for a limited time to avoid jitter */
>> +		if (now > stop) {
>> +			prlog(PR_PRINTF, "Run timers for > 50ms\n");
>> +			break;
>> +		}
>>   	}
>>   }
>
> -Vasant
>

-- 
Stewart Smith
OPAL Architect, IBM.