iflib: invert default restart on VLAN changes

[kbowling]

Summary

In rS360398, a new iflib device method was added to opt out of VLAN events needing an interface reset.

I am switching the default for VLAN and unknown events to not requiring a restart. After fixing various bugs, I do not think this would be a common need of hardware and it is undesirable from the user's perspective causing link flaps and much slower VLAN configuration. Currently, there are no other restart events besides VLAN events, and setting the ifdi_needs_restart default to false will alleviate the need to churn every driver if an odd event is added in the future for specific hardware.

No functional change, do not restart on VLAN change for:

• em (fixed in 2796f7c)
• igc (fixed in 2eaef8e)
• ix (implemented in cf15091)
• ixl (implemented in cf15091)

No function change, continue to restart on VLAN change for:

• ixv (needs code audit, 61a8231 fixed principal issue; re-init probably not necessary)
• axgbe (needs code audit; re-init probably not necessary)
• iavf - (needs code audit; interaction with Malicious Driver Detection mentioned in rS360398)
• mgb - no VLAN functions are currently implemented. Left a comment.

Functional change to:

• bnxt - it was an unintended side effect of rS360398 that this driver is doing re-init for VLAN changes. Toggle it off.
• enic - it was an unintended side effect of rS360398 that this driver is doing re-init for VLAN changes. Toggle it off.
• enetc - it was an unintended side effect of rS360398 that this driver is doing re-init for VLAN changes. Toggle it off.
• vmxnet3 - it was an unintended side effect of rS360398 that this driver is doing re-init for VLAN changes. Toggle it off.
• ice - it doesn't look like it needs to re-init for VLAN changes. Toggle it off.

[kbowling]

sys/dev/bnxt/if_bnxt.c

2464

@markj one other thought, are these SLISTs safe to use like this? I'm not sure about any reentrancy into the driver but may be related to https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=222680

A proposal would be to use CK_SLIST if you think that would be ok.

[kp]

I've only done basic testing, but this does appear to work. I can now create if_vlan devices on top of my bnxt0 interface, and receive VLAN tagged traffic as expected. I can also delete the interface again and not lose connectivity.

[markj]

sys/dev/bnxt/if_bnxt.c

2464

I don't see any particular problems with the way this list is accessed. We are assuming that callers are serialized somehow, SLIST doesn't support concurrent modifications of a list, nor does CK_SLIST. And it looks like all the functions which modify the list are called with iflib's ctx lock held.

[kbowling]

Sweep the drivers

[markj]

setting the ifdi_needs_restart default to false will alleviate the need to churn every driver if an odd event is added in the future for specific hardware.

Returning true by default seems like the safe default to me. Yes, unneeded reinits are annoying, but if we don't reinit when an event actually requires it, the result could be harder to debug.

sys/dev/vmware/vmxnet3/if_vmx.c

2516 ↗

(On Diff #126450)

If you think that this is unnecessary, it'd be nice to include a comment stating that. Same for other drivers which continue to return true in this path.

[kbowling]

Don't toggle vmxnet3

[kbowling]

In D41558#947392, @markj wrote:

setting the ifdi_needs_restart default to false will alleviate the need to churn every driver if an odd event is added in the future for specific hardware.

Returning true by default seems like the safe default to me. Yes, unneeded reinits are annoying, but if we don't reinit when an event actually requires it, the result could be harder to debug.

Can you give some examples of future events where that would make sense? I would analogize this to the quirks in the I/O subsystem.. they should be opt in because they are unlikely to be needed by any future devices and if they are needed you really need to know and specify why because it is a bug in the driver or hardware.

sys/dev/vmware/vmxnet3/if_vmx.c

2516 ↗

(On Diff #126450)

I was thinking vmxnet3_reinit_rxfilters() might need to be called elsewhere but after looking again with more rest it seems IFLIB_RESTART_VLAN_CONFIG can return false for this driver. Toggling the IFCAP_VLAN_HWFILTER would reset the interface inside iflib_if_ioctl() so vmxnet3_reinit_rxfilters() gets called as needed.

[markj]

In D41558#947462, @kbowling wrote:

In D41558#947392, @markj wrote:

setting the ifdi_needs_restart default to false will alleviate the need to churn every driver if an odd event is added in the future for specific hardware.

Returning true by default seems like the safe default to me. Yes, unneeded reinits are annoying, but if we don't reinit when an event actually requires it, the result could be harder to debug.

Can you give some examples of future events where that would make sense? I would analogize this to the quirks in the I/O subsystem.. they should be opt in because they are unlikely to be needed by any future devices and if they are needed you really need to know and specify why because it is a bug in the driver or hardware.

I don't have a specific example in mind. A somewhat contrived one: suppose we had an iflib driver which could somehow handle a SIOCSIFMTU without needing a reinit. Then, to implement that we'd need to update all iflib drivers, because the existing ones really do need a reinit.

I don't really object to the diff, I just want to point out that the current default just seems safer. Either way I can imagine scenarios where adding some new functionality to iflib would require updating all existing drivers, so avoiding churn doesn't seem like a strong argument. Really you kind of want an iflib method which tells you both whether the driver handles an event at all, and if so, whether that event requires a reinit, both defaulting to false. But it's hard to generalize without more examples.