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

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

> Date: Thu, 23 Jan 2020 22:37:55 -0800
> From: Jason Thorpe <thorpej%me.com@localhost>
> 
> There are a 3 basic problems:
> 
> 1- if_timer relies on the KERNEL_LOCK.

Agreed.

> 2- if_timer does not have a way to model multiple queues.

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)?

> 3- There should be one basic way to modeling the transmit watchdog
> timer, rather than N.  (In the examples you cited above, N is
> definitely > 1.)

If we just replaced if_slowtimo by an explicit callout in each driver
just like wm_tick, would you still consider that to be >1 model?

> Callouts are a bit more expensive... to arm them or introspect them
> requires taking an additional lock **shared by other callouts** that
> could be a source of lock contention;

The lock is normally the per-CPU lock of the CPU where it was last
scheduled, so that should be pretty cheap unless something is going
seriously wrong with callouts and/or the driver.

> This could make a difference when you're trying to blast out packets
> at 40Gb/s.

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?

> Callouts also have a larger memory footprint ... not hugely so, mind
> you, but each one is 10 pointers large; my proposal uses a bit less
> memory than callouts... nearly a wash if you consider the fields
> that can be removed from struct ifnet when the conversion of every
> driver is complete.

If we break the drivers into two classes:

1. Those that currently use if_watchdog.
   - Status quo: There already is exactly 1 callout per device.
   - With callouts: There would still be exactly 1 callout per device,
     but in struct foo_softc instead of struct ifnet.
   - With if_txtimer: There would still be exactly 1 callout per
     device, but in struct if_txtimer instead of struct ifnet.

2. Those that currently do not use if_watchdog and do their own thing.
   - Status quo: There already are (1 + n) callouts per device, where
     n is however many the driver allocates to do its own thing.
   - With callouts: There would be n callouts per device because we
     get rid of if_slowtimo.
   - With if_txtimer: There would be n callouts per device because we
     get rid of if_slowtimo.

I don't see a difference.  Am I missing something?

> Tx transmit timers have no need for precision ... 1 second
> granularity is totally sufficient, and many drivers are already
> using a 1-second tick timer for link maintenance / stats updating;
> my proposal piggy-backs on that.  The cost is that you do some work
> once a second.

I don't understand how this makes a difference between just using
callouts directly, and hiding callouts in an API that uses them on
request.  Can you elaborate?

> The intent of if_txtimer_callout is to essentially provide a the
> callout-backed "tick" facility to drive if_txtimer to drivers that
> wouldn't otherwise need that facility for their own purposes.  It's
> a set of convenience functions to reduce code duplication.

Can you show what code is deduplicated this way?

The diff has +102 -82, even if I ignore the additions in net/if.h and
net/if.c.  Some + is to be expected by pushing stuff from struct ifnet
optionally into struct foo_softc, but I read through the patch and I
don't see anything that looks plausibly like deduplication, other than
perhaps the expression in if_txtimer_expired_explicit in wm(4).  The
comment and whitespace density seems to be about the same before and
after, so that seems like it's increasing the amount of code _and_ the
API complexity.

> >  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?
> 
> The rules are pretty simple:

My questions weren't about the rules for use.  My questions were:

1. Will any driver call if_txtimer_start without actually knowing up
   front whether it needs to use the callout?

2. 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?

That is, will there be any code paths which meaningfully take
advantage of having a run-time conditional?  If yes, can you give an
example?  If no, why do we need a replica of the callout API spelled
differently in if_txtimer?

In the diff you showed, the answers to both questions are no: wm(4)
and pcn(4) _know_ they don't use the if_txtimer callout, so they don't
need if_txtimer_start/stop at all; sq(4) _knows_ it uses the
if_txtimer callout, so it could just as well use callout_schedule and
callout_stop instead of if_txtimer_start/stop.

This suggests to me that the if_txtimer API (at least, the version
with optional callouts) is confounding two different purposes and
would make it harder, not easier, to understand the drivers -- it uses
one name, but there are really two disparate usage models.