[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 03/01/2019 06:02 AM, Stewart Smith wrote:
>> 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.
>
> Got it.  If we schedule with `zero` we may endup in continuous loop. We need to 
> cross check this:
>    - __check_timers() -> schedule_timer(.., 0) -> insert to top of `timer_list` 
> -> __check_timers() see message expired.
>
>
>> 
>>>> 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.
>
> Got it. Makes sense. Yes. this issue an independent issue. May be we should have 
> separate patch for this.

Yeah, and limiting the time we could spend in OPAL is probably a good
idea anyway.

I'll spin out a separate patch.

-- 
Stewart Smith
OPAL Architect, IBM.