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

[Ryota Ozaki <ozaki-r%netbsd.org@localhost>]

On Wed, Jan 22, 2020 at 9:22 AM Jason Thorpe <thorpej%me.com@localhost> wrote:
>
>
> > On Jan 20, 2020, at 2:48 PM, Jason Thorpe <thorpej%me.com@localhost> wrote:
> >
> > Folks --
> >
> > The legacy (*if_watchdog)() interface has a couple of problems:
> >
> > 1- It does not have any way to represent multiple hardware-managed transmit queues.
> >
> > 2- It's not easy to make MP-safe because it relies on modifying the ifnet structure periodically outside of the normal locking mechanisms.
> >
> > The wm(4) driver solved the problem in a reasonable way, and to make it easier for the other drivers in the tree to adopt it's strategy, I re-factored it into a new if_txtimer structure and API.
> >
> > So save typing, I'll paste the relevant section of <net/if.h>:
>
> I spent some time thinking about this a little more, especially around making it easier to convert drivers that don't have a periodic tick already (this is mainly drivers that don't use MII), so I added some extra support for such drivers and as a proof of concept, converted the SGI "sq" driver.  You can see that it's a very easy mechanical conversion for legacy drivers, that still gives them a simple NET_MPSAFE migration path (there's a provision for providing a tx queue interlock to the timer expiration callback).
>
> If there is consensus that this is a reasonable direction, then I'll start migrating drivers and, when complete, remove the legacy (*if_watchdog)() and if_timer fields from struct ifnet.

No objection to the direction.  Thank you for the effort!

I have two comments of the API.  See below.

>
> For completeness, here's the big block comment:
>
> /*
>  * if_txtimer --
>  *
>  * Transmission timer (to replace the legacy ifnet watchdog timer).
>  *
>  * The driver should allocate one if_txtimer per hardware-managed
>  * transmission queue.  There are two different ways to allocate
>  * the and use the timer, based on the driver's structure.
>  *
>  * DRIVERS THAT PROVIDE THEIR OWN PERIODIC CALLOUT
>  * ===============================================
>  *
>  * ==> Allocate timers using if_txtimer_alloc().
>  * ==> In the periodic callout, check for txtimer expiration using
>  *     if_txtimer_is_expired() or if_txtimer_is_expired_explicit()
>  *     (see below).
>  *
>  * DRIVERS THAT DO NOT PROVIDE THEIR OWN PERIODIC CALLOUT
>  * ======================================================
>  *
>  * ==> Allocate timers using if_txtimer_alloc_with_callback().
>  *     This variant allocates a callout and provides a facility
>  *     for the callout to invoke a driver-provided callack when
>  *     the timer expires, with an optional interlock (typically
>  *     the transmit queue mutex).
>  *
>  *     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.

>  *     If an interlock is NOT provided, then IPL will be raised to
>  *     splnet() before checking for timer expiration and invoking
>  *     the callback.  In this case, the IPL will be restored on
>  *     the callback's behalf when it returns.
>  * ==> In the driver's (*if_init)() routine, the timer's callout
>  *     should be started with if_txtimer_start().  In the driver's
>  *     (*if_stop)() routine, the timer's callout should be stopped
>  *     with if_txtimer_stop() or if_txtimer_halt() (see callout(9)
>  *     for the difference between stop and halt).

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.

  ozaki-r


>  *
>  * In both cases, all locking of the if_txtimer is the responsibility
>  * of the driver.  The if_txtimer should be protected by the same lock
>  * that protects the associated transmission queue.  The queue
>  * associated with the timer should be locked when arming and disarming
>  * the timer and when checking the timer for expiration.
>  *
>  * When the driver gives packets to the hardware to transmit, it should
>  * arm the timer by calling if_txtimer_arm().  When it is sweeping up
>  * completed transmit jobs, it should disarm the timer by calling
>  * if_txtimer_disarm() if there are no outstanding jobs remaining.
>  *
>  * If a driver needs to check multiple transmission queues, an
>  * optimization is available that avoids repeated calls to fetch
>  * the compare time.  In this case, the driver can get the compare
>  * time by calling if_txtimer_now() and can check for timer expiration
>  * using if_txtimer_is_expired_explicit().
>  *
>  * The granularity of the if_txtimer is 1 second.
>  */
>
>
>
> -- thorpej
>