Hi Alexander, On Mon, Jan 21, 2019 at 06:29:51PM -0800, Alexander Duyck wrote: > On Mon, Jan 21, 2019 at 5:39 PM Feng Tang <feng.t...@intel.com> wrote: > > > > Hi Duyck, > > > > Thanks for your review! > > > > On Mon, Jan 21, 2019 at 02:50:08PM -0800, Alexander Duyck wrote: > > > On Mon, Jan 21, 2019 at 12:36 AM Feng Tang <feng.t...@intel.com> wrote: > > > > > > > > When optimizing boot time for a platform with igb module, we found the > > > > igb driver probe will take about 45 ms, make the probe asynchronous > > > > will save quite some time as the init runs in parallel with other > > > > asynchronous drivers. > > > > > > > > In theory, this could be applied to some other drivers like igc or > > > > e1000, but we don't have HW to verify that. > > > > > > > > Signed-off-by: Feng Tang <feng.t...@intel.com> > > > > > > I really don't like this patch. This seems like you are tuning for a > > > specific platform or system setup by making changes to the driver. Is > > > there any reason why you couldn't just pass the module parameter > > > "igb.async_probe" to accomplish the same thing? > > > > Thanks for the info. I didn't make it clear, in some case we need it to > > be builtin, and this doesn't work per my try. > > Well then why not try implementing a similar feature that could be > used for built-in drivers. My point is that it is really bad to be > modifying a driver to work for a specific use case. It makes the > driver overly brittle from a maintenance point of view.
Ok, I see. > > > > I suspect most people > > > won't care that much about the 45ms boot time difference, and if we > > > > Yes, 45ms is trivial to Desktop/Server. But our platform is for automotive > > and IOTG case, and these count there. US Department of Transportation > > has a requirement that the rear-view camera needs to be functional in 2 > > seconds after power on. To achieve that, we've done things to get 20ms > > from PCI init, 5ms from disabling some ACPI_PM clocksource :) > > That is your set of requirements. There are multiple other users of > this driver out there. We cannot just go in and arbitrarily change > core behaviors for one specific use case. I would much prefer this to > be something that can be changed either via a sysctl or via a module > parameter so that we can have this behavior turned off by default if > so desired. > > > > were to make this sort of change it should probably be more > > > generically applied to either PCI devices or network devices instead > > > of doing this one driver at a time. > > > > Agree. I also mentioned this in commit log, I didn't do it for other drivers > > as I don't have HW to verify, and I would be happy to proceed if > > experts for those HW think this change is fine. > > You normally don't need to test all possible hardware that could be > impacted by a change, but what you should do is try to test what you > have available. > > This sort of change would make more sense as a PCI feature where you > essentially cause endpoint devices to asynchronously probe instead of > doing so synchronously if a certain kernel module parameter is set. > Otherwise if you ever change drivers in the future for some design you > will have to submit yet another patch for this. So for example one > thing you might look at doing is adding a new option for the "pci=" > module parameter. You could specify for example something like > "pci=async_probe=pci:8086:1533" and then that would specify all i210 This is really a good suggestion, which is much more flexible. I will do some try in this direction. > adapters with that device ID would be asynchronously probed instead of > going through the standard probe path. > > > Technically I think it's fine, as making them async will only push their > > init later, which won't break their existing dependence. And there should > > be very few modules which depend on network drivers. > > Actually this will break functionality on the 82576 quad port > adapters. Specifically we are using a static counter to identify which > groupings of 4 ports belonged together using the global_quad_port_a > value. With this changing to asynchronous you will end up confusing > the driver resulting in it randomly assigning WOL to possibly the > wrong port since the async code doesn't guarantee ordering of the > probe routines. Didn't know that, and thanks for the detaied info! - Feng > That would be another good reason why this should be handled as a per > PCI device/function type feature instead of trying to just make it > apply to all of igb. > > > Thanks, > > Feng > > > > Thanks. > > - Alex
