Page MenuHomeFreeBSD

Make options IFLIB a real configuration option.
AcceptedPublic

Authored by sbruno on Jul 6 2018, 1:46 PM.

Details

Summary

Since IFLIB is required for e1000, ixgbe, bnxt and ixl, make it
dependant on those drivers. This allows little artisinal builds to exclude
iflib and these drivers, shrinking their builds several 10s of kilobytes.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 17889
Build 17649: arc lint + arc unit

Event Timeline

sbruno created this revision.Jul 6 2018, 1:46 PM

Does this not also make it so that these drivers require IFLIB, so they need to be marked as such in GENERIC/NOTES/etc and man pages?

sbruno added a comment.Jul 7 2018, 4:44 PM

Does this not also make it so that these drivers require IFLIB, so they need to be marked as such in GENERIC/NOTES/etc and man pages?

Yeah, definitely. It also means that out of tree drivers (intel, bnxt) wouldn't work if a kernel is built without IFLIB. Is this ok to do?

Does this not also make it so that these drivers require IFLIB, so they need to be marked as such in GENERIC/NOTES/etc and man pages?

Yeah, definitely. It also means that out of tree drivers (intel, bnxt) wouldn't work if a kernel is built without IFLIB. Is this ok to do?

I should think it is ok to do, just requires some additional documentation. And that brings up the question, can iflib.ko be created and then the driver modules depend on that? This may be getting larger in scope than 1 review/1 commit though.

erj added a comment.Jul 7 2018, 5:22 PM

can iflib.ko be created and then the driver modules depend on that?

That would be really nice. :-P

cem added a subscriber: cem.EditedJul 7 2018, 9:15 PM
In D16164#342951, @erj wrote:

can iflib.ko be created and then the driver modules depend on that?

That would be really nice. :-P

Driver modules already can and do depend on iflib, without forcing it to be a kld object (.ko) — grep for MODULE_DEPEND(..., iflib, ...).

If the idea is pulling iflib out of the kernel just for space savings, I'm not sure that really makes sense on any platform that supports iflib drivers anyway (i.e., non-embedded).

Please do add the IFLIB option to most/all GENERICs simultaneously with this change, though.

sbruno added a comment.Jul 8 2018, 2:57 PM
In D16164#342962, @cem wrote:
In D16164#342951, @erj wrote:

can iflib.ko be created and then the driver modules depend on that?

That would be really nice. :-P

Driver modules already can and do depend on iflib, without forcing it to be a kld object (.ko) — grep for MODULE_DEPEND(..., iflib, ...).
If the idea is pulling iflib out of the kernel just for space savings, I'm not sure that really makes sense on any platform that supports iflib drivers anyway (i.e., non-embedded).
Please do add the IFLIB option to most/all GENERICs simultaneously with this change, though.

I don't have a strong objection to adding options IFLIB to all the kernels, but isn't that redundant? I've declared and explicit dependency here for the device drivers that need it.

mmacy accepted this revision.Aug 13 2018, 9:20 PM
This revision is now accepted and ready to land.Aug 13 2018, 9:20 PM

@shurd any objections from the peanut gallery?

trivial bump to generate email.

bump to see if phabricator shovels some emails.

shurd accepted this revision.Jan 25 2019, 6:50 PM

No objections, though I do agree with cem@ in principle regarding putting it in the GENERICs. When 3rd-party drivers start relying on it, it would violate POLA removing Intel NICs from the kernel would break a 3rd-party driver. For now it's likely "fine".

cem added a comment.Jan 25 2019, 7:28 PM

To elaborate a bit more, the stated commit log:

Since IFLIB is required for e1000, ixgbe, bnxt and ixl, make it
dependant on those drivers.

is sort of the opposite of the actual change. The change made here is to make iflib optional and those drivers depend on the iflib option.

If you enable 'device ix' but don't enable option 'iflib', you won't get an ixgbe driver.

You could instead change the iflib files (net/ifdi_if.m, net/iflib.c, net/iflib_clone.c, net/mp_ring.c) to be optional ether pci ix | ether pci ixv | ether pci ixl | ether pci ixlv | ether pci em, which would do something more like what the commit message suggests, but obviously that is ugly and only becomes more ugly as other iflib drivers are added.

In D16164#405230, @cem wrote:

To elaborate a bit more, the stated commit log:

Since IFLIB is required for e1000, ixgbe, bnxt and ixl, make it
dependant on those drivers.

is sort of the opposite of the actual change. The change made here is to make iflib optional and those drivers depend on the iflib option.

...
Ouch, that is going to cause some people a bit of head scratching. What if we go the route that was done for mii, simply group these drivers togeather in GENERIC under a comment #These NICs need option iflib and remove the complex and hard to maintain proper files entry that cem@ describes.