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

[Taylor R Campbell <campbell+netbsd-tech-net%mumble.net@localhost>]

I don't really understand why you're proposing a new API.  But let me
make sure I understand the context first.

When we submit a packet to a hardware queue for tx, we want to wait up
to n seconds before giving up -- meaning that we report the packet
dropped (if_oerrors++), reset the device, free the packet, recycle
queue entries, whatever.

Right now, we have a mechanism if_watchdog (a.k.a. if_slowtimo) which
works as follows, for drivers that use it, like wpi(4):  Every second,
if ifp->if_timer is nonzero, we decrement it, and if it becomes zero,
we call ifp->if_watchdog -- all under the kernel lock.

- Some drivers like sk(4) set ifp->if_timer = 5 and use if_watchdog to
  give up after 5sec.

- Some drivers like wpi(4) set ifp->if_timer = 1, sc->sc_tx_timer = 5,
  and use if_watchdog to count down sc->sc_tx_timer every second.
  That way they can give up after 5sec but also do other housekeeping
  like ieee80211_watchdog every second.

- Some drivers like wm(4) use a callout instead of if_watchdog.

- Some drivers like wm(4) maintain one timer for each tx queue.


The _main_ problem you're trying to solve, as I understand it, is that
management of ifp->if_timer -- in net/if.c but also scattered
throughout drivers all over the tree -- is open-coded and relies on
the kernel lock, which you would like to avoid (but please correct me
if I have misunderstood):

> > On Jan 20, 2020, at 2:48 PM, Jason Thorpe <thorpej%me.com@localhost> wrote:
> > 
> > 2- It's not easy to make MP-safe because it relies on modifying
> > the ifnet structure periodically outside of the normal locking
> > mechanisms.

Presumably we could add a new lock for access to if_timer, and that
would solve this problem.  But it seems to me it would be even simpler
to just use a callout to solve this:

- Where a driver currently sets ifp->if_timer = 5, it can instead
  callout_schedule(sc->sc_ch, 5).

- Where a driver currently sets ifp->if_timer = 1 and also uses
  sc->sc_tx_timer, it can instead callout_schedule(sc->sc_ch, 1) and
  continue to use sc->sc_tx_timer under its own lock.

- Where a driver currently sets ifp->if_timer = 0, it can
  callout_stop(sc->sc_ch), provided, of course, that MP-safe drivers
  also callout_halt(sc->sc_ch) before callout_destroy(sc->sc_ch) in
  detach and before anything else relies on the callout not to be
  running.

- Where a driver currently queries ifp->if_timer == 0 to decide
  whether to reschedule it, it can do (under the driver lock):

	if (!callout_pending(sc->sc_ch) && !callout_invoking(sc->sc_ch))
		callout_schedule(sc->sc_ch, ...);

Why do we need anything more than a callout?


You stated a second goal first:

> > 1- It does not have any way to represent multiple hardware-managed
> > transmit queues.

But I don't really see why adding a new txtimer API helps any more
than a callout.  Does it actually reduce any duplicated logic that
could meaningfully be shared between, e.g., wm(4) and another
interface with multiple tx queues?


I'm especially skeptical of an API that, by dynamic conditionals,
may combine

(a) a simple countdown timer (if_txtimer),
and
(b) a replica of the callout API using dynamic allocation but with an
interlock determined up front and other slight changes and that has an
embedded simple countdown timer with a boolean indicating that it's
actually the more complex thing (if_txtimer_callout),

without common logic that is meaningfully agnostic to which case it's
working with.  For example, will any driver call if_txtimer_start
without actually knowing up front whether it needs to use the callout?
Will any driver call if_txtimer_stop in a code path that would not be
adequately served either by if_txtimer_disarm, or by an unconditional
if_txtimer_disarm & callout_stop?