lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20190122014029.wrqvt3hfl3ee7g2x@shbuild888>
Date:   Tue, 22 Jan 2019 09:40:29 +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 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.

> 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 :)

> 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.

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.

Thanks,
Feng

> 
> > ---
> >  drivers/net/ethernet/intel/igb/igb_main.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c
> > index 7137e7f..d477253 100644
> > --- a/drivers/net/ethernet/intel/igb/igb_main.c
> > +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> > @@ -229,6 +229,7 @@ static struct pci_driver igb_driver = {
> >         .id_table = igb_pci_tbl,
> >         .probe    = igb_probe,
> >         .remove   = igb_remove,
> > +       .driver.probe_type = PROBE_PREFER_ASYNCHRONOUS,
> >  #ifdef CONFIG_PM
> >         .driver.pm = &igb_pm_ops,
> >  #endif
> > --
> > 2.7.4
> >
> > _______________________________________________
> > Intel-wired-lan mailing list
> > Intel-wired-lan@...osl.org
> > https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Powered by blists - more mailing lists