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

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

> Date: Fri, 24 Jan 2020 06:57:03 -0800
> From: Jason Thorpe <thorpej%me.com@localhost>
> 
> > On Jan 24, 2020, at 12:27 AM, Taylor R Campbell <campbell+netbsd-tech-net%mumble.net@localhost> wrote:
> > 
> > Does if_txtimer have a way to model multiple queues?  Is it not just
> > driver-specific logic to handle sc->sc_txq[i].txq_lastsent for each
> > queue independently like wm(4)?
> 
> if_txtimer is logically associated with a transmit queue, so you
> would have one if_txtimer per transmit queue.  This is what wm(4)
> does.  (I think I mentioned originally that if_txtimer is
> essentially the wm(4) driver's transmit timer logic wrapped up in a
> small API that makes it easy to adopt in other drivers with minimal
> changes.)

You say if_txtimer is logically associated with a tx queue, but _part_
of the if_txtimer API is never associated with tx queue -- the callout
is, as far as I can see, only ever one-per-driver.  This model strikes
me as confusing: half of what if_txtimer does is per-txqueue, and half
of what it does is per-driver, but the API presents it as one thing --
if_txtimer.

> Perhaps I'm engaged in premature optimization, but I wanted to avoid
> having to use *any* additional locks other than what the driver
> would already be using internally.

I think this is premature optimization because the drivers we're
talking about changing already rely on the kernel lock!

> > Which drivers (a) are for hardware devices that can handle >>1GB/sec,
> > and (b) use if_watchdog -- i.e., which high-throughput drivers would
> > take advantage of the callout-based mechanism inside your proposed
> > if_txtimer?
> 
> dge(4).

Does dge(4) actually attain 10GB/sec using only a single tx queue with
the kernel lock as is?

> My proposal keeps wm(4) at 1 (its own "tick" callout) (because it
> simply encapsulates the wm(4) approach into an API).  Using bare
> callouts to implement the tx watchdog timer would mean wm(4) could
> have up to 17 ("tick" + one callout per hardware tx queue).

Perhaps I wasn't clear -- I didn't mean using one callout per tx
queue; I meant using one `tick' callout per driver.  In other words,
why don't we just convert the drivers that use if_watchdog now to set
up their own tick callout?

No change needed to wm(4), except perhaps the time_uptime expiry
expressions (which as you noted should really use 32-bit atomic
load/store instead of the racy 64-bit time_uptime).

For single-queue drivers currently relying on the kernel lock I
imagine it's perfectly fine to replace ifp->if_timer = 5 by
callout_schedule(sc->sc_ch, 5*hz).  The only other differences from
the status quo are that you would say callout_init(...) instead of
ifp->if_watchdog = ... and you have to call callout_stop/halt in
detach.  Drivers needing to avoid the callout lock, or multi-queue
drivers, can just make the callout tick once per second like wm(4)
does.

What I see in the if_txtimer proposal is that it's supposed to be used
per-queue, EXCEPT for single-queue drivers, in which case it also
serves as another way to spell a tick callout.

But it also has a slightly different variant of the callout API -- and
I see some bugs in the patch you sent (e.g., a disarmed txtimer that
has a callout races with halt), which is part of why I'm suspicious of
adding something that is almost just another way to spell callouts but
with slightly different API requirements.