Page MenuHomeFreeBSD

Make options IFLIB a real configuration option.
AbandonedPublic

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.

erj resigned from this revision.Sep 24 2019, 5:28 PM
imp added a comment.Sep 24 2019, 5:33 PM

I'm struggling to understand here. How does this make it a real option? It seems to say that you must include iflib to get these drivers and there will be no warning if you don't. Maybe I'm misunderstanding something here, but that strikes me as unwise on its surface. I don't think this is a good idea at all, unless I'm missing something.

I'd honestly rather create a new keyword 'requies' and use it here for 'requires iflib' but there's some other keywords that are needed to make that go since this really is more of a 'bring it in regardless of whether the kernel config has iflib' which might be better expressed as 'force' or some other keyword.

imp requested changes to this revision.Sep 24 2019, 5:34 PM
This revision now requires changes to proceed.Sep 24 2019, 5:34 PM
sbruno abandoned this revision.Sep 24 2019, 6:21 PM
sbruno added a subscriber: kib.

This was superceded by @kib in r343617. The purpose of this revision was to provide a simple way to have module support for the various adapters supported by iflib and to cull iflib out when not needed.