Track whether iflib queue mappings may still be accessed by the device. Keep the state private to iflib and conservative: an unknown or failed device must pass through IFDI_STOP() before mappings are reused or released, while a device known to be stopped need not receive another hardware stop. Enter the starting state before IFDI_INIT(), publish running only after receive buffers and framework state are ready, and stop hardware if receive-buffer setup fails after driver initialization. Do not initialize an administratively-down interface merely because its MTU, capabilities, VLAN configuration, or media changed. Preserve successful retries for an administratively-up interface whose previous initialization failed. This state describes ownership of iflib datapath mappings only. It deliberately makes no claim about firmware queues, administrative DMA, PCI power state, or whether a driver can safely elide a hardware reset. MFC after: 2 weeks
Details
Details
- Reviewers
gallatin nick_spun.io shurd - Group Reviewers
iflib
Diff Detail
Diff Detail
- Repository
- rG FreeBSD src repository
- Lint
Lint Skipped - Unit
Tests Skipped
Event Timeline
Comment Actions
Some background on this change is perhaps worth sharing.. I was originally chasing the up/down/up dance on boot, seeing if I could improve boot speed by carefully driving initial ifnet status through a state machine. But the majority of the time is spent in PHY code so doing something careful in iflib only shaved off xx-xxx milliseconds. Therefore the initial investigation is more of a driver state machine problem, and ice(4) has a good example of doing link state well. This ends up closing a few gaps anyway so here is just the lifecycle relevant part, and it lends naturally to the PM state machine that is a child of this review.
| sys/net/iflib.c | ||
|---|---|---|
| 4681–4685 | Maybe a dumb question, but if you're tracking the state, why do you need this complexity? Eg, why can't you check ctx->ifc_datapath_state != IFLIB_DP_STOPPED? | |
| sys/net/iflib.c | ||
|---|---|---|
| 4681–4685 | Not dumb, I updated the iflib_stop path to use the state. But I'm not sure if you are referring to the restart bool overall? | |
| sys/net/iflib.c | ||
|---|---|---|
| 4681–4685 | Yes.. if (bits & IFF_DRV_RUNNING) != 0 then ctx->ifc_datapath_state == IFLIB_DP_RUNNING, right? Or can we get to IFF_DRV_RUNNING without IFLIB_DP_RUNNING? More generally, how does IFF_DRV_RUNNING interact with IFLIB_DP_RUNNING? It feels like this thing is a basically adding extra IFF_DRV* states but the relationship between the states is not super clear to me. Can we add a few others to this review as well? | |
| sys/net/iflib.c | ||
|---|---|---|
| 4681–4685 |
The if_drv_flags and its IFF_DRV_* bits was very early SMPng idea from Robert Watson. The idea was that certain word in ifnet is shared between the driver and the stack: the driver will write to it and the stack will only check it and will avoid sending packets if IFF_DRV_RUNNING is not set. However, no synchronization was provided. IMHO, today it is clear that idea was not great. By modern standards a driver shall expect packets arriving on its if_transmit until it is fully detached. If hardware is not running, it is drivers responsibility to free a packet. IMHO, any new code shall avoid using if_drv_flags. If some driver uses it for internal purposes, e.g. both writing and checking it, then it can just move the flag to its own softc. | |
Comment Actions
Thank you for the clarification and history. I've updated this to remove any dependency on IFF_DRV_* and make this new private state authoritative. This lays groundwork for DMA fencing I am working on; I can chain a broader sweeping of IFF_DRV_* in iflib and its drivers if this is looking like the right direction.