Re: if_txtimer API to replace (*if_watchdog)()

[Andrew Doran <ad%netbsd.org@localhost>]

On Thu, Jan 23, 2020 at 06:28:39AM -0800, Jason Thorpe wrote:
> +ad@ +riastradh@
> 
> > On Jan 22, 2020, at 10:56 PM, Ryota Ozaki <ozaki-r%netbsd.org@localhost> wrote:
> > 
> >> *
> >> *     If an interlock is provided, the interlock will be acquired
> >> *     before checking for timer expiration, and will invoke the
> >> *     callback with the interlock held if the timer has expired.
> >> *     NOTE: the callback is responsible for releasing the interlock.
> > 
> > Why?  I think we should keep lock/unlock in the same place,
> > at least in the same layer, as much as possible unless there is
> > a critical reason to do so.
> 
> Andrew / Taylor, would appreciate you weighing in on this, as well.
> 
> I went back-and-forth on this point.  My thinking there is:
> 
> a) The interlock must be taken in order to test for timer expiration.
> 
> b) The error handling in the callback will need to drop the lock if it
> decides to reset the interface.

How about permitting that but require that the lock is taken again before
return back to if_txtimer_tick().  Would it be a big pessimisation?  If
you're resetting the interface I guess not.

> c) If I drop the lock in if_txtimer_tick() before calling the callback,
> then the state of the timer will be indeterminate when the callback is
> called; the callback would have to reacquire the lock and re-check for
> timer expiration.
> 
> This is why I settled on this asymmetry.  I would add a KASSERT() to check
> the lock state on return from the callback, but I don't think it's
> possible to safely check mutex_owned() on a SPIN mutex.  Andrew, can you
> comment on this?

With spin mutexes you can only do a positive check, i.e.  "I know I should
own this right now and want to check that" because for the negative case
some other CPU could own it, or mutex_owned() could be lying to you because
the kernel was compiled without options MULTIPROCESSOR.
 
> > We need to teach if_txtimer_tick not to call callout_schedule
> > somehow on destruction, otherwise if_txtimer_tick can continue
> > to run even after calling callout_halt.  wm avoids the flaw by
> > checking sc_core_stopping at the beginning of wm_tick.
> 
> This is a general problem in all drivers that use callouts, and
> if_txtimer_tick() doesn't really make it any worse than existing drivers
> that use tick callouts themselves.  I think even wm(4)'s approach has a
> problem in the detach case.  I have some thoughts on how to solve this
> problem generally, and will attack this problem at a future time.
> 
> -- thorpej
>