[Skiboot] [PATCH v7 2/2] SBE: Add timer support

[Benjamin Herrenschmidt]

On Tue, 2018-04-17 at 12:31 +0530, Vasant Hegde wrote:
> 
> > Again, why do you call that here ?
> > 
> > You just called p9_sbe_timeout_poll_one() for that same SBE, which
> > already took the lock *and* called __p9_sbe_interrupt.
> > 
> > I would instead of a call-out from p9_sbe_timeout_poll_one() to check
> > timers.
> 
> Yeah. We can move p9_sbe_timeout_poll inside p9_sbe_timeout_poll_one(). That way 
> we can avoid lock-unlock. But we still need to call __p9_sbe_interrupt() twice.. 
> as timer interrupt will come after removing msg from sbe->msg_list.

I'm not sure I understand why we need to call it twice, worst cast, if
the timer was really very very short, we'll just hit it on the next
call to poll no ? Esp. timeouts, we don't need to be paricularly
precise.

   .../...

+ * This is called with the timer lock held, so there is no
> > > + * issue with re-entrancy or concurrence
> > > + */
> > > +void p9_sbe_update_timer_expiry(uint64_t new_target)
> > > +{
> > > +	u64 now = mftb();
> > > +
> > > +	if (!sbe_has_timer || new_target == sbe_timer_target)
> > > +		return;
> > > +
> > > +	lock(&sbe_timer_lock);
> > > +	/* Timer message is in flight. Record new timer and schedule later */
> > > +	if (p9_sbe_msg_busy(timer_ctrl_msg) || has_new_target) {
> > > +		if (new_target < now)
> > > +			goto out;
> > 
> > Will the caller handle the case where we just passed the target ? Will
> > it re-check timers after calling us ?
> 
> Sorry. I'm not sure I understood your question.
> 
> So we enter this block only when new_target is < inflight timer. So we are good 
> in source.

How so ? We re-snapshot "now", meaning we could have moved beyond
"new_target" already. If that happens, we will hit your "goto out"
above and not program any timer.

Unless the caller checks, which it doesn't ...

> Now on callback path if SBE triggers early we have problem (..as check_timer 
> doesn't re-arm).
> 
> May be fix check_timer to re-arm timer? -OR- do you want me to take care in 
> driver itself?

We should be smarter inside __check_timers(). We should probably pass
down the "from_interrupt" argument from check_timers() and handle that
case, along with the t->running case which currently will cause us to
delay until the next timer interrupt, which isn't a great idea, so we
should probably reconfigure a short delay or something...

> We can take care here by doing something like below in p9_sbe_interrupt () function.
>          if (got_timer_interrupt) {
>                  got_timer_interrupt = false;
>                  /* Drop lock and call timers */
>                  unlock(&sbe->lock);
>                  check_timers(true);
> 
> 		/* Early interrupt , re-arm timer */
> 		if (sbe_last_gen_timestamp > mftb())
> 			p9_sbe_update_timer_expiry(sbe_last_gen_timestamp);
> 
> 	}
> 
> 
> -Vasant
>