Re: A new network interface send queue API

[Jason Thorpe <thorpej%me.com@localhost>]

> On Nov 12, 2023, at 6:42 PM, Taylor R Campbell <campbell+netbsd-tech-net%mumble.net@localhost> wrote:
> 
>> Date: Sat, 11 Nov 2023 15:20:56 -0800
>> From: Jason Thorpe <thorpej%me.com@localhost>
>> 
>> To address the above problems, I'm proposing a new interface output
>> queue API that tackles the above points while also being simple
>> to adopt (with a reasonably straight-forward mapping from the legacy
>> API to the new API).  Converting all drivers to this new API will
>> be pretty easy and will help move the needle on the transition to a
>> NET_MPSAFE world.
> 
> Cool, thanks for looking into this!  I've always found the API
> contract for NIC drivers about packet queueing to be hard to follow,
> and it will be great to clean it up.
> 
> Some questions that came up while I skimmed:
> 
> 1. Does this affect the custom if_transmit path at all?

No, the custom if_transmit path is currently unaffected.  At some point in the future, I would like to collapse everything into a single unified solution, but for the moment, I think incremental progress is good.

> 2. What's the impact on altq?

ALTQ will continue to be supported, but all of the ABI impact of ALTQ will be hidden from drivers — all of it is managed inside the new ifq_*() functions.

> 3. Can you draw a state transition diagram?  Just a summary that's a
>   little easier to digest than the prose descriptions.

INVALID (initial state after ifq_init())
|
+-- driver calls ifq_start() --> READY  |
+-- driver calls ifq_fini() --> DEAD  
STOPPED
|
+-- driver calls ifq_start() --> READY  |
+-- driver calls ifq_fini() --> DEAD  
READY
|
+-- if_start_lock() decides to call (*if_start)() --> BUSY
|
+-- driver calls ifq_stop() --> STOPPED

BUSY
|
+-- (*if_start)() drains queue before running out of HW resources -> READY  |
+-- driver Tx intr handler calls ifq_continue() after freeing up HW -> READY
|
+-- driver calls ifq_stop() -> WAIT_STOP

WAIT_STOP (transitions out of WAIT_STOP broadcast on if_stop_cond)
|
+-- (*if_start)() drains queue before running out of HW resources -> STOPPED
|
+-- driver Tx intr handler calls ifq_continue() after freeing up HW -> STOPPED

DEAD (terminal state)


INVALID and STOPPED are really the same, except INVALID exists to ease the transition for drivers (INVALID opts the queue out of the automatic state transitions).  Once all drivers are converted, INVALID can disappear and STOPPED can become the initial state.  I added DEAD this morning (it has the value 0xdeadbeef and is used to make assertions pop if you try to use a zombie ifq).

> 4. Can you show a complete working example of a driver conversion?
>   (The examples you gave look good, but I don't see, e.g., anything
>   that calls ifq_start.)  I guess this will be on the branch?

Yah, I’ll post this on the branch.

> 5. Will this eliminate all open-coded calls to struct ifnet::if_start
>   and replace them by a wrapper function with a well-defined API
>   contract?

This is mostly already true with the if_start_lock() function (which is a horrible name, sigh).  The main thing that needs to happen is drivers need to adopt the deferred-start mechanism to re-start the queue after Tx interrupt processing is complete.  Well, that’s the preferred pattern anyway.  Otherwise, they need to call a function equivalent to if_lock_start(), but I haven’t dealt with that just yet.

> 6. Will this eliminate all access to ifp->if_flags & IFF_RUNNING in
>   drivers' if_start routines?

Yes.  And all access / fiddling with IFF_OACTIVE.

> 7. Maybe ifq_restage should take the original mbuf as another argument
>   to detect and assert no misuse?

Hm, sure, I could look into that.  I don’t think this will be a problem, just need to make sure that some of the drivers currently using IF_PREPEND() aren’t dumpster fires in this regard.

> 8. Any impact on usbnet(4)?  (Guessing no, but maybe it will let us
>   simplify usbnet(4) further?)

usbnet(4) currently calls (*ifp->if_start)(ifp) directly.  I guess it’ll need to be converted it to use deferred-start.

> I probably won't have time to review the whole thing closely for a
> while, but nothing jumped out at me as problematic, so don't take any
> of my questions here as objections and don't let me get in the way of
> working on this!

-- thorpej