Date:   Tue, 22 Jan 2019 10:39:39 +0800
From:   Feng Tang <feng.tang@...el.com>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
        "David S . Miller" <davem@...emloft.net>,
        intel-wired-lan <intel-wired-lan@...ts.osuosl.org>,
        Netdev <netdev@...r.kernel.org>
Subject: Re: [Intel-wired-lan] [PATCH] igb: Make driver init async

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.tang@...el.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.tang@...el.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.tang@...el.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

Powered by blists - more mailing lists