Discussion:
[RFC v2 2/6] driver-core: add driver async_probe support
(too old to reply)
Luis R. Rodriguez
2014-09-05 06:37:23 UTC
Permalink
From: "Luis R. Rodriguez" <***@suse.com>

We now have two documented use cases for probing asynchronously:

0) since we bundle together driver init() and probe() systemd's
new 30 second timeout has put a limit on the amount of time
a driver probe routine can take, we need to enable drivers
to complete probe gracefully

1) when a built-in driver takes a few seconds to initialize its
delays can stall the overall boot process

The built-in driver issues is pretty straight forward, and for
that we just need to let the probing happen behind the scenes.

The systemd issue is a bit more complex given the history of
how it was identified and work arounds proposed and evaluated.
The systemd issue was first identified first by Joseph when he
bisected and found that Tetsuo Handa's commit 786235ee
"kthread: make kthread_create() killable" modified kthread_create()
to bail as soon as SIGKILL is received [0] [1]. This was found
to cause some issues with some drivers and at times boot. There
are other patches which could also enable the SIGKILL trigger on
driver loading though:

70834d30 "usermodehelper: use UMH_WAIT_PROC consistently"
b3449922 "usermodehelper: introduce umh_complete(sub_info)"
d0bd587a "usermodehelper: implement UMH_KILLABLE"
9d944ef3 "usermodehelper: kill umh_wait, renumber UMH_* constants"
5b9bd473 "usermodehelper: ____call_usermodehelper() doesn't need do_exit()"
3e63a93b "kmod: introduce call_modprobe() helper"
1cc684ab "kmod: make __request_module() killable"

All of these went in on 3.4 upstream, and were part of the fixes
for CVE-2012-4398 [2] and documented more properly on Red Hat's
bugzilla [3]. Any of these patches may contribute to having a
module be properly killed now, but 786235ee is the latest in the
series. For instance on SLE12 cxgb4 has been fond to get the
SIGKILL even though SLE12 does not yet have 786235ee merged [4].

Joseph found that the systemd-udevd process sends SIGKILL to
systemd's usage of kmod for module loading if probe on a driver
takes over 30 seconds [5] [6]. When this happens probe will fail
on any driver, but *iff* its probe path ends up using kthreads.
Its why booting on some systems will fail if the driver happens
to be a storage related driver. When helping debug the issue
Tetsuo suggested fixing this issue by kmodifying kthread_create()
to not leave upon SIGKILL immediately *unless* the source of the
SIGKILL was the OOM, and actually wait for 10 seconds more before
completing the kill [7] *unless* the source of the killer was OOM.
This is not the only source of a kill, as noted above, this same
issue is present on kernels without commit 786235ee. Additionally
upon review of this patch Oleg rejected this change [8] and the
discussion was punted out to systemd-devel to see if the default
timeout could be increased from 30 seconds to 120 [9]. The opinion
of the systemd maintainers was that the driver's behavior should
be fixed [10]. Linus seems to agree [11], however more recently
even networking drivers have been reported to fail on probe
since just writing the firmware to a device and kicking it can
take easy over 60 seconds [4]. Benjamim was able to trace the
issues reported on cxgb4 down to the same systemd-udevd 30
second timeout [6]. Even if asynch firmware loading was used
on the cxgb4 driver the driver would *still* hit other delays
due to the way the driver is currently designed, fixing that
will require a bit of time and until then some users are left
with a completely dysfunctional device.

Folks were a bit confused here though -- its not only module
initialization which is being penalized, because the driver core
will immediately trigger the driver's own bus probe routine if
autoprobe is enabled each driver's probe routine must also
complete within the same 30 second timeout. This means not only
should driver's init complete within the set default systemd
timeout of 30 seconds but so should the probe routine, and
probe would obviously also have less time given that the
timeout is for both the module's init() and its own bus' probe().
A few drivers fail to complete the bus' probe within 30 seconds,
its not the init routine that takes long. The timeout seems
to currently hit *iff* kthreads are used somehow on the driver's
probe path. For example purposely breaking the e1000e driver
by adding a 30 second timeout on the probe path does not let
systemd kill it, however doing the same for iwlwifi triggers
the kill, this is because this driver uses request_threaded_irq()
and behind the scenes the kernel uses ktread_create() on
__setup_irq() to handle the thread *iff* its not nested, these
are drivers that set irq_set_nested_thread(irq, 1).

Hannes Reinecke has implemented now a timeout modifier for
systemd, however *systemd* still needs a way to gracefully
annotate drivers with long probes instead of failing these
drivers and at worst boot. On the kernel side of things we
can circumvent the timeout by probing asynchronously on only
drivers that need it. If a driver is changed to use this new
asynch probing, folks should be aware that any userspace
that assumed that completing driver loading would enable
device functionality will need to changed until the device
appears.

[0] http://thread.gmane.org/gmane.linux.ubuntu.devel.kernel.general/39123
[1] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1276705
[2] http://web.nvd.nist.gov/view/vuln/detail?vulnId=CVE-2012-4398
[3] https://bugzilla.redhat.com/show_bug.cgi?id=CVE-2012-4398
[4] https://bugzilla.novell.com/show_bug.cgi?id=877622
[5] http://article.gmane.org/gmane.linux.kernel/1669550
[6] https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1297248
[7] https://launchpadlibrarian.net/169657493/kthread-defer-leaving.patch
[8] http://article.gmane.org/gmane.linux.kernel/1669604
[9] http://lists.freedesktop.org/archives/systemd-devel/2014-March/018006.html
[10] http://article.gmane.org/gmane.comp.sysutils.systemd.devel/17860
[11] http://article.gmane.org/gmane.linux.kernel/1671333

Cc: Tejun Heo <***@kernel.org>
Cc: Arjan van de Ven <***@linux.intel.com>
Cc: Greg Kroah-Hartman <***@linuxfoundation.org>
Cc: Tetsuo Handa <penguin-***@I-love.SAKURA.ne.jp>
Cc: Joseph Salisbury <***@canonical.com>
Cc: Kay Sievers <***@vrfy.org>
Cc: One Thousand Gnomes <***@lxorguk.ukuu.org.uk>
Cc: Tim Gardner <***@canonical.com>
Cc: Pierre Fersing <pierre-***@pierref.org>
Cc: Andrew Morton <***@linux-foundation.org>
Cc: Oleg Nesterov <***@redhat.com>
Cc: Benjamin Poirier <***@suse.de>
Cc: Nagalakshmi Nandigama <***@avagotech.com>
Cc: Praveen Krishnamoorthy <***@avagotech.com>
Cc: Sreekanth Reddy <***@avagotech.com>
Cc: Abhijit Mahajan <***@avagotech.com>
Cc: Casey Leedom <***@chelsio.com>
Cc: Hariprasad S <***@chelsio.com>
Cc: Santosh Rastapur <***@chelsio.com>
Cc: MPT-***@avagotech.com
Cc: linux-***@vger.kernel.org
Cc: linux-***@vger.kernel.org
Cc: ***@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <***@suse.com>
---
drivers/base/base.h | 6 +++++
drivers/base/bus.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++---
drivers/base/dd.c | 4 ++++
include/linux/device.h | 5 +++++
4 files changed, 71 insertions(+), 3 deletions(-)

diff --git a/drivers/base/base.h b/drivers/base/base.h
index 251c5d3..24836f1 100644
--- a/drivers/base/base.h
+++ b/drivers/base/base.h
@@ -43,11 +43,17 @@ struct subsys_private {
};
#define to_subsys_private(obj) container_of(obj, struct subsys_private, subsys.kobj)

+struct driver_attach_work {
+ struct work_struct work;
+ struct device_driver *driver;
+};
+
struct driver_private {
struct kobject kobj;
struct klist klist_devices;
struct klist_node knode_bus;
struct module_kobject *mkobj;
+ struct driver_attach_work *attach_work;
struct device_driver *driver;
};
#define to_driver(obj) container_of(obj, struct driver_private, kobj)
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index a5f41e4..70d51b2 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -85,6 +85,7 @@ static void driver_release(struct kobject *kobj)
struct driver_private *drv_priv = to_driver(kobj);

pr_debug("driver: '%s': %s\n", kobject_name(kobj), __func__);
+ kfree(drv_priv->attach_work);
kfree(drv_priv);
}

@@ -662,10 +663,56 @@ static void remove_driver_private(struct device_driver *drv)
struct driver_private *priv = drv->p;

kobject_put(&priv->kobj);
+ kfree(priv->attach_work);
kfree(priv);
drv->p = NULL;
}

+static void driver_attach_workfn(struct work_struct *work)
+{
+ int ret;
+ struct driver_attach_work *attach_work =
+ container_of(work, struct driver_attach_work, work);
+ struct device_driver *drv = attach_work->driver;
+ ktime_t calltime, delta, rettime;
+ unsigned long long duration;
+
+ calltime = ktime_get();
+
+ ret = driver_attach(drv);
+ if (ret != 0) {
+ remove_driver_private(drv);
+ bus_put(drv->bus);
+ }
+
+ rettime = ktime_get();
+ delta = ktime_sub(rettime, calltime);
+ duration = (unsigned long long) ktime_to_ns(delta) >> 10;
+
+ pr_debug("bus: '%s': add driver %s attach completed after %lld usecs\n",
+ drv->bus->name, drv->name, duration);
+}
+
+int bus_driver_async_probe(struct device_driver *drv)
+{
+ struct driver_private *priv = drv->p;
+
+ priv->attach_work = kzalloc(sizeof(struct driver_attach_work),
+ GFP_KERNEL);
+ if (!priv->attach_work)
+ return -ENOMEM;
+
+ priv->attach_work->driver = drv;
+ INIT_WORK(&priv->attach_work->work, driver_attach_workfn);
+
+ pr_debug("bus: '%s': probe for driver %s is run asynchronously\n",
+ drv->bus->name, drv->name);
+
+ queue_work(system_unbound_wq, &priv->attach_work->work);
+
+ return 0;
+}
+
/**
* bus_add_driver - Add a driver to the bus.
* @drv: driver.
@@ -698,9 +745,15 @@ int bus_add_driver(struct device_driver *drv)

klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers);
if (drv->bus->p->drivers_autoprobe) {
- error = driver_attach(drv);
- if (error)
- goto out_unregister;
+ if (drv->owner && drv->async_probe) {
+ error = bus_driver_async_probe(drv);
+ if (error)
+ goto out_unregister;
+ } else {
+ error = driver_attach(drv);
+ if (error)
+ goto out_unregister;
+ }
}
module_add_driver(drv->owner, drv);

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index e4ffbcf..f1565f3 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -507,6 +507,10 @@ static void __device_release_driver(struct device *dev)

drv = dev->driver;
if (drv) {
+ if (drv->owner && drv->async_probe) {
+ struct driver_private *priv = drv->p;
+ flush_work(&priv->attach_work->work);
+ }
pm_runtime_get_sync(dev);

driver_sysfs_remove(dev);
diff --git a/include/linux/device.h b/include/linux/device.h
index 43d183a..7de1386b 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -200,6 +200,10 @@ extern struct klist *bus_get_device_klist(struct bus_type *bus);
* @owner: The module owner.
* @mod_name: Used for built-in modules.
* @suppress_bind_attrs: Disables bind/unbind via sysfs.
+ * @async_probe: requests probe to be run asynchronously. Drivers that
+ * have this enabled must take care that userspace will return
+ * immediately upon driver loading as probing will happen behind the
+ * schenes asynchronously.
* @of_match_table: The open firmware table.
* @acpi_match_table: The ACPI match table.
* @probe: Called to query the existence of a specific device,
@@ -233,6 +237,7 @@ struct device_driver {
const char *mod_name; /* used for built-in modules */

bool suppress_bind_attrs; /* disables bind/unbind via sysfs */
+ bool async_probe;

const struct of_device_id *of_match_table;
const struct acpi_device_id *acpi_match_table;
--
2.0.3
Luis R. Rodriguez
2014-09-05 06:37:24 UTC
Permalink
From: "Luis R. Rodriguez" <***@suse.com>

The new umh kill option has allowed kthreads to receive
kill signals but they are generally accepting all sources
of kill signals while the original motivation was to enable
through the OOM from sending the kill. One particular user
which has been found to send kill signals on kthreads is
systemd, it does this upon a 30 second default timeout on
loading modules. That timeout was in place under the
assumption that some driver's init sequences were taking
long. Since the kernel batches both init and probe together
though its actually been the probe routines which take
long. These should not be penalized, the kill would only
happen if and only if the driver's probe routine ends up
using kthreads somehow. To help with this we now have the
async_probe flag for drivers but before we can amend
drivers with this functionality we need to find them. This
patch addresses that by avoiding the kill from any other
source than the OOM killer -- for now.

Users can provide a log output and it should be clear on
the trace what probe / driver got the kill signal.

This patch is based on Tetsuo's patch [0] to try to address
the timeout issue, which in itself is based on Tetsuo's
original patch to also address this months ago [1]. These
patches just lacked addressing all other callers which
would load modules for us. Although Oleg had rejected a
similar change a while ago [2] its now clear what the
source of the problem. A few solutions have been proposed,
one of them was to allow the default systemd timeout to be
modified, that change by Hannes Reinecke is now merged
upstream on systemd, we still however need a non fatal
way to deal with modules that take long and an easy way
for us to find these modules. At least one proposal has
been made for systemd but discussions on that approach
hasn't gotten much traction [3] so we need to address
this on the kernel, this will also be important for users
of new kernels on old versions of systemd.

[0] https://launchpadlibrarian.net/169657493/kthread-defer-leaving.patch
[1] https://lkml.org/lkml/2014/7/29/284
[2] http://article.gmane.org/gmane.linux.kernel/1669604
[3] http://lists.freedesktop.org/archives/systemd-devel/2014-August/021852.html

An example log output captured by purposely breaking the iwlwifi
driver by using ssleep(33) on probe:

[ 43.853997] iwlwifi going to sleep for 33 seconds
[ 76.862975] iwlwifi done sleeping for 33 seconds
[ 76.863880] iwlwifi 0000:03:00.0: irq 34 for MSI/MSI-X
[ 76.863961] ------------[ cut here ]------------
[ 76.864648] WARNING: CPU: 0 PID: 479 at kernel/kthread.c:308 kthread_create_on_node+0x1ea/0x200()
[ 76.865309] Got SIGKILL but not from OOM, if this issue is on probe use .driver.async_probe
[ 76.865974] Modules linked in: xfs libcrc32c x86_pkg_temp_thermal intel_powerclamp coretemp kvm_intel kvm crct10dif_pclmul crc32_pclmul ghash_clmulni_intel aesni_intel snd_hda_codec_realtek snd_hda_codec_hdmi snd_hda_codec_generic snd_hda_intel snd_hda_controller snd_hda_codec snd_hwdep aes_x86_64 uvcvideo glue_helper videobuf2_vmalloc lrw gf128mul snd_pcm ablk_helper iTCO_wdt rtsx_pci_ms videobuf2_memops videobuf2_core rtsx_pci_sdmmc v4l2_common mmc_core videodev snd_timer thinkpad_acpi memstick iTCO_vendor_support snd mei_me rtsx_pci cryptd iwlwifi(+) mei shpchp tpm_tis soundcore pcspkr joydev lpc_ich mfd_core serio_raw tpm btusb wmi i2c_i801 thermal intel_smartconnect ac battery processor dm_mod btrfs xor raid6_pq i915 i2c_algo_bit e1000e drm_kms_helper sr_mod crc32c_intel cdrom xhci
_hcd drm video
[ 76.869197] button sg
[ 76.870035] CPU: 0 PID: 479 Comm: systemd-udevd Not tainted 3.17.0-rc3-25.g1474ea5-desktop+ #12
[ 76.870915] Hardware name: LENOVO 20AW000LUS/20AW000LUS, BIOS GLET43WW (1.18 ) 12/04/2013
[ 76.871801] 0000000000000009 ffff8802133a3908 ffffffff8173960f ffff8802133a3950
[ 76.872771] ffff8802133a3940 ffffffff81072eed ffff8800c9004480 ffffffff810c8fd0
[ 76.873693] ffffffff81a77845 00000000ffffffff ffff8800c9d2abc0 ffff8802133a39a0
[ 76.874620] Call Trace:
[ 76.875522] [<ffffffff8173960f>] dump_stack+0x4d/0x6f
[ 76.876379] [<ffffffff81072eed>] warn_slowpath_common+0x7d/0xa0
[ 76.877286] [<ffffffff810c8fd0>] ? irq_thread_check_affinity+0xb0/0xb0
[ 76.878177] [<ffffffff81072f5c>] warn_slowpath_fmt+0x4c/0x50
[ 76.879048] [<ffffffff810c8fd0>] ? irq_thread_check_affinity+0xb0/0xb0
[ 76.879898] [<ffffffff8108fdea>] kthread_create_on_node+0x1ea/0x200
[ 76.880765] [<ffffffff811bf50e>] ? enable_cpucache+0x4e/0xe0
[ 76.881617] [<ffffffff810c9c55>] __setup_irq+0x165/0x580
[ 76.882459] [<ffffffff8101bca6>] ? dma_generic_alloc_coherent+0x146/0x160
[ 76.883314] [<ffffffffa03cf780>] ? iwl_pcie_disable_ict+0x40/0x40 [iwlwifi]
[ 76.884159] [<ffffffff810ca1cf>] request_threaded_irq+0xcf/0x180
[ 76.885010] [<ffffffffa03d6efa>] iwl_trans_pcie_alloc+0x35a/0x4b1 [iwlwifi]
[ 76.885861] [<ffffffffa03cd3c0>] iwl_pci_probe+0x50/0x260 [iwlwifi]
[ 76.886646] [<ffffffff8146a59d>] ? __pm_runtime_resume+0x4d/0x60
[ 76.887404] [<ffffffff81383595>] local_pci_probe+0x45/0xa0
[ 76.888155] [<ffffffff81384795>] ? pci_match_device+0xe5/0x110
[ 76.888899] [<ffffffff813848d9>] pci_device_probe+0xd9/0x130
[ 76.889646] [<ffffffff8146090d>] driver_probe_device+0x12d/0x3e0
[ 76.890391] [<ffffffff81460c93>] __driver_attach+0x93/0xa0
[ 76.891132] [<ffffffff81460c00>] ? __device_attach+0x40/0x40
[ 76.891870] [<ffffffff8145e713>] bus_for_each_dev+0x63/0xa0
[ 76.892763] [<ffffffff814602de>] driver_attach+0x1e/0x20
[ 76.893528] [<ffffffff8145fe4e>] bus_add_driver+0xfe/0x270
[ 76.894292] [<ffffffffa036d000>] ? 0xffffffffa036d000
[ 76.895118] [<ffffffff814614e4>] driver_register+0x64/0xf0
[ 76.895847] [<ffffffff81382f1c>] __pci_register_driver+0x4c/0x50
[ 76.896615] [<ffffffffa03cd5f4>] iwl_pci_register_driver+0x24/0x40 [iwlwifi]
[ 76.896619] [<ffffffffa036d085>] iwl_drv_init+0x85/0x1000 [iwlwifi]
[ 76.896621] [<ffffffff81002144>] do_one_initcall+0xd4/0x210
[ 76.896624] [<ffffffff811a49e4>] ? __vunmap+0x94/0x100
[ 76.896626] [<ffffffff810f34d5>] load_module+0x1f25/0x2670
[ 76.896627] [<ffffffff810ef170>] ? store_uevent+0x40/0x40
[ 76.896630] [<ffffffff810f3d96>] SyS_finit_module+0x86/0xb0
[ 76.896632] [<ffffffff817413ed>] system_call_fastpath+0x1a/0x1f
[ 76.896632] ---[ end trace 9a32581b585745d8 ]---
[ 76.982019] iwlwifi 0000:03:00.0: loaded firmware version 23.214.9.0 op_mode iwlmvm
[ 77.174150] iwlwifi 0000:03:00.0: Detected Intel(R) Dual Band Wireless AC 7260, REV=0x144
[ 77.174952] iwlwifi 0000:03:00.0: L1 Enabled; Disabling L0S
[ 77.175955] iwlwifi 0000:03:00.0: L1 Enabled; Disabling L0S

Cc: Tejun Heo <***@kernel.org>
Cc: Arjan van de Ven <***@linux.intel.com>
Cc: Greg Kroah-Hartman <***@linuxfoundation.org>
Cc: Tetsuo Handa <penguin-***@I-love.SAKURA.ne.jp>
Cc: Joseph Salisbury <***@canonical.com>
Cc: Kay Sievers <***@vrfy.org>
Cc: One Thousand Gnomes <***@lxorguk.ukuu.org.uk>
Cc: Tim Gardner <***@canonical.com>
Cc: Pierre Fersing <pierre-***@pierref.org>
Cc: Andrew Morton <***@linux-foundation.org>
Cc: Oleg Nesterov <***@redhat.com>
Cc: Benjamin Poirier <***@suse.de>
Cc: Nagalakshmi Nandigama <***@avagotech.com>
Cc: Praveen Krishnamoorthy <***@avagotech.com>
Cc: Sreekanth Reddy <***@avagotech.com>
Cc: Abhijit Mahajan <***@avagotech.com>
Cc: Casey Leedom <***@chelsio.com>
Cc: Hariprasad S <***@chelsio.com>
Cc: Santosh Rastapur <***@chelsio.com>
Cc: MPT-***@avagotech.com
Cc: linux-***@vger.kernel.org
Cc: linux-***@vger.kernel.org
Cc: ***@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <***@suse.com>
---
kernel/kmod.c | 21 +++++++++++++++++++--
kernel/kthread.c | 19 +++++++++++++++++++
2 files changed, 38 insertions(+), 2 deletions(-)

diff --git a/kernel/kmod.c b/kernel/kmod.c
index 8637e04..b22228c 100644
--- a/kernel/kmod.c
+++ b/kernel/kmod.c
@@ -596,16 +596,33 @@ int call_usermodehelper_exec(struct subprocess_info *sub_info, int wait)
goto unlock;

if (wait & UMH_KILLABLE) {
+ unsigned int i;
+
retval = wait_for_completion_killable(&done);
- if (!retval)
+ if (likely(!retval))
goto wait_done;

+ /*
+ * I got SIGKILL, but wait for 60 more seconds for completion
+ * unless chosen by the OOM killer. This delay is there as a
+ * workaround for boot failure caused by SIGKILL upon device
+ * driver initialization timeout.
+ *
+ * N.B. this will actually let the thread complete regularly,
+ * wait_for_completion() will be used eventually, the 60 second
+ * try here is just to check for the OOM over that time.
+ */
+ WARN_ONCE(!test_thread_flag(TIF_MEMDIE),
+ "Got SIGKILL but not from OOM, if this issue is on probe use .driver.async_probe\n");
+ for (i = 0; i < 60 && !test_thread_flag(TIF_MEMDIE); i++)
+ if (wait_for_completion_timeout(&done, HZ))
+ goto wait_done;
+
/* umh_complete() will see NULL and free sub_info */
if (xchg(&sub_info->complete, NULL))
goto unlock;
/* fallthrough, umh_complete() was already called */
}
-
wait_for_completion(&done);
wait_done:
retval = sub_info->retval;
diff --git a/kernel/kthread.c b/kernel/kthread.c
index ef48322..bfb6dbe 100644
--- a/kernel/kthread.c
+++ b/kernel/kthread.c
@@ -292,6 +292,24 @@ struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
* new kernel thread.
*/
if (unlikely(wait_for_completion_killable(&done))) {
+ unsigned int i;
+
+ /*
+ * I got SIGKILL, but wait for 10 more seconds for completion
+ * unless chosen by the OOM killer. This delay is there as a
+ * workaround for boot failure caused by SIGKILL upon device
+ * driver initialization timeout.
+ *
+ * N.B. this will actually let the thread complete regularly,
+ * wait_for_completion() will be used eventually, the 10 second
+ * try here is just to check for the OOM over that time.
+ */
+ WARN_ONCE(!test_thread_flag(TIF_MEMDIE),
+ "Got SIGKILL but not from OOM, if this issue is on probe use .driver.async_probe\n");
+ for (i = 0; i < 10 && !test_thread_flag(TIF_MEMDIE); i++)
+ if (wait_for_completion_timeout(&done, HZ))
+ goto ready;
+
/*
* If I was SIGKILLed before kthreadd (or new kernel thread)
* calls complete(), leave the cleanup of this structure to
@@ -305,6 +323,7 @@ struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
*/
wait_for_completion(&done);
}
+ready:
task = create->result;
if (!IS_ERR(task)) {
static const struct sched_param param = { .sched_priority = 0 };
--
2.0.3
Tejun Heo
2014-09-05 07:19:25 UTC
Permalink
On Thu, Sep 04, 2014 at 11:37:24PM -0700, Luis R. Rodriguez wrote:
...
Post by Luis R. Rodriguez
+ /*
+ * I got SIGKILL, but wait for 60 more seconds for completion
+ * unless chosen by the OOM killer. This delay is there as a
+ * workaround for boot failure caused by SIGKILL upon device
+ * driver initialization timeout.
+ *
+ * N.B. this will actually let the thread complete regularly,
+ * wait_for_completion() will be used eventually, the 60 second
+ * try here is just to check for the OOM over that time.
+ */
+ WARN_ONCE(!test_thread_flag(TIF_MEMDIE),
+ "Got SIGKILL but not from OOM, if this issue is on probe use .driver.async_probe\n");
+ for (i = 0; i < 60 && !test_thread_flag(TIF_MEMDIE); i++)
+ if (wait_for_completion_timeout(&done, HZ))
+ goto wait_done;
+
Ugh... Jesus, this is way too hacky, so now we fail on 90s timeout
instead of 30? Why do we even need this with the proposed async
probing changes?

Thanks.
--
tejun
Luis R. Rodriguez
2014-09-05 07:47:16 UTC
Permalink
Post by Tejun Heo
...
+ /*
+ * I got SIGKILL, but wait for 60 more seconds for completion
+ * unless chosen by the OOM killer. This delay is there as a
+ * workaround for boot failure caused by SIGKILL upon device
+ * driver initialization timeout.
+ *
+ * N.B. this will actually let the thread complete regularly,
+ * wait_for_completion() will be used eventually, the 60 second
+ * try here is just to check for the OOM over that time.
+ */
+ WARN_ONCE(!test_thread_flag(TIF_MEMDIE),
+ "Got SIGKILL but not from OOM, if this issue is on probe use .driver.async_probe\n");
+ for (i = 0; i < 60 && !test_thread_flag(TIF_MEMDIE); i++)
+ if (wait_for_completion_timeout(&done, HZ))
+ goto wait_done;
+
Ugh... Jesus, this is way too hacky, so now we fail on 90s timeout
instead of 30?
Nope! I fell into the same trap and only with tons of patience by part
of Tetsuo with me was I able to grok that the 60 seconds here are not
for increasing the timeout, this is just time spent checking to ensure
that the OOM wasn't the one who triggered the SIGKILL. Even if the
drivers took eons it should be fine now, I tried it :D
Post by Tejun Heo
Why do we even need this with the proposed async
probing changes?
Ah -- well without it the way we "find" drivers that need this new
"async feature" is by a bug report and folks saying their system can't
boot, or they say their device doesn't come up. That's all. Tracing
this to systemd and a timeout was one of the most ugliest things ever.
There two insane bug reports you can go check:

mptsas was the first:

http://article.gmane.org/gmane.linux.kernel/1669550
https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1297248

Then cxgb4:

https://bugzilla.novell.com/show_bug.cgi?id=877622

I only had Cc'd you on the newest gem pata_marvell :

https://bugzilla.kernel.org/show_bug.cgi?id=59581

We can't seriously expect to be doing all this work for every driver.
a WARN_ONCE() would enable us to find the drivers that need this new
async probe "feature".

Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Mike Galbraith
2014-09-05 09:14:08 UTC
Permalink
Post by Luis R. Rodriguez
Post by Tejun Heo
...
+ /*
+ * I got SIGKILL, but wait for 60 more seconds for completion
+ * unless chosen by the OOM killer. This delay is there as a
+ * workaround for boot failure caused by SIGKILL upon device
+ * driver initialization timeout.
+ *
+ * N.B. this will actually let the thread complete regularly,
+ * wait_for_completion() will be used eventually, the 60 second
+ * try here is just to check for the OOM over that time.
+ */
+ WARN_ONCE(!test_thread_flag(TIF_MEMDIE),
+ "Got SIGKILL but not from OOM, if this issue is on probe use .driver.async_probe\n");
+ for (i = 0; i < 60 && !test_thread_flag(TIF_MEMDIE); i++)
+ if (wait_for_completion_timeout(&done, HZ))
+ goto wait_done;
+
Ugh... Jesus, this is way too hacky, so now we fail on 90s timeout
instead of 30?
Nope! I fell into the same trap and only with tons of patience by part
of Tetsuo with me was I able to grok that the 60 seconds here are not
for increasing the timeout, this is just time spent checking to ensure
that the OOM wasn't the one who triggered the SIGKILL. Even if the
drivers took eons it should be fine now, I tried it :D
Post by Tejun Heo
Why do we even need this with the proposed async
probing changes?
Ah -- well without it the way we "find" drivers that need this new
"async feature" is by a bug report and folks saying their system can't
boot, or they say their device doesn't come up. That's all. Tracing
this to systemd and a timeout was one of the most ugliest things ever.
http://article.gmane.org/gmane.linux.kernel/1669550
https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1297248
<quote>
(2) Currently systemd-udevd unconditionally sends SIGKILL upon hardcoded
30 seconds timeout. As a result, finit_module() of mptsas kernel
module receives SIGKILL when waiting for error handler thread to be
started.
</quote>

Hm. Why is this not a systemd-udevd bug for running around killing
stuff when it has no idea whether progress is being made or not?

-Mike
Tejun Heo
2014-09-05 14:12:41 UTC
Permalink
Post by Luis R. Rodriguez
Ah -- well without it the way we "find" drivers that need this new
"async feature" is by a bug report and folks saying their system can't
boot, or they say their device doesn't come up. That's all. Tracing
this to systemd and a timeout was one of the most ugliest things ever.
http://article.gmane.org/gmane.linux.kernel/1669550
https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1297248
https://bugzilla.novell.com/show_bug.cgi?id=877622
https://bugzilla.kernel.org/show_bug.cgi?id=59581
We can't seriously expect to be doing all this work for every driver.
a WARN_ONCE() would enable us to find the drivers that need this new
async probe "feature".
This whole approach of trying to mark specific drivers as needing
"async probing" is completely broken for the problem at hand. It
can't address the problem adequately while breaking backward
compatibility. I don't think this makes much sense.

Nacked-by: Tejun Heo <***@kernel.org>

Thanks.
--
tejun
Dmitry Torokhov
2014-09-05 16:44:05 UTC
Permalink
Post by Tejun Heo
Post by Luis R. Rodriguez
Ah -- well without it the way we "find" drivers that need this new
"async feature" is by a bug report and folks saying their system can't
boot, or they say their device doesn't come up. That's all. Tracing
this to systemd and a timeout was one of the most ugliest things ever.
http://article.gmane.org/gmane.linux.kernel/1669550
https://bugs.launchpad.net/ubuntu/+source/systemd/+bug/1297248
https://bugzilla.novell.com/show_bug.cgi?id=877622
https://bugzilla.kernel.org/show_bug.cgi?id=59581
We can't seriously expect to be doing all this work for every driver.
a WARN_ONCE() would enable us to find the drivers that need this new
async probe "feature".
This whole approach of trying to mark specific drivers as needing
"async probing" is completely broken for the problem at hand. It
can't address the problem adequately while breaking backward
compatibility. I don't think this makes much sense.
Which problem are we talking about here though? It does solve the slow device
stalling the rest if the kernel booting (non-module case) for me.

I also reject the notion that anyone should be relying on drivers to be fully
bound on module loading. It is not nineties anymore. We have hot pluggable
buses, deferred probing, and even for not hot-pluggable ones the module
providing the device itself might not be yet loaded. Any scripts that expect to
find device 100% ready after module loading are simply broken.

Thanks.
--
Dmitry
Tejun Heo
2014-09-05 17:49:25 UTC
Permalink
Hello,
Post by Dmitry Torokhov
Which problem are we talking about here though? It does solve the slow device
stalling the rest if the kernel booting (non-module case) for me.
The other one. The one with timeout. Neither cxgb4 or pata_marvell
has slow probing stalling boot problem.
Post by Dmitry Torokhov
I also reject the notion that anyone should be relying on drivers to be fully
bound on module loading. It is not nineties anymore. We have hot pluggable
buses, deferred probing, and even for not hot-pluggable ones the module
providing the device itself might not be yet loaded. Any scripts that expect to
find device 100% ready after module loading are simply broken.
We've been treating loading + probing as a single operation when
loading drivers and the assumption has always been that the existing
devices at the time of loading finished probing by the time insmod
finishes. We now need to split loading and probing and wait for each
of them differently. The *only* thing we can do is somehow making the
issuer specify that it's gonna wait for probing separately. I'm not
sure this can even be up for discussion. We're talking about a major
userland visible behavior change. We simply can't change it
underneath the existing users.

Thanks.
--
tejun
Dmitry Torokhov
2014-09-05 18:10:03 UTC
Permalink
Post by Tejun Heo
Hello,
Post by Dmitry Torokhov
Which problem are we talking about here though? It does solve the slow device
stalling the rest if the kernel booting (non-module case) for me.
The other one. The one with timeout. Neither cxgb4 or pata_marvell
has slow probing stalling boot problem.
Post by Dmitry Torokhov
I also reject the notion that anyone should be relying on drivers to be fully
bound on module loading. It is not nineties anymore. We have hot pluggable
buses, deferred probing, and even for not hot-pluggable ones the module
providing the device itself might not be yet loaded. Any scripts that expect to
find device 100% ready after module loading are simply broken.
We've been treating loading + probing as a single operation when
loading drivers and the assumption has always been that the existing
devices at the time of loading finished probing by the time insmod
finishes. We now need to split loading and probing and wait for each
of them differently. The *only* thing we can do is somehow making the
issuer specify that it's gonna wait for probing separately. I'm not
sure this can even be up for discussion. We're talking about a major
userland visible behavior change.
I do not agree that it is actually user-visible change: generally speaking you
do not really know if device is there or not. They come and go. Like I said,
consider all permutations, with hot-pluggable buses, deferred probing, etc,
etc.

Thanks.
--
Dmitry
Tejun Heo
2014-09-05 22:29:56 UTC
Permalink
Hello, Dmitry.
Post by Dmitry Torokhov
I do not agree that it is actually user-visible change: generally speaking you
do not really know if device is there or not. They come and go. Like I said,
consider all permutations, with hot-pluggable buses, deferred probing, etc,
It is for storage devices which always have guaranteed synchronous
probing on module load and well-defined probing order. Sure, modern
setups are a lot more dynamic but I'm quite certain that there are
setups in the wild which depend on storage driver loading being
synchronous. We can't simply declare one day that such behavior is
broken and break, most likely, their boots.

Thanks.
--
tejun
Tejun Heo
2014-09-05 22:31:39 UTC
Permalink
Post by Tejun Heo
It is for storage devices which always have guaranteed synchronous
probing on module load and well-defined probing order. Sure, modern
setups are a lot more dynamic but I'm quite certain that there are
setups in the wild which depend on storage driver loading being
synchronous. We can't simply declare one day that such behavior is
broken and break, most likely, their boots.
To add a bit, if the argument here is that dependency on such behavior
shouldn't exist and module loading and device probing should always be
asynchronous, the right approach is implementing "synchronous_probing"
flag not the other way around. I actually wouldn't hate to see that
change happening but whoever submits and routes such a change should
be ready for a major shitstorm, I'm afraid.

Thanks.
--
tejun
Dmitry Torokhov
2014-09-05 22:49:17 UTC
Permalink
Post by Tejun Heo
Post by Tejun Heo
It is for storage devices which always have guaranteed synchronous
probing on module load and well-defined probing order.
Agree about probing order (IIRC that is why we had to revert the
wholesale asynchronous probing a few years back) but totally disagree
about synchronous module loading.

Anyway, I just posted a patch that I think preserves module loading
behavior and solves my issue with built-in modules. It does not help
Luis' issue though (but then I think the main problem is with systemd
being stupid there).
Post by Tejun Heo
Post by Tejun Heo
Sure, modern
setups are a lot more dynamic but I'm quite certain that there are
setups in the wild which depend on storage driver loading being
synchronous. We can't simply declare one day that such behavior is
broken and break, most likely, their boots.
To add a bit, if the argument here is that dependency on such behavior
shouldn't exist and module loading and device probing should always be
asynchronous, the right approach is implementing "synchronous_probing"
flag not the other way around. I actually wouldn't hate to see that
change happening but whoever submits and routes such a change should
be ready for a major shitstorm, I'm afraid.
I think we already had this storm and that is why here we have opt-in
behavior for the drivers.

Thanks.
--
Dmitry
Tejun Heo
2014-09-05 22:55:33 UTC
Permalink
Hello, Dmitry.
Post by Dmitry Torokhov
Post by Tejun Heo
Post by Tejun Heo
It is for storage devices which always have guaranteed synchronous
probing on module load and well-defined probing order.
Agree about probing order (IIRC that is why we had to revert the
wholesale asynchronous probing a few years back) but totally disagree
about synchronous module loading.
I don't get it. This is a behavior userland already depends on for
boots. What's there to agree or disagree? This is just a fact that
we can't do this w/o disturbing some userlands in a major way.
Post by Dmitry Torokhov
Anyway, I just posted a patch that I think preserves module loading
behavior and solves my issue with built-in modules. It does not help
Luis' issue though (but then I think the main problem is with systemd
being stupid there).
This sure can be worked around from userland side too by not imposing
any timeout on module loading but that said for the same reasons that
you've been arguing until now, I actually do think that it's kinda
silly to make device probing synchronous to module loading at this
time and age. What we disagree on is not that we want to separate
those waits. It is about how to achieve it.
Post by Dmitry Torokhov
Post by Tejun Heo
To add a bit, if the argument here is that dependency on such behavior
shouldn't exist and module loading and device probing should always be
asynchronous, the right approach is implementing "synchronous_probing"
flag not the other way around. I actually wouldn't hate to see that
change happening but whoever submits and routes such a change should
be ready for a major shitstorm, I'm afraid.
I think we already had this storm and that is why here we have opt-in
behavior for the drivers.
It's a different shitstorm where we actively break bootings on some
userlands. Trust me. That's gonna be a lot worse.

Thanks.
--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Dmitry Torokhov
2014-09-05 23:22:42 UTC
Permalink
Hi Tejun,
Post by Tejun Heo
Hello, Dmitry.
Post by Dmitry Torokhov
Post by Tejun Heo
Post by Tejun Heo
It is for storage devices which always have guaranteed synchronous
probing on module load and well-defined probing order.
Agree about probing order (IIRC that is why we had to revert the
wholesale asynchronous probing a few years back) but totally disagree
about synchronous module loading.
I don't get it. This is a behavior userland already depends on for
boots. What's there to agree or disagree? This is just a fact that
we can't do this w/o disturbing some userlands in a major way.
I am just expressing my disbelief that somebody relies on module loading
being synchronous with probing. Out of curiosity, do you have any
pointers?
Post by Tejun Heo
Post by Dmitry Torokhov
Anyway, I just posted a patch that I think preserves module loading
behavior and solves my issue with built-in modules. It does not help
Luis' issue though (but then I think the main problem is with systemd
being stupid there).
This sure can be worked around from userland side too by not imposing
any timeout on module loading but that said for the same reasons that
you've been arguing until now, I actually do think that it's kinda
silly to make device probing synchronous to module loading at this
time and age. What we disagree on is not that we want to separate
those waits. It is about how to achieve it.
Well, there are separate things we want to solve. My main issue is not
with modules, but rather compiled-in drivers that stall kernel boot,
and these particular drivers are just fine if they are probed out of
bound.
Post by Tejun Heo
Post by Dmitry Torokhov
Post by Tejun Heo
To add a bit, if the argument here is that dependency on such behavior
shouldn't exist and module loading and device probing should always be
asynchronous, the right approach is implementing "synchronous_probing"
flag not the other way around. I actually wouldn't hate to see that
change happening but whoever submits and routes such a change should
be ready for a major shitstorm, I'm afraid.
I think we already had this storm and that is why here we have opt-in
behavior for the drivers.
It's a different shitstorm where we actively break bootings on some
userlands. Trust me. That's gonna be a lot worse.
That did break bootings and that's why we reverted the wholesale async
probing.

Thanks.
--
Dmitry
Tejun Heo
2014-09-05 23:32:03 UTC
Permalink
Hey,
Post by Dmitry Torokhov
Post by Tejun Heo
I don't get it. This is a behavior userland already depends on for
boots. What's there to agree or disagree? This is just a fact that
we can't do this w/o disturbing some userlands in a major way.
I am just expressing my disbelief that somebody relies on module loading
being synchronous with probing. Out of curiosity, do you have any
pointers?
I've seen initrd scripts which depended on the behavior to wait for
storage devices over the years. AFAIK, none of the modern distros
does it but this has been such a basic feature all along and it seems
highly unlikely to me that there's no userland remaining out there
depending on such behavior. We do have a lot of different userlands,
many of them quite ad-hoc.

Thanks.
--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Arjan van de Ven
2014-09-05 22:45:08 UTC
Permalink
Post by Tejun Heo
Hello, Dmitry.
Post by Dmitry Torokhov
I do not agree that it is actually user-visible change: generally speaking you
do not really know if device is there or not. They come and go. Like I said,
consider all permutations, with hot-pluggable buses, deferred probing, etc,
It is for storage devices which always have guaranteed synchronous
probing on module load and well-defined probing order. Sure, modern
setups are a lot more dynamic but I'm quite certain that there are
setups in the wild which depend on storage driver loading being
synchronous. We can't simply declare one day that such behavior is
broken and break, most likely, their boots.
we even depend on this in the mount-by-label cases

many setups assume that the internal storage prevails over the USB stick in the case of conflicts.
it's a security issue; you don't want the built in secure bootloader that has a kernel root argument
by label/uuid.
the security there tends to assume that built-in wins over USB
Dmitry Torokhov
2014-09-05 22:52:48 UTC
Permalink
Post by Arjan van de Ven
Post by Tejun Heo
Hello, Dmitry.
Post by Dmitry Torokhov
I do not agree that it is actually user-visible change: generally speaking you
do not really know if device is there or not. They come and go. Like I said,
consider all permutations, with hot-pluggable buses, deferred probing, etc,
It is for storage devices which always have guaranteed synchronous
probing on module load and well-defined probing order. Sure, modern
setups are a lot more dynamic but I'm quite certain that there are
setups in the wild which depend on storage driver loading being
synchronous. We can't simply declare one day that such behavior is
broken and break, most likely, their boots.
we even depend on this in the mount-by-label cases
many setups assume that the internal storage prevails over the USB stick in the case of conflicts.
it's a security issue; you don't want the built in secure bootloader that has a kernel root argument
by label/uuid.
the security there tends to assume that built-in wins over USB
Ahem... and they sure it works reliably with large storage arrays? With
SCSI doing probing asynchronously already?

Thanks.
--
Dmitry
Tejun Heo
2014-09-05 22:57:32 UTC
Permalink
Hello,
Post by Dmitry Torokhov
Ahem... and they sure it works reliably with large storage arrays? With
SCSI doing probing asynchronously already?
I believe this has been mentioned before too but, yes, SCSI device
probing is asynchronous and parallelized but the registration of the
discovered devices are fully serialized according to driver attach
order. Storage devices are probed in parallel and attached in a fully
deterministic order. That part has never changed.

Thanks.
--
tejun
Arjan van de Ven
2014-09-05 23:05:30 UTC
Permalink
Post by Dmitry Torokhov
Post by Arjan van de Ven
Post by Tejun Heo
Hello, Dmitry.
Post by Dmitry Torokhov
I do not agree that it is actually user-visible change: generally speaking you
do not really know if device is there or not. They come and go. Like I said,
consider all permutations, with hot-pluggable buses, deferred probing, etc,
It is for storage devices which always have guaranteed synchronous
probing on module load and well-defined probing order. Sure, modern
setups are a lot more dynamic but I'm quite certain that there are
setups in the wild which depend on storage driver loading being
synchronous. We can't simply declare one day that such behavior is
broken and break, most likely, their boots.
we even depend on this in the mount-by-label cases
many setups assume that the internal storage prevails over the USB stick in the case of conflicts.
it's a security issue; you don't want the built in secure bootloader that has a kernel root argument
by label/uuid.
the security there tends to assume that built-in wins over USB
Ahem... and they sure it works reliably with large storage arrays? With
SCSI doing probing asynchronously already?
you tend to trust your large storage array
you tend to not trust the walk up USB stick.
Dmitry Torokhov
2014-09-05 23:18:22 UTC
Permalink
Post by Arjan van de Ven
Post by Dmitry Torokhov
Post by Arjan van de Ven
Post by Tejun Heo
Hello, Dmitry.
Post by Dmitry Torokhov
I do not agree that it is actually user-visible change: generally speaking you
do not really know if device is there or not. They come and go. Like I said,
consider all permutations, with hot-pluggable buses, deferred probing, etc,
It is for storage devices which always have guaranteed synchronous
probing on module load and well-defined probing order. Sure, modern
setups are a lot more dynamic but I'm quite certain that there are
setups in the wild which depend on storage driver loading being
synchronous. We can't simply declare one day that such behavior is
broken and break, most likely, their boots.
we even depend on this in the mount-by-label cases
many setups assume that the internal storage prevails over the USB stick in the case of conflicts.
it's a security issue; you don't want the built in secure bootloader that has a kernel root argument
by label/uuid.
the security there tends to assume that built-in wins over USB
Ahem... and they sure it works reliably with large storage arrays? With
SCSI doing probing asynchronously already?
you tend to trust your large storage array
you tend to not trust the walk up USB stick.
If you allow physical access it does not matter really.
--
Dmitry
Luis R. Rodriguez
2014-09-05 18:12:17 UTC
Permalink
Post by Tejun Heo
Hello,
Post by Dmitry Torokhov
Which problem are we talking about here though? It does solve the slow device
stalling the rest if the kernel booting (non-module case) for me.
The other one. The one with timeout. Neither cxgb4 or pata_marvell
has slow probing stalling boot problem.
Post by Dmitry Torokhov
I also reject the notion that anyone should be relying on drivers to be fully
bound on module loading. It is not nineties anymore. We have hot pluggable
buses, deferred probing, and even for not hot-pluggable ones the module
providing the device itself might not be yet loaded. Any scripts that expect to
find device 100% ready after module loading are simply broken.
We've been treating loading + probing as a single operation when
loading drivers and the assumption has always been that the existing
devices at the time of loading finished probing by the time insmod
finishes. We now need to split loading and probing and wait for each
of them differently. The *only* thing we can do is somehow making the
issuer specify that it's gonna wait for probing separately. I'm not
sure this can even be up for discussion. We're talking about a major
userland visible behavior change. We simply can't change it
underneath the existing users.
Meanwhile we are allowing a major design consideration such as a 30
second timeout for both init + probe all of a sudden become a hard
requirement for device drivers. I see your point but can't also be
introducing major design changes willy nilly either. We *need* a
solution for the affected drivers.

Also what stops drivers from going ahead and just implementing their
own async probe? Would that now be frowned upon as it strives away
from the original design? The bool would let those drivers do this
easily, and we would still need to identify these drivers, although
this particular change can be NAK'd Oleg's suggestion on
WARN_ON(fatal_signal_pending() at the end of load_module() seems to me
at least needed. And if its not async probe... what do those with
failed drivers do?

Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Dmitry Torokhov
2014-09-05 18:29:28 UTC
Permalink
Post by Luis R. Rodriguez
Post by Tejun Heo
Hello,
Post by Dmitry Torokhov
Which problem are we talking about here though? It does solve the slow device
stalling the rest if the kernel booting (non-module case) for me.
The other one. The one with timeout. Neither cxgb4 or pata_marvell
has slow probing stalling boot problem.
Post by Dmitry Torokhov
I also reject the notion that anyone should be relying on drivers to be fully
bound on module loading. It is not nineties anymore. We have hot pluggable
buses, deferred probing, and even for not hot-pluggable ones the module
providing the device itself might not be yet loaded. Any scripts that expect to
find device 100% ready after module loading are simply broken.
We've been treating loading + probing as a single operation when
loading drivers and the assumption has always been that the existing
devices at the time of loading finished probing by the time insmod
finishes. We now need to split loading and probing and wait for each
of them differently. The *only* thing we can do is somehow making the
issuer specify that it's gonna wait for probing separately. I'm not
sure this can even be up for discussion. We're talking about a major
userland visible behavior change. We simply can't change it
underneath the existing users.
Meanwhile we are allowing a major design consideration such as a 30
second timeout for both init + probe all of a sudden become a hard
requirement for device drivers. I see your point but can't also be
introducing major design changes willy nilly either. We *need* a
solution for the affected drivers.
Also what stops drivers from going ahead and just implementing their
own async probe?
They already do and the problem is that they do that poorly. One of the issues
is that the device is considered bound and so may attempt to suspend/resume
them, or unbind them, and the driver is not ready for such operations to take
place.

And even though driver is bound "synchronously" it does not help the user in
the slightest and the object that is the result of driver initialization is
still created asynchronously and is not ready (well, it might if drivers use
async_schedule as we are doing asych_sycnhronize_full() in module load.unload).

Thanks.
--
Dmitry
Tejun Heo
2014-09-05 22:40:47 UTC
Permalink
Hello, Luis.
Post by Luis R. Rodriguez
Meanwhile we are allowing a major design consideration such as a 30
second timeout for both init + probe all of a sudden become a hard
requirement for device drivers. I see your point but can't also be
introducing major design changes willy nilly either. We *need* a
solution for the affected drivers.
Yes, make the behavior specifically specified from userland. When did
I ever say that there should be no solution for the problem? I've
been saying that the behavior should be selected from userland from
the get-go, haven't I?

I have no idea how the seleciton should be. It could be per-insmod or
maybe just a system-wide flag with explicit exceptions marked on
drivers is good enough. I don't know.
Post by Luis R. Rodriguez
Also what stops drivers from going ahead and just implementing their
own async probe? Would that now be frowned upon as it strives away
The drivers can't. How many times should I explain the same thing
over and over again. libata can't simply make probing asynchronous
w.r.t. module loading no matter how it does it. Yeah, sure, there can
be other drivers which can do that without most people noticing it but
a storage driver isn't one of them and the storage drivers are the
problematic ones already, right?
Post by Luis R. Rodriguez
from the original design? The bool would let those drivers do this
easily, and we would still need to identify these drivers, although
this particular change can be NAK'd Oleg's suggestion on
WARN_ON(fatal_signal_pending() at the end of load_module() seems to me
at least needed. And if its not async probe... what do those with
failed drivers do?
I'm getting tired of explaining the same thing over and over again.
The said change was nacked because the whole approach of "let's see
which drivers get reported on the issue which exists basically for all
drivers and just change the behavior of them" is braindead. It makes
no sense whatsoever. It doesn't address the root cause of the problem
while making the same class of drivers behave significantly
differently for no good reason. Please stop chasing your own tail and
try to understand the larger picture.

Thanks.
--
tejun
Luis R. Rodriguez
2014-09-09 01:04:23 UTC
Permalink
Post by Tejun Heo
Hello, Luis.
Post by Luis R. Rodriguez
Meanwhile we are allowing a major design consideration such as a 30
second timeout for both init + probe all of a sudden become a hard
requirement for device drivers. I see your point but can't also be
introducing major design changes willy nilly either. We *need* a
solution for the affected drivers.
Yes, make the behavior specifically specified from userland. When did
I ever say that there should be no solution for the problem? I've
been saying that the behavior should be selected from userland from
the get-go, haven't I?
I have no idea how the selection should be. It could be per-insmod or
maybe just a system-wide flag with explicit exceptions marked on
drivers is good enough. I don't know.
Its perfectly understandable if we don't know what path to take yet
and its also understandable for it to take time to figure out --
meanwhile though systemd already has merged a policy of a 30 second
timeout for *all drivers* though so we therefore need:

0) a solutions for affected combination of systemd / drivers
1) an agreed path forward

If we want a tight integration between both kernel / init system we
need to be able to communicate effectively folks and I'm afraid this
isn't happening. I last noted on systemd-devel how the 30 second
timeout issue was merged under incorrect assumptions -- that it was
not just init that at times caused delays, and that since we currently
batch both init and probe on the driver core we need a non fatal
userspace solution [0], while we work on design on the kernel side of
things for async'ing for drivers that make sense. A proper kernel
solution may take longer than expected, we can't just assume a
probe_async flag will suffice on drivers, in fact as Tejun notes, its
wrong since historically we have had some random userland depend on
the synhronous behaviour of module loading of some drivers, and that
*could* have taken a while.

Kay, Lennart, any recommendations ?

[0] http://lists.freedesktop.org/archives/systemd-devel/2014-August/022696.html
Post by Tejun Heo
Post by Luis R. Rodriguez
Also what stops drivers from going ahead and just implementing their
own async probe? Would that now be frowned upon as it strives away
The drivers can't. How many times should I explain the same thing
over and over again. libata can't simply make probing asynchronous
w.r.t. module loading no matter how it does it. Yeah, sure, there can
be other drivers which can do that without most people noticing it but
a storage driver isn't one of them and the storage drivers are the
problematic ones already, right?
Its one of the subsystems that has suffered from this, but not the only one.
Post by Tejun Heo
Post by Luis R. Rodriguez
from the original design? The bool would let those drivers do this
easily, and we would still need to identify these drivers, although
this particular change can be NAK'd Oleg's suggestion on
WARN_ON(fatal_signal_pending() at the end of load_module() seems to me
at least needed. And if its not async probe... what do those with
failed drivers do?
I'm getting tired of explaining the same thing over and over again.
The said change was nacked because the whole approach of "let's see
which drivers get reported on the issue which exists basically for all
drivers and just change the behavior of them" is braindead. It makes
no sense whatsoever. It doesn't address the root cause of the problem
while making the same class of drivers behave significantly
differently for no good reason. Please stop chasing your own tail and
try to understand the larger picture.
Understood.

Luis
Tejun Heo
2014-09-09 01:10:59 UTC
Permalink
Hello, Luis.
Post by Luis R. Rodriguez
Post by Tejun Heo
I have no idea how the selection should be. It could be per-insmod or
maybe just a system-wide flag with explicit exceptions marked on
drivers is good enough. I don't know.
Its perfectly understandable if we don't know what path to take yet
and its also understandable for it to take time to figure out --
meanwhile though systemd already has merged a policy of a 30 second
I'm not too convinced this is such a difficult problem to figure out.
We already have most of logic in place and the only thing missing is
how to switch it. Wouldn't something like the following work?

* Add a sysctl knob to enable asynchronous device probing on module
load and enable asynchronous probing globally if the knob is set.

* Identify cases which can't be asynchronous and make them
synchronous. e.g. keep who's doing request_module() and avoid
asynchronous probing if current is probing one of those.

Thanks.
--
tejun
Tejun Heo
2014-09-09 01:13:02 UTC
Permalink
Post by Tejun Heo
* Identify cases which can't be asynchronous and make them
synchronous. e.g. keep who's doing request_module() and avoid
asynchronous probing if current is probing one of those.
That wouldn't work as we don't know what's gonna happen in userland
but we can start with just disallowing async probing for char devices
for now.

Thanks.
--
tejun
Tejun Heo
2014-09-09 01:22:27 UTC
Permalink
Post by Tejun Heo
I'm not too convinced this is such a difficult problem to figure out.
We already have most of logic in place and the only thing missing is
how to switch it. Wouldn't something like the following work?
* Add a sysctl knob to enable asynchronous device probing on module
load and enable asynchronous probing globally if the knob is set.
Alternatively, add a module-generic param "async_probe" or whatever
and use that to switch the behavior should work too. I don't know
which way is better but either should work fine.

Thanks.
--
tejun
Luis R. Rodriguez
2014-09-09 01:26:04 UTC
Permalink
Post by Tejun Heo
Post by Tejun Heo
I'm not too convinced this is such a difficult problem to figure out.
We already have most of logic in place and the only thing missing is
how to switch it. Wouldn't something like the following work?
* Add a sysctl knob to enable asynchronous device probing on module
load and enable asynchronous probing globally if the knob is set.
Alternatively, add a module-generic param "async_probe" or whatever
and use that to switch the behavior should work too. I don't know
which way is better but either should work fine.
I take it by this you meant a generic system-wide sysctl or kernel cmd
line option to enable this for al drivers?

Luis
Tejun Heo
2014-09-09 01:29:33 UTC
Permalink
Post by Luis R. Rodriguez
Post by Tejun Heo
Alternatively, add a module-generic param "async_probe" or whatever
and use that to switch the behavior should work too. I don't know
which way is better but either should work fine.
I take it by this you meant a generic system-wide sysctl or kernel cmd
line option to enable this for al drivers?
Well, either global or per-insmod switch should work. There probably
are details that I haven't mentioned - e.g. probably global switch is
easier to backport and deploy to existing systems - but as long as it
works I don't have fundmental objections either way.

Thanks.
--
tejun
Luis R. Rodriguez
2014-09-09 01:38:34 UTC
Permalink
Post by Tejun Heo
Post by Luis R. Rodriguez
Post by Tejun Heo
Alternatively, add a module-generic param "async_probe" or whatever
and use that to switch the behavior should work too. I don't know
which way is better but either should work fine.
I take it by this you meant a generic system-wide sysctl or kernel cmd
line option to enable this for al drivers?
Well, either global or per-insmod switch should work. There probably
are details that I haven't mentioned - e.g. probably global switch is
easier to backport and deploy to existing systems
Yes a global sysctl solution might make it easier to backport.
Post by Tejun Heo
- but as long as it
works I don't have fundmental objections either way.
OK then one only concern I would have with this is that the presence
of such a flag doesn't necessarily mean that all drivers on a system
have been tested for asynch probe yet. I'd feel much more comfortable
if this global flag allowed say specific drivers that *did* have such
a bool enabled, for example. Then that would enable synchronous
behaviour for the kernel by default, require the flag for enabling the
new async feature but only for drivers that have been tested.

That also still would not technically solve the issue of the current
existence of the timeout, unless of course we wish to ask systemd to
only make the timeout take effect *iff* the global sysctl flag /
whatever was enabled.

Luis
Tejun Heo
2014-09-09 01:47:54 UTC
Permalink
Hello,
Post by Luis R. Rodriguez
OK then one only concern I would have with this is that the presence
of such a flag doesn't necessarily mean that all drivers on a system
have been tested for asynch probe yet. I'd feel much more comfortable
Given that the behvaior change is from driver core and that device
probing can happen post-loading anyway, I don't think we need to worry
about drivers breaking from probing made asynchronous to loading. The
problem is the expectation of the entity which initiated loading of
the module. If it's depending on device being probed synchronously
but insmod returns before that, it can break things. We probably
should audit request_module() users and see which ones expect such
behavior.
Post by Luis R. Rodriguez
if this global flag allowed say specific drivers that *did* have such
a bool enabled, for example. Then that would enable synchronous
behaviour for the kernel by default, require the flag for enabling the
new async feature but only for drivers that have been tested.
If we're gonna do the global switch, I personally think the right
approach is blacklisting instead of the other way around because each
specific driver doesn't really have much to do with it and the
exceptions are about specific use cases that we don't have a good way
to identify them from module loading path.
Post by Luis R. Rodriguez
That also still would not technically solve the issue of the current
existence of the timeout, unless of course we wish to ask systemd to
only make the timeout take effect *iff* the global sysctl flag /
whatever was enabled.
Userland could backport a fix to set the sysctl. Given that we need
both synchrnous and asynchronous behaviors, it's unlikely that we can
come up with a solution which doesn't need cooperation from userland.

Thanks.
--
tejun
Luis R. Rodriguez
2014-09-09 02:28:58 UTC
Permalink
Post by Tejun Heo
Hello,
Post by Luis R. Rodriguez
OK then one only concern I would have with this is that the presence
of such a flag doesn't necessarily mean that all drivers on a system
have been tested for asynch probe yet. I'd feel much more comfortable
Given that the behvaior change is from driver core and that device
probing can happen post-loading anyway,
Ah but lets not forget Dmitry's requirement which is for in-kernel
drivers. We'd need to deal with both built-in and modules. Dmitry's
case is completely orthogonal to the systemd issue and is just needed
to help not stall boot but I see no reason to blend these two issues
into one requirement together.
Post by Tejun Heo
I don't think we need to worry
about drivers breaking from probing made asynchronous to loading. The
problem is the expectation of the entity which initiated loading of
the module. If it's depending on device being probed synchronously
but insmod returns before that, it can break things. We probably
should audit request_module() users and see which ones expect such
behavior.
Sure. Based on a quick glance I see sloppy uses of this, this should
probably be fixed anyway.
Post by Tejun Heo
Post by Luis R. Rodriguez
if this global flag allowed say specific drivers that *did* have such
a bool enabled, for example. Then that would enable synchronous
behaviour for the kernel by default, require the flag for enabling the
new async feature but only for drivers that have been tested.
If we're gonna do the global switch, I personally think the right
approach is blacklisting instead of the other way around because each
specific driver doesn't really have much to do with it and the
exceptions are about specific use cases that we don't have a good way
to identify them from module loading path.
OK sure... even if we did whitelist I'm afraid such a white list might
be subjective in terms of design to specific systems anyway... I
suppose the only real way to do it right is to push and strive towards
a full system whitelist and address the black list as you mention.

In terms of approach we would still need to decide on a path for how
to do asynch probing for both in-kernel drivers and modules, do we
want async_schedule(), or queue_work()? If async_schedule() do we want
to use a new domain or a new one shared for all drivers? Priority on
the schedular was one of my other concerns which we'd need to make
right to match existing load on drivers through finit_module() and
synchronous probe.
Post by Tejun Heo
Post by Luis R. Rodriguez
That also still would not technically solve the issue of the current
existence of the timeout, unless of course we wish to ask systemd to
only make the timeout take effect *iff* the global sysctl flag /
whatever was enabled.
Userland could backport a fix to set the sysctl. Given that we need
both synchrnous and asynchronous behaviors, it's unlikely that we can
come up with a solution which doesn't need cooperation from userland.
True and then the timeout would also have to be skipped for device
drivers that have the sync_probe flag set, so I guess we'd need to
expose that too. I'm not too sure if systemd is equipped to be happy
with no timeout on module loading based previous discussions [0] so
we'd need to ensure we're all in agreement there that such drivers
exist and we may need *something*, if at the very least a really long
fucking timeout (TM) for such drivers.

[0] http://lists.freedesktop.org/archives/systemd-devel/2014-August/021852.html

Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Tejun Heo
2014-09-09 02:39:43 UTC
Permalink
Hello,
Post by Luis R. Rodriguez
Post by Tejun Heo
Given that the behvaior change is from driver core and that device
probing can happen post-loading anyway,
Ah but lets not forget Dmitry's requirement which is for in-kernel
drivers. We'd need to deal with both built-in and modules. Dmitry's
case is completely orthogonal to the systemd issue and is just needed
to help not stall boot but I see no reason to blend these two issues
into one requirement together.
Maybe we can piggy back the two on the same mechanism but as you said
the two issues are orthogonal. Let's keep it that way for now. We
need them separate anyway for backports.
Post by Luis R. Rodriguez
In terms of approach we would still need to decide on a path for how
to do asynch probing for both in-kernel drivers and modules, do we
want async_schedule(), or queue_work()? If async_schedule() do we want
to use a new domain or a new one shared for all drivers? Priority on
I don't think async_schedule() is the right mechanism for this use
case as the mechanism is inherently opportunistic. It also gets
tangled up with async synchronization at the end of module loading.
Post by Luis R. Rodriguez
the schedular was one of my other concerns which we'd need to make
right to match existing load on drivers through finit_module() and
synchronous probe.
Why do we care about the priority of probing tasks? Does that
actually make any meaningful difference? If so, how?
Post by Luis R. Rodriguez
Post by Tejun Heo
Userland could backport a fix to set the sysctl. Given that we need
both synchrnous and asynchronous behaviors, it's unlikely that we can
come up with a solution which doesn't need cooperation from userland.
True and then the timeout would also have to be skipped for device
drivers that have the sync_probe flag set, so I guess we'd need to
I'm not sure about skipping for sync_probe flag. That seems like an
implementation detail to me. Sure, we do that now because we don't
have a better way of figuring out whether request_module() is waiting
for it or not but hopefully we'd be able to in the future. I think we
just should make exceptions sensible so that it works fine in practice
for now (and I don't think that'd be too hard). So, the only
cooperation necessary from userland would be just saying "I don't
wanna wait for device probing on module load."

Thanks.
--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Luis R. Rodriguez
2014-09-09 02:57:28 UTC
Permalink
Post by Tejun Heo
Hello,
Post by Luis R. Rodriguez
Post by Tejun Heo
Given that the behvaior change is from driver core and that device
probing can happen post-loading anyway,
Ah but lets not forget Dmitry's requirement which is for in-kernel
drivers. We'd need to deal with both built-in and modules. Dmitry's
case is completely orthogonal to the systemd issue and is just needed
to help not stall boot but I see no reason to blend these two issues
into one requirement together.
Maybe we can piggy back the two on the same mechanism but as you said
the two issues are orthogonal. Let's keep it that way for now. We
need them separate anyway for backports.
OK.
Post by Tejun Heo
Post by Luis R. Rodriguez
In terms of approach we would still need to decide on a path for how
to do asynch probing for both in-kernel drivers and modules, do we
want async_schedule(), or queue_work()? If async_schedule() do we want
to use a new domain or a new one shared for all drivers? Priority on
I don't think async_schedule() is the right mechanism for this use
case as the mechanism is inherently opportunistic. It also gets
tangled up with async synchronization at the end of module loading.
Post by Luis R. Rodriguez
the schedular was one of my other concerns which we'd need to make
right to match existing load on drivers through finit_module() and
synchronous probe.
Why do we care about the priority of probing tasks? Does that
actually make any meaningful difference? If so, how?
As I noted before -- I have yet to provide clear metrics but at least
changing both init paths + probe from finit_module() to kthread
certainly had a measurable time increase, I suspect using
queue_work(system_unbound_wq, async_probe_work) will make probe
slower. I'll get to these metrics this week.
Post by Tejun Heo
Post by Luis R. Rodriguez
Post by Tejun Heo
Userland could backport a fix to set the sysctl. Given that we need
both synchrnous and asynchronous behaviors, it's unlikely that we can
come up with a solution which doesn't need cooperation from userland.
True and then the timeout would also have to be skipped for device
drivers that have the sync_probe flag set, so I guess we'd need to
I'm not sure about skipping for sync_probe flag. That seems like an
implementation detail to me. Sure, we do that now because we don't
have a better way of figuring out whether request_module() is waiting
for it or not but hopefully we'd be able to in the future.
Oh I was not thinking about just request_modules() users but also any
of those stragglers which we might have ended up finding through run
time analysis. The alternative right now is these drivers won't load.
No bueno.
Post by Tejun Heo
I think we
just should make exceptions sensible so that it works fine in practice
for now (and I don't think that'd be too hard). So, the only
cooperation necessary from userland would be just saying "I don't
wanna wait for device probing on module load."
But we're talking about drivers that have a flag that says 'you gotta
wait sucker', what do we want systemd to do then? I'd be happy if it'd
would not send the sigkill for these drivers, for example.

Luis
Tejun Heo
2014-09-09 03:03:53 UTC
Permalink
Post by Luis R. Rodriguez
Post by Dmitry Torokhov
I think we
just should make exceptions sensible so that it works fine in practice
for now (and I don't think that'd be too hard). So, the only
cooperation necessary from userland would be just saying "I don't
wanna wait for device probing on module load."
But we're talking about drivers that have a flag that says 'you gotta
wait sucker', what do we want systemd to do then? I'd be happy if it'd
would not send the sigkill for these drivers, for example.
Hah? Can you give me an example? I'm having hard time imagining a
driver with such requirement given our current driver core
implementation.

Thanks.
--
tejun
Luis R. Rodriguez
2014-09-09 03:19:12 UTC
Permalink
Post by Tejun Heo
Post by Luis R. Rodriguez
Post by Dmitry Torokhov
I think we
just should make exceptions sensible so that it works fine in practice
for now (and I don't think that'd be too hard). So, the only
cooperation necessary from userland would be just saying "I don't
wanna wait for device probing on module load."
But we're talking about drivers that have a flag that says 'you gotta
wait sucker', what do we want systemd to do then? I'd be happy if it'd
would not send the sigkill for these drivers, for example.
Hah? Can you give me an example? I'm having hard time imagining a
driver with such requirement given our current driver core
implementation.
I didn't say I had one in mind, but if you're certain these *shouldn't
exist* that's sufficient by me as well.

OK so I'll respin this series to enable a sysctl that would enable
async probe for *all drivers* using queue_work(system_unbound_wq) and
only use sync probe for now on request_module() users, we'll address
scheduling issues as they come up. I'll be ignoring built-in.

On the systemd side of things it should enable this sysctl and for
older kernels what should it do?

Luis
Tejun Heo
2014-09-09 03:25:29 UTC
Permalink
Hello,
Post by Luis R. Rodriguez
On the systemd side of things it should enable this sysctl and for
older kernels what should it do?
Supposing the change is backported via -stable, it can try to set the
sysctl on all kernels. If the knob doesn't exist, the fix is not
there and nothing can be done about it.

Thanks.
--
tejun
Tejun Heo
2014-09-09 23:03:33 UTC
Permalink
Post by Tejun Heo
Hello,
Post by Luis R. Rodriguez
On the systemd side of things it should enable this sysctl and for
older kernels what should it do?
Supposing the change is backported via -stable, it can try to set the
sysctl on all kernels. If the knob doesn't exist, the fix is not
there and nothing can be done about it.
The more I think about it, the more I think this should be a
per-insmod instance thing rather than a system-wide switch. Currently
the kernel param code doesn't allow a generic param outside the ones
specified by the module itself but adding support for something like
driver.async_load=1 shouldn't be too difficult, applying that to
existing systems shouldn't be much more difficult than a system-wide
switch, and it'd be siginificantly cleaner than fiddling with driver
blacklist.

Thanks.
--
tejun
Luis R. Rodriguez
2014-09-12 20:14:22 UTC
Permalink
Post by Tejun Heo
Post by Tejun Heo
Hello,
Post by Luis R. Rodriguez
On the systemd side of things it should enable this sysctl and for
older kernels what should it do?
Supposing the change is backported via -stable, it can try to set the
sysctl on all kernels. If the knob doesn't exist, the fix is not
there and nothing can be done about it.
The more I think about it, the more I think this should be a
per-insmod instance thing rather than a system-wide switch.
Agreed, a good use case that comes to mind would be systemd's
modules-load.d lists used by systemd services to require modules, the
hooks there however likely expect probe to complete as part of the
service, since the timeout is not applicable to these the synchronous
probe for them would be good while systemd would use async probe for
regular modules.
Post by Tejun Heo
Currently
the kernel param code doesn't allow a generic param outside the ones
specified by the module itself but adding support for something like
driver.async_load=1 shouldn't be too difficult, applying that to
existing systems shouldn't be much more difficult than a system-wide
switch, and it'd be siginificantly cleaner than fiddling with driver
blacklist.
Agreed.

Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Luis R. Rodriguez
2014-09-22 16:36:31 UTC
Permalink
On Mon, Sep 8, 2014 at 7:57 PM, Luis R. Rodriguez
Post by Luis R. Rodriguez
Post by Tejun Heo
Why do we care about the priority of probing tasks? Does that
actually make any meaningful difference? If so, how?
As I noted before -- I have yet to provide clear metrics but at least
changing both init paths + probe from finit_module() to kthread
certainly had a measurable time increase, I suspect using
queue_work(system_unbound_wq, async_probe_work) will make probe
slower. I'll get to these metrics this week.
The results are in and I'm glad to report my suspicions were incorrect
about kthread() being slower than queue_work(system_unbound_wq), it
actually works faster. Results will likely vary depending on
subsystems but in this particular case the cxgb4 driver was tested
requiring firmware loading and then without requiring firmware loading
and for these two types of driver loading all mechanisms make probe
take just about the same out of time. What was surprising was that
when firmware loading is required the amount of time it takes to run
probe does vary and quite considerably in terms of microseconds. The
discrepancies are by no means terrible... but should be considered if
one is thinking of large systems and if we do wish to optimize things
further and offer equivalent behavior, specially when probing multiple
devices with the same driver. The method used to collect the amount of
time for probe was to use:

ktime_t calltime, delta, rettime;
calltime = ktime_get();
driver_attach();
rettime = ktime_get();
delta = ktime_sub(rettime, calltime);
duration = (unsigned long long) ktime_to_ns(delta) >> 10;

And then print that time of microsecond out right after it finishes,
whether that be through the default kernel synchronous run or the
async runs.

The collection and testing was then done by Santosh. Details of the
collections are at:

https://bugzilla.novell.com/show_bug.cgi?id=877622

The summary:

The driver actually probed 2 cards in the tests so we don't have
results for 1 card, the kernel serially calls probe for each device so
to get the amount of time for one run lets just divide the results by
2. For each strategy there is the requirement of using firmware and a
run where no firmware loading is required. The results for both cards
are:

=====================================================================|
strategy fw (usec) no-fw (usec) |
---------------------------------------------------------------------|
synchronous 48945138 2615126 |
kthread 50132831 2619737 |
queue_work(system_unbound_wq) 49827323 2615262 |
---------------------------------------------------------------------|

For one device then that comes out to:

=====================================================================|
strategy fw (usec) no-fw (usec) |
---------------------------------------------------------------------|
synchronous 24472569 1307563 |
kthread 25066415.5 1309868.5 |
queue_work(system_unbound_wq) 24913661.5 1307631 |
---------------------------------------------------------------------|

Converting that to seconds:

=====================================================================|
strategy fw (s) no-fw (s) |
---------------------------------------------------------------------|
synchronous 24.47 1.31 |
kthread 25.07 1.31 |
queue_work(system_unbound_wq) 24.91 1.31 |
---------------------------------------------------------------------|

Graph friendly versions of the results for probe of 1 device:

Probe with firmware:

Loading Image...

Probe without firmware:

Loading Image...

Luis
Tom Gundersen
2014-09-10 05:13:41 UTC
Permalink
On Tue, Sep 9, 2014 at 3:26 AM, Luis R. Rodriguez
Post by Luis R. Rodriguez
Post by Tejun Heo
Post by Tejun Heo
I'm not too convinced this is such a difficult problem to figure out.
We already have most of logic in place and the only thing missing is
how to switch it. Wouldn't something like the following work?
* Add a sysctl knob to enable asynchronous device probing on module
load and enable asynchronous probing globally if the knob is set.
Alternatively, add a module-generic param "async_probe" or whatever
and use that to switch the behavior should work too. I don't know
which way is better but either should work fine.
I take it by this you meant a generic system-wide sysctl or kernel cmd
line option to enable this for al drivers?
If the expectation is that this feature should be enabled
unconditionally for all systemd systems, wouldn't it make more sense
to make it a Kconfig option (possibly overridable from the kernel
commandline in case that makes testing simpler)?

Cheers,

Tom
James Bottomley
2014-09-09 05:38:29 UTC
Permalink
Post by Tejun Heo
Hello, Luis.
Post by Luis R. Rodriguez
Post by Tejun Heo
I have no idea how the selection should be. It could be per-insmod or
maybe just a system-wide flag with explicit exceptions marked on
drivers is good enough. I don't know.
Its perfectly understandable if we don't know what path to take yet
and its also understandable for it to take time to figure out --
meanwhile though systemd already has merged a policy of a 30 second
I'm not too convinced this is such a difficult problem to figure out.
We already have most of logic in place and the only thing missing is
how to switch it. Wouldn't something like the following work?
* Add a sysctl knob to enable asynchronous device probing on module
load and enable asynchronous probing globally if the knob is set.
* Identify cases which can't be asynchronous and make them
synchronous. e.g. keep who's doing request_module() and avoid
asynchronous probing if current is probing one of those.
What's wrong with just fixing systemd? Arbitrary timeouts in init
scripts for system bring up are plain wrong ... I thought we had this
sorted out ten years ago when we were first having the arguments about
how long to wait for root; I'm surprised it's coming back again.

If we want to sort out some sync/async mechanism for probing devices, as
an agreement between the init systems and the kernel, that's fine, but
its a to-be negotiated enhancement. For the current bug fix, just fix
the component that broke ... which would be systemd.

James
Luis R. Rodriguez
2014-09-09 19:16:12 UTC
Permalink
On Mon, Sep 8, 2014 at 10:38 PM, James Bottomley
Post by James Bottomley
Post by Tejun Heo
Hello, Luis.
Post by Luis R. Rodriguez
Post by Tejun Heo
I have no idea how the selection should be. It could be per-insmod or
maybe just a system-wide flag with explicit exceptions marked on
drivers is good enough. I don't know.
Its perfectly understandable if we don't know what path to take yet
and its also understandable for it to take time to figure out --
meanwhile though systemd already has merged a policy of a 30 second
I'm not too convinced this is such a difficult problem to figure out.
We already have most of logic in place and the only thing missing is
how to switch it. Wouldn't something like the following work?
* Add a sysctl knob to enable asynchronous device probing on module
load and enable asynchronous probing globally if the knob is set.
* Identify cases which can't be asynchronous and make them
synchronous. e.g. keep who's doing request_module() and avoid
asynchronous probing if current is probing one of those.
What's wrong with just fixing systemd? Arbitrary timeouts in init
scripts for system bring up are plain wrong ... I thought we had this
sorted out ten years ago when we were first having the arguments about
how long to wait for root; I'm surprised it's coming back again.
By design it seems systemd should not allow worker processes to block
indefinitely and in fact it currently uses the same timeout for all
types of worker processes. I last recommended a multiplier to at least
allow systemd to distinguish and allow us to modify the timeout based
on the type of process by using an enum used to classify these, kmod
for example would be one type of command:

http://lists.freedesktop.org/archives/systemd-devel/2014-August/021852.html

This was deemed to introduce unnecessary complexity, but I believe
this was before we realized that the timeout was penalizing kmod usage
unfairly given that the original assumption that it was just init that
should be penalized was incorrect given that we batch both init +
probe together. I have been relaying updates back on that thread as we
move along with this discussion on the issues found with the timeout,
but haven't gotten feedback yet as to which path folks on systemd
would like to take in light of recent discussions / clarifications.
Perhaps your arguments might help folks here reconsider things a bit
as well.

If we want *tight* integration between init system / kernel these
discussions are necessary not only when we find issues but also should
be part of the design phase for major changes.
Post by James Bottomley
If we want to sort out some sync/async mechanism for probing devices, as
an agreement between the init systems and the kernel, that's fine, but
its a to-be negotiated enhancement.
Unfortunately as Tejun notes the train has left which already made
assumptions on this. I'm afraid distributions that want to avoid this
sigkill at least on the kernel front will have to work around this
issue either on systemd by increasing the default timeout which is now
possible thanks to Hannes' changes or by some other means such as the
combination of a modified non-chatty version of this patch + a check
at the end of load_module() as mentioned earlier on these threads.
Post by James Bottomley
For the current bug fix, just fix the component that broke ... which would be systemd.
For new systems it seems the proposed fix is to have systemd tell the
kernel what it thought it should be seeing and that is all pure async
probes through a sysctl, and then we'd do async probe on all modules
unless a driver is specifically flagged with a need to run synchronous
(we'll enable this for request_firmware() users for example to start
off with).

Luis
James Bottomley
2014-09-09 19:35:46 UTC
Permalink
Post by Luis R. Rodriguez
On Mon, Sep 8, 2014 at 10:38 PM, James Bottomley
Post by James Bottomley
If we want to sort out some sync/async mechanism for probing devices, as
an agreement between the init systems and the kernel, that's fine, but
its a to-be negotiated enhancement.
Unfortunately as Tejun notes the train has left which already made
assumptions on this.
Well, that's why it's a bug. It's a material regression impacting
users.
Post by Luis R. Rodriguez
I'm afraid distributions that want to avoid this
sigkill at least on the kernel front will have to work around this
issue either on systemd by increasing the default timeout which is now
possible thanks to Hannes' changes or by some other means such as the
combination of a modified non-chatty version of this patch + a check
at the end of load_module() as mentioned earlier on these threads.
Increasing the default timeout in systemd seems like the obvious bug fix
to me. If the patch exists already, having distros that want it use it
looks to be correct ... not every bug is a kernel bug, after all.

Negotiating a probe vs init split for drivers is fine too, but it's a
longer term thing rather than a bug fix.
Post by Luis R. Rodriguez
Post by James Bottomley
For the current bug fix, just fix the component that broke ... which would be systemd.
For new systems it seems the proposed fix is to have systemd tell the
kernel what it thought it should be seeing and that is all pure async
probes through a sysctl, and then we'd do async probe on all modules
unless a driver is specifically flagged with a need to run synchronous
(we'll enable this for request_firmware() users for example to start
off with).
I don't have very strong views on this one. However, I've got to say
from a systems point of view that if the desire is to flag when the
module is having problems, probing and initializing synchronously in a
thread spawned by init which the init process can watchdog and thus can
flash up warning messages seems to be more straightforwards than an
elaborate asynchronous mechanism with completion signalling which
achieves the same thing in a more complicated (and thus bug prone)
fashion.

James
Luis R. Rodriguez
2014-09-09 20:45:30 UTC
Permalink
On Tue, Sep 9, 2014 at 12:35 PM, James Bottomley
Post by James Bottomley
Post by Luis R. Rodriguez
On Mon, Sep 8, 2014 at 10:38 PM, James Bottomley
Post by James Bottomley
If we want to sort out some sync/async mechanism for probing devices, as
an agreement between the init systems and the kernel, that's fine, but
its a to-be negotiated enhancement.
Unfortunately as Tejun notes the train has left which already made
assumptions on this.
Well, that's why it's a bug. It's a material regression impacting
users.
Indeed. I believe the issue with this regression however was that the
original commit e64fae55 (January 2012) was only accepted by *kernel
folks* to be a real regression until recently. More than two years
have gone by on growing design and assumptions on top of that original
commit. I'm not sure if *systemd folks* yet believe its was a design
regression?
Post by James Bottomley
Post by Luis R. Rodriguez
I'm afraid distributions that want to avoid this
sigkill at least on the kernel front will have to work around this
issue either on systemd by increasing the default timeout which is now
possible thanks to Hannes' changes or by some other means such as the
combination of a modified non-chatty version of this patch + a check
at the end of load_module() as mentioned earlier on these threads.
Increasing the default timeout in systemd seems like the obvious bug fix
to me. If the patch exists already, having distros that want it use it
looks to be correct ... not every bug is a kernel bug, after all.
Its merged upstream on systemd now, along with a few fixes on top of
it. I also see Kay merged a change to the default timeout to 60 second
on August 30. Its unclear if these discussions had any impact on that
decision or if that was just because udev firmware loading got now
ripped out. I'll note that the new 60 second timeout wouldn't suffice
for cxgb4 even if it didn't do firmware loading, its probe takes over
one full minute.
Post by James Bottomley
Negotiating a probe vs init split for drivers is fine too, but it's a
longer term thing rather than a bug fix.
Indeed. What I proposed with a multiplier for the timeout for the
different types of built in commands was deemed complex but saw no
alternatives proposed despite my interest to work on one and
clarifications noted that this was a design regression. Not quite sure
what else I could have done here. I'm interested in learning what the
better approach is for the future as if we want to marry init + kernel
we need a smooth way for us to discuss design without getting worked
up about it, or taking it personal. I really want this to work as I
personally like systemd so far.
Post by James Bottomley
Post by Luis R. Rodriguez
Post by James Bottomley
For the current bug fix, just fix the component that broke ... which would be systemd.
For new systems it seems the proposed fix is to have systemd tell the
kernel what it thought it should be seeing and that is all pure async
probes through a sysctl, and then we'd do async probe on all modules
unless a driver is specifically flagged with a need to run synchronous
(we'll enable this for request_firmware() users for example to start
off with).
I don't have very strong views on this one. However, I've got to say
from a systems point of view that if the desire is to flag when the
module is having problems, probing and initializing synchronously in a
thread spawned by init which the init process can watchdog and thus can
flash up warning messages seems to be more straightforwards
Indeed however it was not understood that module loading did init +
probe synchrounously, and indeed what you recommend is also what I was
hoping systemd *should do* instead of a hard sigkill at the default
timeout.
Post by James Bottomley
than an
elaborate asynchronous mechanism with completion signalling which
achieves the same thing in a more complicated (and thus bug prone)
fashion.
I couldn't be in any more agreement with you. It takes two to tango though.

Luis
Tom Gundersen
2014-09-10 06:46:34 UTC
Permalink
On Tue, Sep 9, 2014 at 10:45 PM, Luis R. Rodriguez
Post by Luis R. Rodriguez
On Tue, Sep 9, 2014 at 12:35 PM, James Bottomley
Post by James Bottomley
Post by Luis R. Rodriguez
On Mon, Sep 8, 2014 at 10:38 PM, James Bottomley
Post by James Bottomley
If we want to sort out some sync/async mechanism for probing devices, as
an agreement between the init systems and the kernel, that's fine, but
its a to-be negotiated enhancement.
Unfortunately as Tejun notes the train has left which already made
assumptions on this.
Well, that's why it's a bug. It's a material regression impacting
users.
Indeed. I believe the issue with this regression however was that the
original commit e64fae55 (January 2012) was only accepted by *kernel
folks* to be a real regression until recently.
Just for the record, this only caused user-visible problems after
kernel commit 786235ee (November 2013), right?
Post by Luis R. Rodriguez
More than two years
have gone by on growing design and assumptions on top of that original
commit. I'm not sure if *systemd folks* yet believe its was a design
regression?
I don't think so. udev should not allow its workers to run for an
unbounded length of time. Whether the upper bound should be 30, 60,
180 seconds or something else is up for debate (currently it is 60,
but if that is too short for some drivers we could certainly revisit
that). Moreover, it seems from this discussion that the aim is (still)
that insmod should be near-instantaneous (i.e., not wait for probe),
so it seems to me that the basic design is correct and all we need is
some temporary work-around and a way to better detect misbehaving
drivers?
Post by Luis R. Rodriguez
Post by James Bottomley
Post by Luis R. Rodriguez
I'm afraid distributions that want to avoid this
sigkill at least on the kernel front will have to work around this
issue either on systemd by increasing the default timeout which is now
possible thanks to Hannes' changes or by some other means such as the
combination of a modified non-chatty version of this patch + a check
at the end of load_module() as mentioned earlier on these threads.
Increasing the default timeout in systemd seems like the obvious bug fix
to me. If the patch exists already, having distros that want it use it
looks to be correct ... not every bug is a kernel bug, after all.
Its merged upstream on systemd now, along with a few fixes on top of
it. I also see Kay merged a change to the default timeout to 60 second
on August 30. Its unclear if these discussions had any impact on that
decision or if that was just because udev firmware loading got now
ripped out. I'll note that the new 60 second timeout wouldn't suffice
for cxgb4 even if it didn't do firmware loading, its probe takes over
one full minute.
Post by James Bottomley
Negotiating a probe vs init split for drivers is fine too, but it's a
longer term thing rather than a bug fix.
Indeed. What I proposed with a multiplier for the timeout for the
different types of built in commands was deemed complex but saw no
alternatives proposed despite my interest to work on one and
clarifications noted that this was a design regression. Not quite sure
what else I could have done here. I'm interested in learning what the
better approach is for the future as if we want to marry init + kernel
we need a smooth way for us to discuss design without getting worked
up about it, or taking it personal. I really want this to work as I
personally like systemd so far.
How about this: keep the timeout global, but also introduce a
(relatively short, say 10 or 15 seconds) timeout after which a warning
is printed. Even if nothing is actually killed, having workers (be it
insmod or something else) take longer than a couple of seconds is
likely a sign that something is seriously off somewhere.

Cheers,

Tom
Ceriel Jacobs
2014-09-10 10:07:48 UTC
Permalink
Post by Tom Gundersen
Post by Luis R. Rodriguez
Indeed. What I proposed with a multiplier for the timeout for the
different types of built in commands was deemed complex but saw no
alternatives proposed despite my interest to work on one and
clarifications noted that this was a design regression. Not quite sure
what else I could have done here. I'm interested in learning what the
better approach is for the future as if we want to marry init + kernel
we need a smooth way for us to discuss design without getting worked
up about it, or taking it personal. I really want this to work as I
personally like systemd so far.
How about this: keep the timeout global, but also introduce a
(relatively short, say 10 or 15 seconds) timeout after which a warning
is printed. Even if nothing is actually killed, having workers (be it
insmod or something else) take longer than a couple of seconds is
likely a sign that something is seriously off somewhere.
I don't agree with the statement that something is seriously off when it
takes more then 10 to 15 seconds.

When probing only one hard disk drive, then I do agree that something is
seriously off after 10 to 15 seconds.

When probing a SAS bus with one hundred hard disk drives in standby
mode, then I do expect that to take longer then 10 to 15 seconds.
James Bottomley
2014-09-10 13:31:41 UTC
Permalink
Post by Ceriel Jacobs
Post by Tom Gundersen
Post by Luis R. Rodriguez
Indeed. What I proposed with a multiplier for the timeout for the
different types of built in commands was deemed complex but saw no
alternatives proposed despite my interest to work on one and
clarifications noted that this was a design regression. Not quite sure
what else I could have done here. I'm interested in learning what the
better approach is for the future as if we want to marry init + kernel
we need a smooth way for us to discuss design without getting worked
up about it, or taking it personal. I really want this to work as I
personally like systemd so far.
How about this: keep the timeout global, but also introduce a
(relatively short, say 10 or 15 seconds) timeout after which a warning
is printed. Even if nothing is actually killed, having workers (be it
insmod or something else) take longer than a couple of seconds is
likely a sign that something is seriously off somewhere.
I don't agree with the statement that something is seriously off when it
takes more then 10 to 15 seconds.
When probing only one hard disk drive, then I do agree that something is
seriously off after 10 to 15 seconds.
Really? We keep explaining that arbitrary times are wrong. A while ago
the Adaptec driver used to use 15s as its bus settle time after the
initial reset (it's now a Kconfig variable set at 5s) and a Parallel bus
takes a minimum of 4s to scan and has to be done sequentially. If any
probed device is having difficulty, that can escalate way beyond this
into the tens to hundreds of seconds. If your root disk is on it,
you're waiting or not booting.
Post by Ceriel Jacobs
When probing a SAS bus with one hundred hard disk drives in standby
mode, then I do expect that to take longer then 10 to 15 seconds.
Good luck with that even on SAS if you have a lot of expanders.

For an installed system, you know what you need (usually root and
possibly one other disc like /home), so you spawn all the insertions
asynchronously and then wait for just the devices you need them but,
since the alternative is panic when init isn't found, this wait better
be quite long (if not forever, given the consequence is guaranteed
failure). Everything else can be async, but, as I've pointed out
before, it can be async in user space (fire and forget) instead of the
kernel.

James
Luis R. Rodriguez
2014-09-10 21:10:37 UTC
Permalink
Tom, thanks for reviewing this! My reply below!
Post by Tom Gundersen
On Tue, Sep 9, 2014 at 10:45 PM, Luis R. Rodriguez
Post by Luis R. Rodriguez
On Tue, Sep 9, 2014 at 12:35 PM, James Bottomley
Post by James Bottomley
Post by Luis R. Rodriguez
On Mon, Sep 8, 2014 at 10:38 PM, James Bottomley
Post by James Bottomley
If we want to sort out some sync/async mechanism for probing devices, as
an agreement between the init systems and the kernel, that's fine, but
its a to-be negotiated enhancement.
Unfortunately as Tejun notes the train has left which already made
assumptions on this.
Well, that's why it's a bug. It's a material regression impacting
users.
Indeed. I believe the issue with this regression however was that the
original commit e64fae55 (January 2012) was only accepted by *kernel
folks* to be a real regression until recently.
Just for the record, this only caused user-visible problems after
kernel commit 786235ee (November 2013), right?
Another one was cxgb4:

https://bugzilla.novell.com/show_bug.cgi?id=877622

SLE12 does not yet have commit 786235ee merged. Benjamim did some hard
work to debug this and trace the kill down to systemd-udev. A debug
kernel build has been provided now to try to pick up exactly on the
place where the kill was received, but its at least clear this came
from systemd.
Post by Tom Gundersen
Post by Luis R. Rodriguez
More than two years
have gone by on growing design and assumptions on top of that original
commit. I'm not sure if *systemd folks* yet believe its was a design
regression?
I don't think so. udev should not allow its workers to run for an
unbounded length of time. Whether the upper bound should be 30, 60,
180 seconds or something else is up for debate (currently it is 60,
but if that is too short for some drivers we could certainly revisit
that).
That's the thing -- the timeout was put in place under the assumption
probe was asyncronous and its not, the driver core issues both module
init *and* probe together, the loader has to wait. That alone makes
the timeout a design flaw, and then systemd carried on top of that
design over two years after that. Its not systemd's fault, its just
that we never spoke about this as a design thing broadly and we should
have, and I will mention that even when the first issues creeped up,
the issue was still tossed back a driver problems. It was only until
recently that we realized that both init and probe run together that
we've been thinking about this problem differently. Systemd was trying
to ensure init on driver don't take long but its not init that is
taking long, its probe, and probe gets then penalized as the driver
core batches both init and probe synchronously before finishing module
loading. Furthermore as clarified by Tejun random userland is known to
exist that will wait indefinitely for module loading under the simple
assumption things *are done synchronously*, and its precisely why we
can't just blindly enable async probe upstream through a new driver
boolean as it can be unfair to this old userland. What is being
evaluated is to enable aync probe for *all* drivers through a new
general system-wide option. We cannot regress old userspace and
assumptions but we can create a new shiny universe.
Post by Tom Gundersen
Moreover, it seems from this discussion that the aim is (still)
that insmod should be near-instantaneous (i.e., not wait for probe),
The only reason that is being discussed is that systemd has not
accepted the timeout as a system design despite me pointing out the
original design flaw recently and at this point even if was accepted
as a design flaw it seems its too late. The approach taken to help
make all drivers async is simply an afterthought to give systemd what
it *thought* was in place, and it by no measure should be considered
the proper fix to the regression introduced by systemd, it may perhaps
be the right step long term for systemd systems given it goes with
what it assumed was there, but the timeout was flawed. Its not clear
if systemd can help with old kernels, it seems the ship has sailed and
there seems no options but for folks to work around that -- unless of
course some reasonable solution is found which doesn't break current
systemd design?
Post by Tom Gundersen
so it seems to me that the basic design is correct and all we need is
some temporary work-around and a way to better detect misbehaving
drivers?
As part of this series I addressed hunting for the "misbehaving
drivers" in-kernel as I saw no progress on the systemd side of things
to non-fatally detect "misbehaving drivers" despite my original RFCs
and request for advice. I quote "misbehaving drivers" as its a flawed
view to consider them misbehaving now in light of clarifications of
how the driver core works in that it batches both init and probe
together always and we can't be penalizing long probes due to the fact
long probes are simply fine. My patch to pick up "misbehaving drivers"
drivers on the kernel front by picking up systemd's signal was
reactive but it was also simply braindead given the same exact reasons
why systemd's original timeout was flawed. We want a general solution
and we don't want to work around the root cause, in this case it was
systemd's assumption on how drivers work.

Keep in mind that the above just addresses kmod built-in cmd on
systemd, its where the timeout was introduced but as has been
clarified here assuming the same timeout on *all* other built-in
likely is likely pretty flawed as well and this does concern me. Its
why I mentioned that more than two years have gone by now on growing
design and assumptions on top of that original commit and its why its
hard for systemd to consider an alternative.
Post by Tom Gundersen
Post by Luis R. Rodriguez
Post by James Bottomley
Post by Luis R. Rodriguez
I'm afraid distributions that want to avoid this
sigkill at least on the kernel front will have to work around this
issue either on systemd by increasing the default timeout which is now
possible thanks to Hannes' changes or by some other means such as the
combination of a modified non-chatty version of this patch + a check
at the end of load_module() as mentioned earlier on these threads.
Increasing the default timeout in systemd seems like the obvious bug fix
to me. If the patch exists already, having distros that want it use it
looks to be correct ... not every bug is a kernel bug, after all.
Its merged upstream on systemd now, along with a few fixes on top of
it. I also see Kay merged a change to the default timeout to 60 second
on August 30. Its unclear if these discussions had any impact on that
decision or if that was just because udev firmware loading got now
ripped out. I'll note that the new 60 second timeout wouldn't suffice
for cxgb4 even if it didn't do firmware loading, its probe takes over
one full minute.
Post by James Bottomley
Negotiating a probe vs init split for drivers is fine too, but it's a
longer term thing rather than a bug fix.
Indeed. What I proposed with a multiplier for the timeout for the
different types of built in commands was deemed complex but saw no
alternatives proposed despite my interest to work on one and
clarifications noted that this was a design regression. Not quite sure
what else I could have done here. I'm interested in learning what the
better approach is for the future as if we want to marry init + kernel
we need a smooth way for us to discuss design without getting worked
up about it, or taking it personal. I really want this to work as I
personally like systemd so far.
How about this: keep the timeout global, but also introduce a
(relatively short, say 10 or 15 seconds) timeout after which a warning
is printed.
That is something that I originally was looking forward to on systemd,
but here's the thing once that warning comes up -- what would we do
with it? This patch addresses this warning in-kernel and the idea was
that we'd then peg an async_probe bool as true on the driver as a fix,
that was decided to be silly given all the above. These drivers are
actually not misbehaving and it would be even more incorrect to try to
"fix" them by making them run asynchronously. In fact for some old
storage drivers it may even be the worst thing to do given the
possible slew of userland deployment and scripts which assume things
*are* synchronous.
Post by Tom Gundersen
Even if nothing is actually killed, having workers (be it
insmod or something else) take longer than a couple of seconds is
likely a sign that something is seriously off somewhere.
Probe can take a long time and that's fine, so for kmod the current
assumption is flawed. If we had an option to async probe all drivers
then perhaps this kmod timeout *might be reasonable*, and even then I
do recommend for a clear warning that can be collected on logs on its
first iteration rather than sigkilling, only after a whlie should
sigkilling be done really. If systemd can deal with module loading in
the background for drivers that take a long time and warning on that
intsead of sigkiling it may be good start prior to enabling a default
sigkill on drivers. This is perhaps also true for other workers but
its not clear if this is a reasonable strategy for systemd.

Luis
Alexander E. Patrakov
2014-09-11 05:42:04 UTC
Permalink
Post by Luis R. Rodriguez
Tom, thanks for reviewing this! My reply below!
Post by Tom Gundersen
On Tue, Sep 9, 2014 at 10:45 PM, Luis R. Rodriguez
Post by Luis R. Rodriguez
On Tue, Sep 9, 2014 at 12:35 PM, James Bottomley
Post by James Bottomley
Post by Luis R. Rodriguez
On Mon, Sep 8, 2014 at 10:38 PM, James Bottomley
Post by James Bottomley
If we want to sort out some sync/async mechanism for probing devices, as
an agreement between the init systems and the kernel, that's fine, but
its a to-be negotiated enhancement.
Unfortunately as Tejun notes the train has left which already made
assumptions on this.
Well, that's why it's a bug. It's a material regression impacting
users.
Indeed. I believe the issue with this regression however was that the
original commit e64fae55 (January 2012) was only accepted by *kernel
folks* to be a real regression until recently.
Just for the record, this only caused user-visible problems after
kernel commit 786235ee (November 2013), right?
https://bugzilla.novell.com/show_bug.cgi?id=877622
SLE12 does not yet have commit 786235ee merged. Benjamim did some hard
work to debug this and trace the kill down to systemd-udev. A debug
kernel build has been provided now to try to pick up exactly on the
place where the kill was received, but its at least clear this came
from systemd.
Post by Tom Gundersen
Post by Luis R. Rodriguez
More than two years
have gone by on growing design and assumptions on top of that original
commit. I'm not sure if *systemd folks* yet believe its was a design
regression?
I don't think so. udev should not allow its workers to run for an
unbounded length of time. Whether the upper bound should be 30, 60,
180 seconds or something else is up for debate (currently it is 60,
but if that is too short for some drivers we could certainly revisit
that).
That's the thing -- the timeout was put in place under the assumption
probe was asyncronous and its not, the driver core issues both module
init *and* probe together, the loader has to wait. That alone makes
the timeout a design flaw, and then systemd carried on top of that
design over two years after that. Its not systemd's fault, its just
that we never spoke about this as a design thing broadly and we should
have, and I will mention that even when the first issues creeped up,
the issue was still tossed back a driver problems. It was only until
recently that we realized that both init and probe run together that
we've been thinking about this problem differently. Systemd was trying
to ensure init on driver don't take long but its not init that is
taking long, its probe, and probe gets then penalized as the driver
core batches both init and probe synchronously before finishing module
loading. Furthermore as clarified by Tejun random userland is known to
exist that will wait indefinitely for module loading under the simple
assumption things *are done synchronously*, and its precisely why we
can't just blindly enable async probe upstream through a new driver
boolean as it can be unfair to this old userland. What is being
evaluated is to enable aync probe for *all* drivers through a new
general system-wide option. We cannot regress old userspace and
assumptions but we can create a new shiny universe.
Post by Tom Gundersen
Moreover, it seems from this discussion that the aim is (still)
that insmod should be near-instantaneous (i.e., not wait for probe),
The only reason that is being discussed is that systemd has not
accepted the timeout as a system design despite me pointing out the
original design flaw recently and at this point even if was accepted
as a design flaw it seems its too late. The approach taken to help
make all drivers async is simply an afterthought to give systemd what
it *thought* was in place, and it by no measure should be considered
the proper fix to the regression introduced by systemd, it may perhaps
be the right step long term for systemd systems given it goes with
what it assumed was there, but the timeout was flawed. Its not clear
if systemd can help with old kernels, it seems the ship has sailed and
there seems no options but for folks to work around that -- unless of
course some reasonable solution is found which doesn't break current
systemd design?
Post by Tom Gundersen
so it seems to me that the basic design is correct and all we need is
some temporary work-around and a way to better detect misbehaving
drivers?
As part of this series I addressed hunting for the "misbehaving
drivers" in-kernel as I saw no progress on the systemd side of things
to non-fatally detect "misbehaving drivers" despite my original RFCs
and request for advice. I quote "misbehaving drivers" as its a flawed
view to consider them misbehaving now in light of clarifications of
how the driver core works in that it batches both init and probe
together always and we can't be penalizing long probes due to the fact
long probes are simply fine. My patch to pick up "misbehaving drivers"
drivers on the kernel front by picking up systemd's signal was
reactive but it was also simply braindead given the same exact reasons
why systemd's original timeout was flawed. We want a general solution
and we don't want to work around the root cause, in this case it was
systemd's assumption on how drivers work.
Keep in mind that the above just addresses kmod built-in cmd on
systemd, its where the timeout was introduced but as has been
clarified here assuming the same timeout on *all* other built-in
likely is likely pretty flawed as well and this does concern me. Its
why I mentioned that more than two years have gone by now on growing
design and assumptions on top of that original commit and its why its
hard for systemd to consider an alternative.
Post by Tom Gundersen
Post by Luis R. Rodriguez
Post by James Bottomley
Post by Luis R. Rodriguez
I'm afraid distributions that want to avoid this
sigkill at least on the kernel front will have to work around this
issue either on systemd by increasing the default timeout which is now
possible thanks to Hannes' changes or by some other means such as the
combination of a modified non-chatty version of this patch + a check
at the end of load_module() as mentioned earlier on these threads.
Increasing the default timeout in systemd seems like the obvious bug fix
to me. If the patch exists already, having distros that want it use it
looks to be correct ... not every bug is a kernel bug, after all.
Its merged upstream on systemd now, along with a few fixes on top of
it. I also see Kay merged a change to the default timeout to 60 second
on August 30. Its unclear if these discussions had any impact on that
decision or if that was just because udev firmware loading got now
ripped out. I'll note that the new 60 second timeout wouldn't suffice
for cxgb4 even if it didn't do firmware loading, its probe takes over
one full minute.
Post by James Bottomley
Negotiating a probe vs init split for drivers is fine too, but it's a
longer term thing rather than a bug fix.
Indeed. What I proposed with a multiplier for the timeout for the
different types of built in commands was deemed complex but saw no
alternatives proposed despite my interest to work on one and
clarifications noted that this was a design regression. Not quite sure
what else I could have done here. I'm interested in learning what the
better approach is for the future as if we want to marry init + kernel
we need a smooth way for us to discuss design without getting worked
up about it, or taking it personal. I really want this to work as I
personally like systemd so far.
How about this: keep the timeout global, but also introduce a
(relatively short, say 10 or 15 seconds) timeout after which a warning
is printed.
That is something that I originally was looking forward to on systemd,
but here's the thing once that warning comes up -- what would we do
with it? This patch addresses this warning in-kernel and the idea was
that we'd then peg an async_probe bool as true on the driver as a fix,
that was decided to be silly given all the above. These drivers are
actually not misbehaving and it would be even more incorrect to try to
"fix" them by making them run asynchronously. In fact for some old
storage drivers it may even be the worst thing to do given the
possible slew of userland deployment and scripts which assume things
*are* synchronous.
Post by Tom Gundersen
Even if nothing is actually killed, having workers (be it
insmod or something else) take longer than a couple of seconds is
likely a sign that something is seriously off somewhere.
Probe can take a long time and that's fine, so for kmod the current
assumption is flawed. If we had an option to async probe all drivers
then perhaps this kmod timeout *might be reasonable*, and even then I
do recommend for a clear warning that can be collected on logs on its
first iteration rather than sigkilling, only after a whlie should
sigkilling be done really. If systemd can deal with module loading in
the background for drivers that take a long time and warning on that
intsead of sigkiling it may be good start prior to enabling a default
sigkill on drivers. This is perhaps also true for other workers but
its not clear if this is a reasonable strategy for systemd.
Luis
Just two small remarks to the whole thread.

First, I am quite surprised that nobody brought up the argument that
module loading is serialized by the kernel. So, while pata-marvell on my
laptop does its dirty "wait-reset-wait-reset-work" thing, no other
module can be loaded. This prevention of loading other drivers is the
thing that slows down the boot.

Second, I am going to XDC2014, LinuxCon Europe and Plumbers. I will take
my laptop with me, feel free to see the situation firsthand or try
debugging patches.
--
Alexander E. Patrakov
Tom Gundersen
2014-09-11 21:43:28 UTC
Permalink
On Wed, Sep 10, 2014 at 11:10 PM, Luis R. Rodriguez
Post by Luis R. Rodriguez
Post by Tom Gundersen
Post by Luis R. Rodriguez
More than two years
have gone by on growing design and assumptions on top of that original
commit. I'm not sure if *systemd folks* yet believe its was a design
regression?
I don't think so. udev should not allow its workers to run for an
unbounded length of time. Whether the upper bound should be 30, 60,
180 seconds or something else is up for debate (currently it is 60,
but if that is too short for some drivers we could certainly revisit
that).
That's the thing -- the timeout was put in place under the assumption
probe was asyncronous and its not, the driver core issues both module
init *and* probe together, the loader has to wait. That alone makes
the timeout a design flaw, and then systemd carried on top of that
design over two years after that. Its not systemd's fault, its just
that we never spoke about this as a design thing broadly and we should
have, and I will mention that even when the first issues creeped up,
the issue was still tossed back a driver problems. It was only until
recently that we realized that both init and probe run together that
we've been thinking about this problem differently. Systemd was trying
to ensure init on driver don't take long but its not init that is
taking long, its probe, and probe gets then penalized as the driver
core batches both init and probe synchronously before finishing module
loading.
Just to clarify: udev/systemd is not trying to get into the business
of what the kernel does on finit_module(), we just need to make sure
that none of our workers stay around forever, which is why we have a
global timeout. If necessary we can bump this higher (as mentioned in
another thread I just bumped it to 180 secs), but we cannot abolish it
entirely.
Post by Luis R. Rodriguez
Furthermore as clarified by Tejun random userland is known to
exist that will wait indefinitely for module loading under the simple
assumption things *are done synchronously*, and its precisely why we
can't just blindly enable async probe upstream through a new driver
boolean as it can be unfair to this old userland. What is being
evaluated is to enable aync probe for *all* drivers through a new
general system-wide option. We cannot regress old userspace and
assumptions but we can create a new shiny universe.
How about simply introducing a new flag to finit_module() to indicate
that the caller does not care about asynchronicity. We could then pass
this from udev, but existing scripts calling modprobe/insmod will not
be affected.
Post by Luis R. Rodriguez
Post by Tom Gundersen
Moreover, it seems from this discussion that the aim is (still)
that insmod should be near-instantaneous (i.e., not wait for probe),
The only reason that is being discussed is that systemd has not
accepted the timeout as a system design despite me pointing out the
original design flaw recently and at this point even if was accepted
as a design flaw it seems its too late. The approach taken to help
make all drivers async is simply an afterthought to give systemd what
it *thought* was in place, and it by no measure should be considered
the proper fix to the regression introduced by systemd, it may perhaps
be the right step long term for systemd systems given it goes with
what it assumed was there, but the timeout was flawed. Its not clear
if systemd can help with old kernels, it seems the ship has sailed and
there seems no options but for folks to work around that -- unless of
course some reasonable solution is found which doesn't break current
systemd design?
If I read the git logs correctly the hard timeout was introduced in
April 2011, so reverting it now seems indeed not to help much with all
the running systems out there.
Post by Luis R. Rodriguez
As part of this series I addressed hunting for the "misbehaving
drivers" in-kernel as I saw no progress on the systemd side of things
to non-fatally detect "misbehaving drivers" despite my original RFCs
and request for advice. I quote "misbehaving drivers" as its a flawed
view to consider them misbehaving now in light of clarifications of
how the driver core works in that it batches both init and probe
together always and we can't be penalizing long probes due to the fact
long probes are simply fine. My patch to pick up "misbehaving drivers"
drivers on the kernel front by picking up systemd's signal was
reactive but it was also simply braindead given the same exact reasons
why systemd's original timeout was flawed. We want a general solution
and we don't want to work around the root cause, in this case it was
systemd's assumption on how drivers work.
Would your ongoing work to make probing asynchronous solve this
problem in the long-term? In the short-term I guess bumping the udev
timeout should be sufficient.
Post by Luis R. Rodriguez
Keep in mind that the above just addresses kmod built-in cmd on
systemd, its where the timeout was introduced but as has been
clarified here assuming the same timeout on *all* other built-in
likely is likely pretty flawed as well and this does concern me. Its
why I mentioned that more than two years have gone by now on growing
design and assumptions on top of that original commit and its why its
hard for systemd to consider an alternative.
All built-ins should be near-instantaneous. If they are not, that
needs to be fixed, or they should not be udev built-ins at all. I have
now added a warning to udev if any builtin-in takes more than a third
of the timeout, so hopefully any problems should be spotted early.
Post by Luis R. Rodriguez
Post by Tom Gundersen
Post by Luis R. Rodriguez
Post by James Bottomley
Post by Luis R. Rodriguez
I'm afraid distributions that want to avoid this
sigkill at least on the kernel front will have to work around this
issue either on systemd by increasing the default timeout which is now
possible thanks to Hannes' changes or by some other means such as the
combination of a modified non-chatty version of this patch + a check
at the end of load_module() as mentioned earlier on these threads.
Increasing the default timeout in systemd seems like the obvious bug fix
to me. If the patch exists already, having distros that want it use it
looks to be correct ... not every bug is a kernel bug, after all.
Its merged upstream on systemd now, along with a few fixes on top of
it. I also see Kay merged a change to the default timeout to 60 second
on August 30. Its unclear if these discussions had any impact on that
decision or if that was just because udev firmware loading got now
ripped out. I'll note that the new 60 second timeout wouldn't suffice
for cxgb4 even if it didn't do firmware loading, its probe takes over
one full minute.
Post by James Bottomley
Negotiating a probe vs init split for drivers is fine too, but it's a
longer term thing rather than a bug fix.
Indeed. What I proposed with a multiplier for the timeout for the
different types of built in commands was deemed complex but saw no
alternatives proposed despite my interest to work on one and
clarifications noted that this was a design regression. Not quite sure
what else I could have done here. I'm interested in learning what the
better approach is for the future as if we want to marry init + kernel
we need a smooth way for us to discuss design without getting worked
up about it, or taking it personal. I really want this to work as I
personally like systemd so far.
How about this: keep the timeout global, but also introduce a
(relatively short, say 10 or 15 seconds) timeout after which a warning
is printed.
That is something that I originally was looking forward to on systemd,
but here's the thing once that warning comes up -- what would we do
with it?
Short term: bump the timeout further. Long-term, hopefully the driver
(core) can be changed to avoid the problem.
Post by Luis R. Rodriguez
This patch addresses this warning in-kernel and the idea was
that we'd then peg an async_probe bool as true on the driver as a fix,
that was decided to be silly given all the above. These drivers are
actually not misbehaving and it would be even more incorrect to try to
"fix" them by making them run asynchronously. In fact for some old
storage drivers it may even be the worst thing to do given the
possible slew of userland deployment and scripts which assume things
*are* synchronous.
As mentioned above, it probably makes sense to switch on the
asynchronous behaviour only for a given call to finit_module(), and
not globally to avoid problems with userland assumptions.
Post by Luis R. Rodriguez
Post by Tom Gundersen
Even if nothing is actually killed, having workers (be it
insmod or something else) take longer than a couple of seconds is
likely a sign that something is seriously off somewhere.
Probe can take a long time and that's fine,
But isn't finit_module() taking a long time a serious problem given
that it means no other module can be loaded in parallel? Even if you
have some storage device which legitimately needs to take a couple of
minutes to probe, you probably still want your computer to boot and
get on with its other tasks whilst you wait... Or worse still, some
insignificant driver is broken and simply hangs in probe, but surely
you still want the rest of the system to boot?
Post by Luis R. Rodriguez
so for kmod the current
assumption is flawed. If we had an option to async probe all drivers
then perhaps this kmod timeout *might be reasonable*, and even then I
do recommend for a clear warning that can be collected on logs on its
first iteration rather than sigkilling, only after a whlie should
sigkilling be done really. If systemd can deal with module loading in
the background for drivers that take a long time and warning on that
intsead of sigkiling it may be good start prior to enabling a default
sigkill on drivers. This is perhaps also true for other workers but
its not clear if this is a reasonable strategy for systemd.
Luis R. Rodriguez
2014-09-11 22:26:11 UTC
Permalink
Post by Tom Gundersen
On Wed, Sep 10, 2014 at 11:10 PM, Luis R. Rodriguez
Post by Luis R. Rodriguez
Post by Tom Gundersen
Post by Luis R. Rodriguez
More than two years
have gone by on growing design and assumptions on top of that original
commit. I'm not sure if *systemd folks* yet believe its was a design
regression?
I don't think so. udev should not allow its workers to run for an
unbounded length of time. Whether the upper bound should be 30, 60,
180 seconds or something else is up for debate (currently it is 60,
but if that is too short for some drivers we could certainly revisit
that).
That's the thing -- the timeout was put in place under the assumption
probe was asyncronous and its not, the driver core issues both module
init *and* probe together, the loader has to wait. That alone makes
the timeout a design flaw, and then systemd carried on top of that
design over two years after that. Its not systemd's fault, its just
that we never spoke about this as a design thing broadly and we should
have, and I will mention that even when the first issues creeped up,
the issue was still tossed back a driver problems. It was only until
recently that we realized that both init and probe run together that
we've been thinking about this problem differently. Systemd was trying
to ensure init on driver don't take long but its not init that is
taking long, its probe, and probe gets then penalized as the driver
core batches both init and probe synchronously before finishing module
loading.
Just to clarify: udev/systemd is not trying to get into the business
of what the kernel does on finit_module(), we just need to make sure
that none of our workers stay around forever, which is why we have a
global timeout. If necessary we can bump this higher (as mentioned in
another thread I just bumped it to 180 secs), but we cannot abolish it
entirely.
180 seconds is certainly better than 30, but let me be clear here on
the extent to which the timeout at least for kmod built-in command can
be an issue. The driver core not only batches init and probe together
synchronously, it also runs probe for *all* devices that the device
driver can claim and all those series of probes run synchronously
within itself, that is bus_for_each_dev() runs synchronously on each
device. So, if a init takes 1 second and probe for each device takes
120 seconds and the system has 2 devices with the new timeout the
second device would not be successfully probed (and in fact I'm not
sure if this would kill the first).
Post by Tom Gundersen
Post by Luis R. Rodriguez
Furthermore as clarified by Tejun random userland is known to
exist that will wait indefinitely for module loading under the simple
assumption things *are done synchronously*, and its precisely why we
can't just blindly enable async probe upstream through a new driver
boolean as it can be unfair to this old userland. What is being
evaluated is to enable aync probe for *all* drivers through a new
general system-wide option. We cannot regress old userspace and
assumptions but we can create a new shiny universe.
How about simply introducing a new flag to finit_module() to indicate
that the caller does not care about asynchronicity. We could then pass
this from udev, but existing scripts calling modprobe/insmod will not
be affected.
Do you mean that you *do want asynchronicity*?
Post by Tom Gundersen
Post by Luis R. Rodriguez
Post by Tom Gundersen
Moreover, it seems from this discussion that the aim is (still)
that insmod should be near-instantaneous (i.e., not wait for probe),
The only reason that is being discussed is that systemd has not
accepted the timeout as a system design despite me pointing out the
original design flaw recently and at this point even if was accepted
as a design flaw it seems its too late. The approach taken to help
make all drivers async is simply an afterthought to give systemd what
it *thought* was in place, and it by no measure should be considered
the proper fix to the regression introduced by systemd, it may perhaps
be the right step long term for systemd systems given it goes with
what it assumed was there, but the timeout was flawed. Its not clear
if systemd can help with old kernels, it seems the ship has sailed and
there seems no options but for folks to work around that -- unless of
course some reasonable solution is found which doesn't break current
systemd design?
If I read the git logs correctly the hard timeout was introduced in
April 2011, so reverting it now seems indeed not to help much with all
the running systems out there.
yeah figured :(
Post by Tom Gundersen
Post by Luis R. Rodriguez
As part of this series I addressed hunting for the "misbehaving
drivers" in-kernel as I saw no progress on the systemd side of things
to non-fatally detect "misbehaving drivers" despite my original RFCs
and request for advice. I quote "misbehaving drivers" as its a flawed
view to consider them misbehaving now in light of clarifications of
how the driver core works in that it batches both init and probe
together always and we can't be penalizing long probes due to the fact
long probes are simply fine. My patch to pick up "misbehaving drivers"
drivers on the kernel front by picking up systemd's signal was
reactive but it was also simply braindead given the same exact reasons
why systemd's original timeout was flawed. We want a general solution
and we don't want to work around the root cause, in this case it was
systemd's assumption on how drivers work.
Would your ongoing work to make probing asynchronous solve this
problem in the long-term? In the short-term I guess bumping the udev
timeout should be sufficient.
That and the global flag / module param to specify the async desire
which would not regress old userspace. Probe afterall is the main
source of the issue.
Post by Tom Gundersen
Post by Luis R. Rodriguez
Keep in mind that the above just addresses kmod built-in cmd on
systemd, its where the timeout was introduced but as has been
clarified here assuming the same timeout on *all* other built-in
likely is likely pretty flawed as well and this does concern me. Its
why I mentioned that more than two years have gone by now on growing
design and assumptions on top of that original commit and its why its
hard for systemd to consider an alternative.
All built-ins should be near-instantaneous. If they are not, that
needs to be fixed, or they should not be udev built-ins at all. I have
now added a warning to udev if any builtin-in takes more than a third
of the timeout, so hopefully any problems should be spotted early.
Great thanks. Collecting these should be valuable and help being proactive.
Post by Tom Gundersen
Post by Luis R. Rodriguez
Post by Tom Gundersen
Post by Luis R. Rodriguez
Post by James Bottomley
Post by Luis R. Rodriguez
I'm afraid distributions that want to avoid this
sigkill at least on the kernel front will have to work around this
issue either on systemd by increasing the default timeout which is now
possible thanks to Hannes' changes or by some other means such as the
combination of a modified non-chatty version of this patch + a check
at the end of load_module() as mentioned earlier on these threads.
Increasing the default timeout in systemd seems like the obvious bug fix
to me. If the patch exists already, having distros that want it use it
looks to be correct ... not every bug is a kernel bug, after all.
Its merged upstream on systemd now, along with a few fixes on top of
it. I also see Kay merged a change to the default timeout to 60 second
on August 30. Its unclear if these discussions had any impact on that
decision or if that was just because udev firmware loading got now
ripped out. I'll note that the new 60 second timeout wouldn't suffice
for cxgb4 even if it didn't do firmware loading, its probe takes over
one full minute.
Post by James Bottomley
Negotiating a probe vs init split for drivers is fine too, but it's a
longer term thing rather than a bug fix.
Indeed. What I proposed with a multiplier for the timeout for the
different types of built in commands was deemed complex but saw no
alternatives proposed despite my interest to work on one and
clarifications noted that this was a design regression. Not quite sure
what else I could have done here. I'm interested in learning what the
better approach is for the future as if we want to marry init + kernel
we need a smooth way for us to discuss design without getting worked
up about it, or taking it personal. I really want this to work as I
personally like systemd so far.
How about this: keep the timeout global, but also introduce a
(relatively short, say 10 or 15 seconds) timeout after which a warning
is printed.
That is something that I originally was looking forward to on systemd,
but here's the thing once that warning comes up -- what would we do
with it?
Short term: bump the timeout further. Long-term, hopefully the driver
(core) can be changed to avoid the problem.
Fine by me, although I think some folks still have concerns with the
sigkill completely. But not sure if we escape it now.
Post by Tom Gundersen
Post by Luis R. Rodriguez
This patch addresses this warning in-kernel and the idea was
that we'd then peg an async_probe bool as true on the driver as a fix,
that was decided to be silly given all the above. These drivers are
actually not misbehaving and it would be even more incorrect to try to
"fix" them by making them run asynchronously. In fact for some old
storage drivers it may even be the worst thing to do given the
possible slew of userland deployment and scripts which assume things
*are* synchronous.
As mentioned above, it probably makes sense to switch on the
asynchronous behaviour only for a given call to finit_module(), and
not globally to avoid problems with userland assumptions.
Sure that's one way.
Post by Tom Gundersen
Post by Luis R. Rodriguez
Post by Tom Gundersen
Even if nothing is actually killed, having workers (be it
insmod or something else) take longer than a couple of seconds is
likely a sign that something is seriously off somewhere.
Probe can take a long time and that's fine,
But isn't finit_module() taking a long time a serious problem given
that it means no other module can be loaded in parallel?
Indeed but having a desire to make the init() complete fast is
different than the desire to have the combination of both init and
probe fast synchronously. If userspace wants init to be fast and let
probe be async then userspace has no option but to deal with the fact
that async probe will be async, and it should then use other methods
to match any dependencies if its doing that itself. For example
networking should not kick off after a network driver is loaded but
rather one the device creeps up on udev. We should be good with
networking dealing with this correctly today but not sure about other
subsystems. depmod should be able to load the required modules in
order and if bus drivers work right then probe of the remnant devices
should happen asynchronously. The one case I can think of that is a
bit different is modules-load.d things but those *do not rely on the
timeout*, but are loaded prior to a service requirement. Note though
that if those modules had probe and they then run async'd then systemd
service would probably need to consider that the requirements may not
be there until later. If this is not carefully considered that could
introduce regression to users of modules-load.d when async probe is
fully deployed. The same applies to systemd making assumptions of kmod
loading a module and a dependency being complete as probe would have
run it before.
Post by Tom Gundersen
Even if you
have some storage device which legitimately needs to take a couple of
minutes to probe, you probably still want your computer to boot and
get on with its other tasks whilst you wait... Or worse still, some
insignificant driver is broken and simply hangs in probe, but surely
you still want the rest of the system to boot?
Agreed, I believe one concern here lies in on whether or not userspace
is properly equipped to deal with the requirements on module loading
doing async probing and that possibly failing. Perhaps systemd might
think all userspace is ready for that but are we sure that's the case?
Another obvious issue was if the driver was a storage driver and your
boot depended upon it. If it takes a while we kill it and you can't
boot, no bueno. If systemd can avoid those situations that'd be nice.
That was the source of the first major issue reported by Joseph.

Chattiness on issues before the timeout should help a lot, we should
start collecting these somehow. These should be collected and
addressed. If we really want to be good on this we should put a bit of
effort on monitoring these and not being reactive.

Luis
Tom Gundersen
2014-09-12 05:48:16 UTC
Permalink
On Fri, Sep 12, 2014 at 12:26 AM, Luis R. Rodriguez
Post by Luis R. Rodriguez
Post by Tom Gundersen
How about simply introducing a new flag to finit_module() to indicate
that the caller does not care about asynchronicity. We could then pass
this from udev, but existing scripts calling modprobe/insmod will not
be affected.
Do you mean that you *do want asynchronicity*?
Precisely, udev would opt-in, but existing scripts etc would not.
Post by Luis R. Rodriguez
Post by Tom Gundersen
But isn't finit_module() taking a long time a serious problem given
that it means no other module can be loaded in parallel?
Indeed but having a desire to make the init() complete fast is
different than the desire to have the combination of both init and
probe fast synchronously.
I guess no one is arguing that probe should somehow be required to be
Post by Luis R. Rodriguez
If userspace wants init to be fast and let
probe be async then userspace has no option but to deal with the fact
that async probe will be async, and it should then use other methods
to match any dependencies if its doing that itself.
Correct. And this therefore likely needs to be opt-in behaviour per
finit_module() invocation to avoid breaking old assumptions.
Post by Luis R. Rodriguez
For example
networking should not kick off after a network driver is loaded but
rather one the device creeps up on udev. We should be good with
networking dealing with this correctly today but not sure about other
subsystems. depmod should be able to load the required modules in
order and if bus drivers work right then probe of the remnant devices
should happen asynchronously. The one case I can think of that is a
bit different is modules-load.d things but those *do not rely on the
timeout*, but are loaded prior to a service requirement. Note though
that if those modules had probe and they then run async'd then systemd
service would probably need to consider that the requirements may not
be there until later. If this is not carefully considered that could
introduce regression to users of modules-load.d when async probe is
fully deployed. The same applies to systemd making assumptions of kmod
loading a module and a dependency being complete as probe would have
run it before.
Yeah, these all needs to be considered when deciding whether or not to
enable async in each specific case.
Post by Luis R. Rodriguez
I believe one concern here lies in on whether or not userspace
is properly equipped to deal with the requirements on module loading
doing async probing and that possibly failing. Perhaps systemd might
think all userspace is ready for that but are we sure that's the case?
There almost certainly are custom things out there relying on the
synchronous behaviour, but if we make it opt-in we should not have a
problem.

Cheers,

Tom
Luis R. Rodriguez
2014-09-12 20:09:57 UTC
Permalink
Post by Tom Gundersen
On Fri, Sep 12, 2014 at 12:26 AM, Luis R. Rodriguez
Post by Luis R. Rodriguez
Post by Tom Gundersen
How about simply introducing a new flag to finit_module() to indicate
that the caller does not care about asynchronicity. We could then pass
this from udev, but existing scripts calling modprobe/insmod will not
be affected.
Do you mean that you *do want asynchronicity*?
Precisely, udev would opt-in, but existing scripts etc would not.
Sure that's the other alternative that Tejun was mentioning.
Post by Tom Gundersen
Post by Luis R. Rodriguez
Post by Tom Gundersen
But isn't finit_module() taking a long time a serious problem given
that it means no other module can be loaded in parallel?
Indeed but having a desire to make the init() complete fast is
different than the desire to have the combination of both init and
probe fast synchronously.
I guess no one is arguing that probe should somehow be required to be
Post by Luis R. Rodriguez
If userspace wants init to be fast and let
probe be async then userspace has no option but to deal with the fact
that async probe will be async, and it should then use other methods
to match any dependencies if its doing that itself.
Correct. And this therefore likely needs to be opt-in behaviour per
finit_module() invocation to avoid breaking old assumptions.
Sure.
Post by Tom Gundersen
Post by Luis R. Rodriguez
For example
networking should not kick off after a network driver is loaded but
rather one the device creeps up on udev. We should be good with
networking dealing with this correctly today but not sure about other
subsystems. depmod should be able to load the required modules in
order and if bus drivers work right then probe of the remnant devices
should happen asynchronously. The one case I can think of that is a
bit different is modules-load.d things but those *do not rely on the
timeout*, but are loaded prior to a service requirement. Note though
that if those modules had probe and they then run async'd then systemd
service would probably need to consider that the requirements may not
be there until later. If this is not carefully considered that could
introduce regression to users of modules-load.d when async probe is
fully deployed. The same applies to systemd making assumptions of kmod
loading a module and a dependency being complete as probe would have
run it before.
Yeah, these all needs to be considered when deciding whether or not to
enable async in each specific case.
Yes and come to think of it I'd recommend opting out of async
functionality for modules-load.d given that it does *not* hooked with
the timeout and there is a good chances its users likely do want to
wait for probe to run at this point.

Given this I also am inclined now for the per module request to be
async or not (default) from userspace. The above would be a good
example starting use case.
Post by Tom Gundersen
Post by Luis R. Rodriguez
I believe one concern here lies in on whether or not userspace
is properly equipped to deal with the requirements on module loading
doing async probing and that possibly failing. Perhaps systemd might
think all userspace is ready for that but are we sure that's the case?
There almost certainly are custom things out there relying on the
synchronous behaviour, but if we make it opt-in we should not have a
problem.
Indeed.

BTW as for the cxgb4 device driver it fails to load because it relies
on get_vpd_params() on probe, that end sup calling
pci_vpd_pci22_wait() which will fail if if
fatal_signal_pending(current). This is an example now completely
unrelated to the OOM series, and any other uses of
fatal_signal_pending(current) should trigger similar failures on
device drivers.

Luis
Anatol Pomozov
2014-10-10 21:54:14 UTC
Permalink
Hi

On Fri, Sep 12, 2014 at 1:09 PM, Luis R. Rodriguez
Post by Luis R. Rodriguez
Post by Tom Gundersen
On Fri, Sep 12, 2014 at 12:26 AM, Luis R. Rodriguez
Post by Luis R. Rodriguez
Post by Tom Gundersen
How about simply introducing a new flag to finit_module() to indicate
that the caller does not care about asynchronicity. We could then pass
this from udev, but existing scripts calling modprobe/insmod will not
be affected.
Do you mean that you *do want asynchronicity*?
Precisely, udev would opt-in, but existing scripts etc would not.
Sure that's the other alternative that Tejun was mentioning.
Post by Tom Gundersen
Post by Luis R. Rodriguez
Post by Tom Gundersen
But isn't finit_module() taking a long time a serious problem given
that it means no other module can be loaded in parallel?
Indeed but having a desire to make the init() complete fast is
different than the desire to have the combination of both init and
probe fast synchronously.
I guess no one is arguing that probe should somehow be required to be
Post by Luis R. Rodriguez
If userspace wants init to be fast and let
probe be async then userspace has no option but to deal with the fact
that async probe will be async, and it should then use other methods
to match any dependencies if its doing that itself.
Correct. And this therefore likely needs to be opt-in behaviour per
finit_module() invocation to avoid breaking old assumptions.
Sure.
Post by Tom Gundersen
Post by Luis R. Rodriguez
For example
networking should not kick off after a network driver is loaded but
rather one the device creeps up on udev. We should be good with
networking dealing with this correctly today but not sure about other
subsystems. depmod should be able to load the required modules in
order and if bus drivers work right then probe of the remnant devices
should happen asynchronously. The one case I can think of that is a
bit different is modules-load.d things but those *do not rely on the
timeout*, but are loaded prior to a service requirement. Note though
that if those modules had probe and they then run async'd then systemd
service would probably need to consider that the requirements may not
be there until later. If this is not carefully considered that could
introduce regression to users of modules-load.d when async probe is
fully deployed. The same applies to systemd making assumptions of kmod
loading a module and a dependency being complete as probe would have
run it before.
Yeah, these all needs to be considered when deciding whether or not to
enable async in each specific case.
Yes and come to think of it I'd recommend opting out of async
functionality for modules-load.d given that it does *not* hooked with
the timeout and there is a good chances its users likely do want to
wait for probe to run at this point.
Given this I also am inclined now for the per module request to be
async or not (default) from userspace. The above would be a good
example starting use case.
Post by Tom Gundersen
Post by Luis R. Rodriguez
I believe one concern here lies in on whether or not userspace
is properly equipped to deal with the requirements on module loading
doing async probing and that possibly failing. Perhaps systemd might
think all userspace is ready for that but are we sure that's the case?
There almost certainly are custom things out there relying on the
synchronous behaviour, but if we make it opt-in we should not have a
problem.
We recently discussed this "timeout module loading" issue in Arch IRC
and here are few more ideas:

1) Why not to make the timeout configurable through config file? There
is already udev.conf you can put config option there. Thus people with
modprobe issues can easily "fix" the problem. And then decrease
default timeout back to 30 seconds. I agree that long module loading
(more than 30 secs) is abnormal and should be investigated by driver
authors.

2) Could you add 'echo w > /proc/sysrq-trigger' to udev code right
before killing the "modprobe" thread? sysrq will print information
about stuck threads (including modprobe itself) this will make
debugging easier. e.g. dmesg here
https://bugs.archlinux.org/task/40454 says nothing where the threads
were stuck.
Tom Gundersen
2014-10-10 22:45:17 UTC
Permalink
On Fri, Oct 10, 2014 at 11:54 PM, Anatol Pomozov
Post by Anatol Pomozov
1) Why not to make the timeout configurable through config file? There
is already udev.conf you can put config option there. Thus people with
modprobe issues can easily "fix" the problem. And then decrease
default timeout back to 30 seconds. I agree that long module loading
(more than 30 secs) is abnormal and should be investigated by driver
authors.
We can already configure this either on the udev or kernel
commandline, is that not sufficient (I don't object to also adding it
to the config file, just asking)?
Post by Anatol Pomozov
2) Could you add 'echo w > /proc/sysrq-trigger' to udev code right
before killing the "modprobe" thread? sysrq will print information
about stuck threads (including modprobe itself) this will make
debugging easier. e.g. dmesg here
https://bugs.archlinux.org/task/40454 says nothing where the threads
were stuck.
Are the current warnings (in udev git) sufficient (should tell you
which module is taking long, but still won't tell you which kernel
thread of course)?

Cheers,

Tom
Anatol Pomozov
2014-10-15 19:41:42 UTC
Permalink
Hi
Post by Tom Gundersen
On Fri, Oct 10, 2014 at 11:54 PM, Anatol Pomozov
Post by Anatol Pomozov
1) Why not to make the timeout configurable through config file? There
is already udev.conf you can put config option there. Thus people with
modprobe issues can easily "fix" the problem. And then decrease
default timeout back to 30 seconds. I agree that long module loading
(more than 30 secs) is abnormal and should be investigated by driver
authors.
We can already configure this either on the udev or kernel
commandline, is that not sufficient (I don't object to also adding it
to the config file, just asking)?
I did not know that udev timeout can be configured via kernel cmd. And
because other people ask about changing timeout they most like did not
know about it neither. Actually looking at
http://www.freedesktop.org/software/systemd/man/kernel-command-line.html
I do not see where it mentions udev timeout.

I think adding configuration to the right place (udev config file) and
adding documentation to make the option more discoverable will solve
the topic starter issue. Now anyone can easily set timeout they want.
The default timeout can go back to 30 sec in this case.
Post by Tom Gundersen
Post by Anatol Pomozov
2) Could you add 'echo w > /proc/sysrq-trigger' to udev code right
before killing the "modprobe" thread? sysrq will print information
about stuck threads (including modprobe itself) this will make
debugging easier. e.g. dmesg here
https://bugs.archlinux.org/task/40454 says nothing where the threads
were stuck.
Are the current warnings (in udev git) sufficient (should tell you
which module is taking long, but still won't tell you which kernel
thread of course)?
True. module name should be enough. In this case to debug the issue user needs:
- disable failing udev rule (or blacklist module?)
- reboot, it will let the user get into shell
- modprobe the failing module
- use sysrq-trigger to get more information about stuck process

So it is more matter of easier problem debugging. Not critical but it
will be useful imho. This feature can be configured via udev.conf
Alexander E. Patrakov
2014-10-15 19:46:44 UTC
Permalink
Post by Anatol Pomozov
- disable failing udev rule (or blacklist module?)
- reboot, it will let the user get into shell
- modprobe the failing module
- use sysrq-trigger to get more information about stuck process
Nitpick: this only works only if the "stuck modprobe" bug is 100%
reproducible. Which is not a given. So it is better to collect as much
information about the bug when it is noticed by systemd.
--
Alexander E. Patrakov
Tejun Heo
2014-09-09 21:42:40 UTC
Permalink
Hey, James.
Post by James Bottomley
I don't have very strong views on this one. However, I've got to say
from a systems point of view that if the desire is to flag when the
module is having problems, probing and initializing synchronously in a
thread spawned by init which the init process can watchdog and thus can
flash up warning messages seems to be more straightforwards than an
elaborate asynchronous mechanism with completion signalling which
achieves the same thing in a more complicated (and thus bug prone)
fashion.
We no longer report back error on probe failure on module load. It
used to make sense to indicate error for module load on probe failure
when the hardware was a lot simpler and drivers did their own device
enumeration. With the current bus / device setup, it doesn't make any
sense and driver core silently suppresses all probe failures. There's
nothing the probing thread can monitor anymore.

In that sense, we already separated out device probing from module
loading simply because the hardware reality mandated it and we have
dynamic mechanisms to listen for device probes exactly for the same
reason, so I think it makes sense to separate out the waiting too, at
least in the long term. In a modern dynamic setup, the waits are
essentially arbitrary and doesn't buy us anything.

Thanks.
--
tejun
James Bottomley
2014-09-09 22:26:02 UTC
Permalink
Post by Tejun Heo
Hey, James.
Post by James Bottomley
I don't have very strong views on this one. However, I've got to say
from a systems point of view that if the desire is to flag when the
module is having problems, probing and initializing synchronously in a
thread spawned by init which the init process can watchdog and thus can
flash up warning messages seems to be more straightforwards than an
elaborate asynchronous mechanism with completion signalling which
achieves the same thing in a more complicated (and thus bug prone)
fashion.
We no longer report back error on probe failure on module load.
Yes, we do; for every probe failure of a device on a driver we'll print
a warning (see drivers/base/dd.c). Now if someone is proposing we
should report this in a better fashion, that's probably a good idea, but
I must have missed that patch.
Post by Tejun Heo
It
used to make sense to indicate error for module load on probe failure
when the hardware was a lot simpler and drivers did their own device
enumeration. With the current bus / device setup, it doesn't make any
sense and driver core silently suppresses all probe failures. There's
nothing the probing thread can monitor anymore.
Except the length of time taken to probe. That seems to be what systemd
is interested in, hence this whole thread, right?
Post by Tejun Heo
In that sense, we already separated out device probing from module
loading simply because the hardware reality mandated it and we have
dynamic mechanisms to listen for device probes exactly for the same
reason, so I think it makes sense to separate out the waiting too, at
least in the long term. In a modern dynamic setup, the waits are
essentially arbitrary and doesn't buy us anything.
But that's nothing to do with sync or async. Nowadays we register a
driver, the driver may bind to multiple devices. If one of those
devices encounters an error during probe, we just report the fact in
dmesg and move on. The module_init thread currently returns when all
the probe routines for all enumerated devices have been called, so
module_init has no indication of any failures (because they might be
mixed with successes); successes are indicated as the device appears but
we have nothing other than the kernel log to indicate the failures. How
does moving to async probing alter this? It doesn't as far as I can
see, except that module_init returns earlier but now we no longer have
an indication of when the probe completes, so we have to add yet another
mechanism to tell us if we're interested in that. I really don't see
what this buys us.

James
Tejun Heo
2014-09-09 22:41:43 UTC
Permalink
Hello,
Post by James Bottomley
Post by Tejun Heo
We no longer report back error on probe failure on module load.
Yes, we do; for every probe failure of a device on a driver we'll print
a warning (see drivers/base/dd.c). Now if someone is proposing we
should report this in a better fashion, that's probably a good idea, but
I must have missed that patch.
We can do printks all the same from anywhere. There's nothing special
about printing from the module loading thread. The only way to
actually take advantage of the synchronisity would be propagating
error return to the waiting issuer, which we used to do but no longer
can.
Post by James Bottomley
Post by Tejun Heo
It
used to make sense to indicate error for module load on probe failure
when the hardware was a lot simpler and drivers did their own device
enumeration. With the current bus / device setup, it doesn't make any
sense and driver core silently suppresses all probe failures. There's
nothing the probing thread can monitor anymore.
Except the length of time taken to probe. That seems to be what systemd
is interested in, hence this whole thread, right?
No, systemd in this case isn't interested in the time taken to probe
at all. It is expecting module load to just do that - load the
module. Modern userlands, systemd or not, no longer depend on or make
use of the wait.
Post by James Bottomley
But that's nothing to do with sync or async. Nowadays we register a
driver, the driver may bind to multiple devices. If one of those
devices encounters an error during probe, we just report the fact in
dmesg and move on. The module_init thread currently returns when all
the probe routines for all enumerated devices have been called, so
module_init has no indication of any failures (because they might be
mixed with successes); successes are indicated as the device appears but
we have nothing other than the kernel log to indicate the failures. How
does moving to async probing alter this? It doesn't as far as I can
see, except that module_init returns earlier but now we no longer have
an indication of when the probe completes, so we have to add yet another
mechanism to tell us if we're interested in that. I really don't see
what this buys us.
The thing is that we have to have dynamic mechanism to listen for
device attachments no matter what and such mechanism has been in place
for a long time at this point. The synchronous wait simply doesn't
serve any purpose anymore and kinda gets in the way in that it makes
it a possibly extremely slow process to tell whether loading of a
module succeeded or not because the wait for the initial round of
probe is piggybacked.

Thanks.
--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
James Bottomley
2014-09-09 22:46:23 UTC
Permalink
Post by Tejun Heo
Hello,
Post by James Bottomley
Post by Tejun Heo
We no longer report back error on probe failure on module load.
Yes, we do; for every probe failure of a device on a driver we'll print
a warning (see drivers/base/dd.c). Now if someone is proposing we
should report this in a better fashion, that's probably a good idea, but
I must have missed that patch.
We can do printks all the same from anywhere. There's nothing special
about printing from the module loading thread. The only way to
actually take advantage of the synchronisity would be propagating
error return to the waiting issuer, which we used to do but no longer
can.
If you want the return of an individual device probe a log scraper gives
it to you ... and nothing else does currently. The advantage of the
prink in dd.c is that it's standard for everything and can be scanned
for ... if you take that out, you'll get complaints about the lack of
standard messages (you'd be surprised at the number of enterprise
monitoring systems that actually do log scraping).
Post by Tejun Heo
Post by James Bottomley
Post by Tejun Heo
It
used to make sense to indicate error for module load on probe failure
when the hardware was a lot simpler and drivers did their own device
enumeration. With the current bus / device setup, it doesn't make any
sense and driver core silently suppresses all probe failures. There's
nothing the probing thread can monitor anymore.
Except the length of time taken to probe. That seems to be what systemd
is interested in, hence this whole thread, right?
No, systemd in this case isn't interested in the time taken to probe
at all. It is expecting module load to just do that - load the
module. Modern userlands, systemd or not, no longer depend on or make
use of the wait.
So what's the problem? it can just fire and forget; that's what fork()
is for.
Post by Tejun Heo
Post by James Bottomley
But that's nothing to do with sync or async. Nowadays we register a
driver, the driver may bind to multiple devices. If one of those
devices encounters an error during probe, we just report the fact in
dmesg and move on. The module_init thread currently returns when all
the probe routines for all enumerated devices have been called, so
module_init has no indication of any failures (because they might be
mixed with successes); successes are indicated as the device appears but
we have nothing other than the kernel log to indicate the failures. How
does moving to async probing alter this? It doesn't as far as I can
see, except that module_init returns earlier but now we no longer have
an indication of when the probe completes, so we have to add yet another
mechanism to tell us if we're interested in that. I really don't see
what this buys us.
The thing is that we have to have dynamic mechanism to listen for
device attachments no matter what and such mechanism has been in place
for a long time at this point. The synchronous wait simply doesn't
serve any purpose anymore and kinda gets in the way in that it makes
it a possibly extremely slow process to tell whether loading of a
module succeeded or not because the wait for the initial round of
probe is piggybacked.
OK, so we just fire and forget in userland ... why bother inventing an
elaborate new infrastructure in the kernel to do exactly what

modprobe <mod> &

would do?

James
Tejun Heo
2014-09-09 22:52:45 UTC
Permalink
Hello, James.
Post by James Bottomley
If you want the return of an individual device probe a log scraper gives
it to you ... and nothing else does currently. The advantage of the
prink in dd.c is that it's standard for everything and can be scanned
for ... if you take that out, you'll get complaints about the lack of
standard messages (you'd be surprised at the number of enterprise
monitoring systems that actually do log scraping).
Why would a log scaper care about which task is printing the messages?
The printk can stay there. There's nothing wrong with it. Log
scapers tend to be asynchronous in nature but if a log scraper wants
to operate synchronously for whatever reason, it can simply not turn
on async probing.
Post by James Bottomley
OK, so we just fire and forget in userland ... why bother inventing an
elaborate new infrastructure in the kernel to do exactly what
modprobe <mod> &
would do?
I think the argument there is that the issuer wants to know whether
such operations succeeded or not and wants to report and record the
result and possibly take other actions in response. We're currently
mixing wait and error reporting for one type of operation with wait
for another. I'm not saying it's a fatal flaw or anything but it can
get in the way.

Thanks.
--
tejun
Dmitry Torokhov
2014-09-09 23:01:32 UTC
Permalink
Post by James Bottomley
Post by Tejun Heo
The thing is that we have to have dynamic mechanism to listen for
device attachments no matter what and such mechanism has been in place
for a long time at this point. The synchronous wait simply doesn't
serve any purpose anymore and kinda gets in the way in that it makes
it a possibly extremely slow process to tell whether loading of a
module succeeded or not because the wait for the initial round of
probe is piggybacked.
OK, so we just fire and forget in userland ... why bother inventing an
elaborate new infrastructure in the kernel to do exactly what
modprobe <mod> &
would do?
Just so we do not forget: we also want the no-modules case to also be able
to probe asynchronously so that a slow device does not stall kernel booting.

Thanks.
--
Dmitry
James Bottomley
2014-09-11 19:59:25 UTC
Permalink
Post by Dmitry Torokhov
Post by James Bottomley
Post by Tejun Heo
The thing is that we have to have dynamic mechanism to listen for
device attachments no matter what and such mechanism has been in place
for a long time at this point. The synchronous wait simply doesn't
serve any purpose anymore and kinda gets in the way in that it makes
it a possibly extremely slow process to tell whether loading of a
module succeeded or not because the wait for the initial round of
probe is piggybacked.
OK, so we just fire and forget in userland ... why bother inventing an
elaborate new infrastructure in the kernel to do exactly what
modprobe <mod> &
would do?
Just so we do not forget: we also want the no-modules case to also be able
to probe asynchronously so that a slow device does not stall kernel booting.
Yes, but we mostly do this anyway. SCSI for instance does asynchronous
scanning of attached devices (once the cards are probed) but has a sync
point for ordering.

The problem of speeding up boot is different from the one of init
processes killing modprobes. There are elements in common, but by and
large the biggest headaches at least in large device number boots have
already been tackled by the enterprise crowd (they don't like their
S390's or 1024 core NUMA systems taking half an hour to come up).

James
Dmitry Torokhov
2014-09-11 20:23:54 UTC
Permalink
Post by James Bottomley
Post by Dmitry Torokhov
Post by James Bottomley
Post by Tejun Heo
The thing is that we have to have dynamic mechanism to listen for
device attachments no matter what and such mechanism has been in place
for a long time at this point. The synchronous wait simply doesn't
serve any purpose anymore and kinda gets in the way in that it makes
it a possibly extremely slow process to tell whether loading of a
module succeeded or not because the wait for the initial round of
probe is piggybacked.
OK, so we just fire and forget in userland ... why bother inventing an
elaborate new infrastructure in the kernel to do exactly what
modprobe <mod> &
would do?
Just so we do not forget: we also want the no-modules case to also be able
to probe asynchronously so that a slow device does not stall kernel booting.
Yes, but we mostly do this anyway. SCSI for instance does asynchronous
scanning of attached devices (once the cards are probed)
What would it do it card was a bit slow to probe?
Post by James Bottomley
but has a sync
point for ordering.
Quite often we do not really care about ordering of devices. I mean,
does it matter if your mouse is discovered before your keyboard or
after?
Post by James Bottomley
The problem of speeding up boot is different from the one of init
processes killing modprobes.
Right. One is systemd doing stupid things, another is kernel could be
smarter.
Post by James Bottomley
There are elements in common, but by and
large the biggest headaches at least in large device number boots have
already been tackled by the enterprise crowd (they don't like their
S390's or 1024 core NUMA systems taking half an hour to come up).
Please do not position this as a mostly solved large systems problem,
For us it is touchpad detection stalling kernel for 0.5-1 sec. Which is
a lot given that we boot in seconds.

Thanks.
--
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Luis R. Rodriguez
2014-09-11 20:42:20 UTC
Permalink
On Thu, Sep 11, 2014 at 1:23 PM, Dmitry Torokhov
Post by Dmitry Torokhov
Post by James Bottomley
There are elements in common, but by and
large the biggest headaches at least in large device number boots have
already been tackled by the enterprise crowd (they don't like their
S390's or 1024 core NUMA systems taking half an hour to come up).
Please do not position this as a mostly solved large systems problem,
For us it is touchpad detection stalling kernel for 0.5-1 sec. Which is
a lot given that we boot in seconds.
Dmitry, would working on top of the aysnc series be reasonable? Then
we could address these as separate things which we'd build on top of.
The one aspect I see us needing to share is the "async probe universe
is OK" flag.

Luis
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Dmitry Torokhov
2014-09-11 20:53:08 UTC
Permalink
Post by Luis R. Rodriguez
On Thu, Sep 11, 2014 at 1:23 PM, Dmitry Torokhov
Post by Dmitry Torokhov
Post by James Bottomley
There are elements in common, but by and
large the biggest headaches at least in large device number boots have
already been tackled by the enterprise crowd (they don't like their
S390's or 1024 core NUMA systems taking half an hour to come up).
Please do not position this as a mostly solved large systems problem,
For us it is touchpad detection stalling kernel for 0.5-1 sec. Which is
a lot given that we boot in seconds.
Dmitry, would working on top of the aysnc series be reasonable? Then
we could address these as separate things which we'd build on top of.
The one aspect I see us needing to share is the "async probe universe
is OK" flag.
Sure. Are you planning on refreshing your series? I think the
code-related discussion kind of stalled...
--
Dmitry
Luis R. Rodriguez
2014-09-11 21:08:15 UTC
Permalink
On Thu, Sep 11, 2014 at 1:53 PM, Dmitry Torokhov
Post by Dmitry Torokhov
Post by Luis R. Rodriguez
On Thu, Sep 11, 2014 at 1:23 PM, Dmitry Torokhov
Post by Dmitry Torokhov
Post by James Bottomley
There are elements in common, but by and
large the biggest headaches at least in large device number boots have
already been tackled by the enterprise crowd (they don't like their
S390's or 1024 core NUMA systems taking half an hour to come up).
Please do not position this as a mostly solved large systems problem,
For us it is touchpad detection stalling kernel for 0.5-1 sec. Which is
a lot given that we boot in seconds.
Dmitry, would working on top of the aysnc series be reasonable? Then
we could address these as separate things which we'd build on top of.
The one aspect I see us needing to share is the "async probe universe
is OK" flag.
Sure. Are you planning on refreshing your series?
Yes.
Post by Dmitry Torokhov
I think the code-related discussion kind of stalled...
I was just waiting for any possible brain farts to flush out before a
new respin. I'll tackle this now.

Luis
Pavel Machek
2014-09-22 19:49:06 UTC
Permalink
Post by Dmitry Torokhov
Post by James Bottomley
Post by Dmitry Torokhov
Post by James Bottomley
Post by Tejun Heo
The thing is that we have to have dynamic mechanism to listen for
device attachments no matter what and such mechanism has been in place
for a long time at this point. The synchronous wait simply doesn't
serve any purpose anymore and kinda gets in the way in that it makes
it a possibly extremely slow process to tell whether loading of a
module succeeded or not because the wait for the initial round of
probe is piggybacked.
OK, so we just fire and forget in userland ... why bother inventing an
elaborate new infrastructure in the kernel to do exactly what
modprobe <mod> &
would do?
Just so we do not forget: we also want the no-modules case to also be able
to probe asynchronously so that a slow device does not stall kernel booting.
Yes, but we mostly do this anyway. SCSI for instance does asynchronous
scanning of attached devices (once the cards are probed)
What would it do it card was a bit slow to probe?
Post by James Bottomley
but has a sync
point for ordering.
Quite often we do not really care about ordering of devices. I mean,
does it matter if your mouse is discovered before your keyboard or
after?
Actually yes, I suspect it does.

I do evtest /dev/input/eventX by hand, occassionaly. It would be
annoying if they moved between reboots.
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Dmitry Torokhov
2014-09-22 20:23:54 UTC
Permalink
Post by Pavel Machek
Post by Dmitry Torokhov
Post by James Bottomley
Post by Dmitry Torokhov
Post by James Bottomley
Post by Tejun Heo
The thing is that we have to have dynamic mechanism to listen for
device attachments no matter what and such mechanism has been in place
for a long time at this point. The synchronous wait simply doesn't
serve any purpose anymore and kinda gets in the way in that it makes
it a possibly extremely slow process to tell whether loading of a
module succeeded or not because the wait for the initial round of
probe is piggybacked.
OK, so we just fire and forget in userland ... why bother inventing an
elaborate new infrastructure in the kernel to do exactly what
modprobe <mod> &
would do?
Just so we do not forget: we also want the no-modules case to also be able
to probe asynchronously so that a slow device does not stall kernel
booting.> >
Yes, but we mostly do this anyway. SCSI for instance does asynchronous
scanning of attached devices (once the cards are probed)
What would it do it card was a bit slow to probe?
Post by James Bottomley
but has a sync
point for ordering.
Quite often we do not really care about ordering of devices. I mean,
does it matter if your mouse is discovered before your keyboard or
after?
Actually yes, I suspect it does.
I do evtest /dev/input/eventX by hand, occassionaly. It would be
annoying if they moved between reboots.
I am sorry but you will have to cope with such annoyances. It' snot like we
fail to boot the box here.

The systems are now mostly hot-pluggable and userland is supposed to
handle it, and it does, at least for input devices. If you want stable naming
use udev facilities to rename devices as needed or add needed symlinks (by-id,
etc.).

Thanks.
--
Dmitry
Pavel Machek
2014-09-30 21:06:34 UTC
Permalink
Post by Dmitry Torokhov
Post by Pavel Machek
Post by Dmitry Torokhov
Post by James Bottomley
Yes, but we mostly do this anyway. SCSI for instance does asynchronous
scanning of attached devices (once the cards are probed)
What would it do it card was a bit slow to probe?
Post by James Bottomley
but has a sync
point for ordering.
Quite often we do not really care about ordering of devices. I mean,
does it matter if your mouse is discovered before your keyboard or
after?
Actually yes, I suspect it does.
I do evtest /dev/input/eventX by hand, occassionaly. It would be
annoying if they moved between reboots.
I am sorry but you will have to cope with such annoyances. It' snot like we
fail to boot the box here.
The systems are now mostly hot-pluggable and userland is supposed to
handle it, and it does, at least for input devices. If you want stable naming
use udev facilities to rename devices as needed or add needed symlinks (by-id,
etc.).
Well, it would be nice if udev was not mandatory. Do the sync points
for ordering actually cost us something?
Pavel
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
Dmitry Torokhov
2014-09-30 21:34:44 UTC
Permalink
Post by Pavel Machek
Post by Dmitry Torokhov
Post by Pavel Machek
Post by Dmitry Torokhov
Post by James Bottomley
Yes, but we mostly do this anyway. SCSI for instance does asynchronous
scanning of attached devices (once the cards are probed)
What would it do it card was a bit slow to probe?
Post by James Bottomley
but has a sync
point for ordering.
Quite often we do not really care about ordering of devices. I mean,
does it matter if your mouse is discovered before your keyboard or
after?
Actually yes, I suspect it does.
I do evtest /dev/input/eventX by hand, occassionaly. It would be
annoying if they moved between reboots.
I am sorry but you will have to cope with such annoyances. It' snot like we
fail to boot the box here.
The systems are now mostly hot-pluggable and userland is supposed to
handle it, and it does, at least for input devices. If you want stable naming
use udev facilities to rename devices as needed or add needed symlinks (by-id,
etc.).
Well, it would be nice if udev was not mandatory. Do the sync points
for ordering actually cost us something?
Yes, boot time. We can save a second or two off the boot time if we probe
several devices/drivers simultaneously.

Thanks.
--
Dmitry
Jiri Kosina
2014-09-09 22:00:31 UTC
Permalink
Post by Luis R. Rodriguez
By design it seems systemd should not allow worker processes to block
indefinitely and in fact it currently uses the same timeout for all
types of worker processes.
And I whole-heartedly believe this is something that fundamentally needs
to be addressed in systemd, not in the kernel.

This aproach is actually introducing a user-visible regressions. Look, for
example, exec() never times out. Therefore if your system is on its knees,
heavily overloaded (or completely broken), you are likely to be able to
`reboot' it, because exec("/sbin/reboot") ultimately succeeds.

But with all the timeouts, dbus, "Failed to issue method call: Did
not receive a reply" messages, this is getting close to impossible.
--
Jiri Kosina
SUSE Labs
Oleg Nesterov
2014-09-05 10:59:49 UTC
Permalink
Post by Luis R. Rodriguez
The new umh kill option has allowed kthreads to receive
kill signals but they are generally accepting all sources
of kill signals
And I think this is right,
Post by Luis R. Rodriguez
while the original motivation was to enable
through the OOM from sending the kill.
even if the main concern was OOM.
Post by Luis R. Rodriguez
Users can provide a log output and it should be clear on
the trace what probe / driver got the kill signal.
Well, if you need a WARN output, perhaps you could just add
WARN_ON(fatal_signal_pending()) at the end of load_module() ?

Not only kthread_create() can fail if systemd sends SIGKILL.
Post by Luis R. Rodriguez
Although Oleg had rejected a
similar change a while ago
And honestly, I still dislike this change.

Oleg.
Luis R. Rodriguez
2014-09-05 17:35:30 UTC
Permalink
Post by Oleg Nesterov
Post by Luis R. Rodriguez
The new umh kill option has allowed kthreads to receive
kill signals but they are generally accepting all sources
of kill signals
And I think this is right,
Post by Luis R. Rodriguez
while the original motivation was to enable
through the OOM from sending the kill.
even if the main concern was OOM.
Post by Luis R. Rodriguez
Users can provide a log output and it should be clear on
the trace what probe / driver got the kill signal.
Well, if you need a WARN output, perhaps you could just add
WARN_ON(fatal_signal_pending()) at the end of load_module() ?
We could and that's a good idea, thanks! This however would
at least allow the device to be functional in the case the
kill was received during kthread usage, but it would certainly
also set precedents for doing similar things in the kernel
which I do agree with is hacky. If we had upstream at
least WARN_ON(fatal_signal_pending()) as you note then
I think it would at least be a reasonable compromise.
Post by Oleg Nesterov
Not only kthread_create() can fail if systemd sends SIGKILL.
Sure, although its currently the only source found and debugged.
Post by Oleg Nesterov
Post by Luis R. Rodriguez
Although Oleg had rejected a
similar change a while ago
And honestly, I still dislike this change.
Don't blame you. The code is sensitive and hacky.

Luis
Luis R. Rodriguez
2014-09-05 06:37:25 UTC
Permalink
From: "Luis R. Rodriguez" <***@suse.com>

cxgb4 probe can take up to over 1 minute when the firmware is
is written and installed on the device, even after this the device
driver still does some device probing and can take quite a bit.
systemd will kill this driver when probe does take over 30 seconds,
use the asynch probe mechanism to circumvent this.

Cc: Tetsuo Handa <penguin-***@I-love.SAKURA.ne.jp>
Cc: Joseph Salisbury <***@canonical.com>
Cc: One Thousand Gnomes <***@lxorguk.ukuu.org.uk>
Cc: Tim Gardner <***@canonical.com>
Cc: Pierre Fersing <pierre-***@pierref.org>
Cc: Andrew Morton <***@linux-foundation.org>
Cc: Oleg Nesterov <***@redhat.com>
Cc: Benjamin Poirier <***@suse.de>
Cc: Greg Kroah-Hartman <***@linuxfoundation.org>
Cc: Nagalakshmi Nandigama <***@avagotech.com>
Cc: Praveen Krishnamoorthy <***@avagotech.com>
Cc: Sreekanth Reddy <***@avagotech.com>
Cc: Abhijit Mahajan <***@avagotech.com>
Cc: Hariprasad S <***@chelsio.com>
Cc: Santosh Rastapur <***@chelsio.com>
Cc: Casey Leedom <***@chelsio.com>
Cc: MPT-***@avagotech.com
Cc: linux-***@vger.kernel.org
Cc: linux-***@vger.kernel.org
Cc: ***@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <***@suse.com>
---
drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
index 18fb9c6..5f7d24a 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_main.c
@@ -6794,6 +6794,7 @@ static struct pci_driver cxgb4_driver = {
.remove = remove_one,
.shutdown = remove_one,
.err_handler = &cxgb4_eeh,
+ .driver.async_probe = true,
};

static int __init cxgb4_init_module(void)
--
2.0.3
Luis R. Rodriguez
2014-09-05 06:37:26 UTC
Permalink
From: "Luis R. Rodriguez" <***@suse.com>

Its reported that mptsas can at times take over 30 seconds
to recognize SCSI storage devices [0], this is done on the
driver's probe path. Use the the new asynch probe to
circumvent systemd from killing this driver.

[0] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1276705

Cc: Tetsuo Handa <penguin-***@I-love.SAKURA.ne.jp>
Cc: Joseph Salisbury <***@canonical.com>
Cc: One Thousand Gnomes <***@lxorguk.ukuu.org.uk>
Cc: Tim Gardner <***@canonical.com>
Cc: Pierre Fersing <pierre-***@pierref.org>
Cc: Andrew Morton <***@linux-foundation.org>
Cc: Oleg Nesterov <***@redhat.com>
Cc: Benjamin Poirier <***@suse.de>
Cc: Greg Kroah-Hartman <***@linuxfoundation.org>
Cc: Nagalakshmi Nandigama <***@avagotech.com>
Cc: Praveen Krishnamoorthy <***@avagotech.com>
Cc: Sreekanth Reddy <***@avagotech.com>
Cc: Abhijit Mahajan <***@avagotech.com>
Cc: Hariprasad S <***@chelsio.com>
Cc: Santosh Rastapur <***@chelsio.com>
Cc: Casey Leedom <***@chelsio.com>
Cc: MPT-***@avagotech.com
Cc: linux-***@vger.kernel.org
Cc: linux-***@vger.kernel.org
Cc: ***@vger.kernel.org
Signed-off-by: Luis R. Rodriguez <***@suse.com>
---
drivers/message/fusion/mptsas.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
index 0707fa2..6dfee95 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -5385,6 +5385,7 @@ static struct pci_driver mptsas_driver = {
.suspend = mptscsih_suspend,
.resume = mptscsih_resume,
#endif
+ .driver.async_probe = true,
};

static int __init
--
2.0.3
Tejun Heo
2014-09-05 07:16:43 UTC
Permalink
Post by Luis R. Rodriguez
Its reported that mptsas can at times take over 30 seconds
to recognize SCSI storage devices [0], this is done on the
driver's probe path. Use the the new asynch probe to
circumvent systemd from killing this driver.
Again, *ANY* SCSI storage controller may. The fact that a specific
bug report was filed on mptsas doesn't really mean anything.

Thanks.
--
tejun
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to ***@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Hannes Reinecke
2014-09-05 07:23:28 UTC
Permalink
Post by Luis R. Rodriguez
Its reported that mptsas can at times take over 30 seconds
to recognize SCSI storage devices [0], this is done on the
driver's probe path. Use the the new asynch probe to
circumvent systemd from killing this driver.
[0] https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1276705
---
drivers/message/fusion/mptsas.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/message/fusion/mptsas.c b/drivers/message/fusion/mptsas.c
index 0707fa2..6dfee95 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -5385,6 +5385,7 @@ static struct pci_driver mptsas_driver = {
.suspend = mptscsih_suspend,
.resume = mptscsih_resume,
#endif
+ .driver.async_probe = true,
};
static int __init
This is the wrong appoach.
First of all, the mptsas, mpt2sas, and mpt3sas all share the same
driver layout, so any issue happeing with this driver will most
likely affect the others, too.
Secondly the driver is event-based anyway, so we should be moving
the initialisation to the already existing event handler:

diff --git a/drivers/message/fusion/mptsas.c
b/drivers/message/fusion/mptsas.c
index 0707fa2..6f41e2c 100644
--- a/drivers/message/fusion/mptsas.c
+++ b/drivers/message/fusion/mptsas.c
@@ -5305,7 +5305,7 @@ mptsas_probe(struct pci_dev *pdev, const
struct pci_device_id *id)
/* older firmware doesn't support expander events */
if ((ioc->facts.HeaderVersion >> 8) < 0xE)
ioc->old_sas_discovery_protocal = 1;
- mptsas_scan_sas_topology(ioc);
+ mptsas_queue_rescan(ioc);
mptsas_fw_event_on(ioc);
return 0;
Oleg Nesterov
2014-09-05 11:24:17 UTC
Permalink
Post by Luis R. Rodriguez
struct driver_private {
struct kobject kobj;
struct klist klist_devices;
struct klist_node knode_bus;
struct module_kobject *mkobj;
+ struct driver_attach_work *attach_work;
struct device_driver *driver;
I am not arguing, just curious...

Are you trying to shrink sizeof(driver_private) ? The code can be simpler
if you just embedd "struct work_struct attach_work" into driver_private,
and you do not need "struct driver_attach_work" or another ->driver pointer
this way.

Oleg.
Luis R. Rodriguez
2014-09-05 17:25:41 UTC
Permalink
Post by Oleg Nesterov
Post by Luis R. Rodriguez
struct driver_private {
struct kobject kobj;
struct klist klist_devices;
struct klist_node knode_bus;
struct module_kobject *mkobj;
+ struct driver_attach_work *attach_work;
struct device_driver *driver;
I am not arguing, just curious...
Are you trying to shrink sizeof(driver_private) ?
Yeap.
Post by Oleg Nesterov
The code can be simpler
if you just embedd "struct work_struct attach_work" into driver_private,
and you do not need "struct driver_attach_work" or another ->driver pointer
this way.
Agreed, I considered it and figured it wouldn't make much sense
to push onto folks more bytes if this feature was optional and
likely only used by a few drivers, so a pointer / kzalloc seemed
better to deal with. This saves us 24 bytes. I even tried to
implement a container_of_p() for pointers but that obviosly
didn't work well fast as a pointer can have any address and is
not relative to the parent, and if its on stack the address
can vary depending on implementation. For example the first member
should always have the same address as the struct but if the
first member is a pointer it would be off for me by 12 bytes.
I am not sure if this is standarized or not.

Luis
Dmitry Torokhov
2014-09-05 22:10:29 UTC
Permalink
Hi Luis,
Post by Luis R. Rodriguez
1) when a built-in driver takes a few seconds to initialize its
delays can stall the overall boot process
This patch does not solve the 2nd issue fully as it only calls probe
asynchronously during driver registration (and also only for modules???
- it checks drv->owner in a few places). The device may get created
after driver is initialized, in this case we still want probe to be
called asynchronously.

I think something like the patch below should work. Note that it uses
async_checdule(), so that will satisy for the moment Tejun's objections
to the behavior with regard to module loading and initialization, but it
does not solve your issue with modules being killed after 30 seconds.

To tell the truth I think systemd should not be doing it; it is not its
place to dictate how long module should take to load. It may print
warnings and we'll work on fixing the drivers, but aborting boot just
because they feel like it took too long is not a good idea.

Thanks.
--
Dmitry


driver-core: add driver async_probe support

From: Dmitry Torokhov <***@gmail.com>

Some devices take a long time when initializing, and not all drivers are
suited to initialize their devices when they are open. For example, input
drivers need to interrogate device in order to publish its capabilities
before userspace will open them. When such drivers are compiled into kernel
they may stall entire kernel initialization.

This change allows drivers request for their probe functions to be called
asynchronously during driver and device registration (manual binding is
still synchronous). Because async_schedule is used to perform asynchronous
calls module loading will still wait for the probing to complete.

This is based on earlier patch by "Luis R. Rodriguez" <***@suse.com>

Signed-off-by: Dmitry Torokhov <***@gmail.com>
---
drivers/base/bus.c | 31 ++++++++++----
drivers/base/dd.c | 106 +++++++++++++++++++++++++++++++++++++++---------
include/linux/device.h | 2 +
3 files changed, 112 insertions(+), 27 deletions(-)

diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 83e910a..49fe573 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -10,6 +10,7 @@
*
*/

+#include <linux/async.h>
#include <linux/device.h>
#include <linux/module.h>
#include <linux/errno.h>
@@ -547,15 +548,12 @@ void bus_probe_device(struct device *dev)
{
struct bus_type *bus = dev->bus;
struct subsys_interface *sif;
- int ret;

if (!bus)
return;

- if (bus->p->drivers_autoprobe) {
- ret = device_attach(dev);
- WARN_ON(ret < 0);
- }
+ if (bus->p->drivers_autoprobe)
+ device_initial_probe(dev);

mutex_lock(&bus->p->mutex);
list_for_each_entry(sif, &bus->p->interfaces, node)
@@ -657,6 +655,17 @@ static ssize_t uevent_store(struct device_driver *drv, const char *buf,
}
static DRIVER_ATTR_WO(uevent);

+static void driver_attach_async(void *_drv, async_cookie_t cookie)
+{
+ struct device_driver *drv = _drv;
+ int ret;
+
+ ret = driver_attach(drv);
+
+ pr_debug("bus: '%s': driver %s async attach completed: %d\n",
+ drv->bus->name, drv->name, ret);
+}
+
/**
* bus_add_driver - Add a driver to the bus.
* @drv: driver.
@@ -689,9 +698,15 @@ int bus_add_driver(struct device_driver *drv)

klist_add_tail(&priv->knode_bus, &bus->p->klist_drivers);
if (drv->bus->p->drivers_autoprobe) {
- error = driver_attach(drv);
- if (error)
- goto out_unregister;
+ if (drv->async_probe) {
+ pr_debug("bus: '%s': probing driver %s asynchronously\n",
+ drv->bus->name, drv->name);
+ async_schedule(driver_attach_async, drv);
+ } else {
+ error = driver_attach(drv);
+ if (error)
+ goto out_unregister;
+ }
}
module_add_driver(drv->owner, drv);

diff --git a/drivers/base/dd.c b/drivers/base/dd.c
index e4ffbcf..67a2f85 100644
--- a/drivers/base/dd.c
+++ b/drivers/base/dd.c
@@ -402,31 +402,52 @@ int driver_probe_device(struct device_driver *drv, struct device *dev)
return ret;
}

-static int __device_attach(struct device_driver *drv, void *data)
+struct device_attach_data {
+ struct device *dev;
+ bool check_async;
+ bool want_async;
+ bool have_async;
+};
+
+static int __device_attach_driver(struct device_driver *drv, void *_data)
{
- struct device *dev = data;
+ struct device_attach_data *data = _data;
+ struct device *dev = data->dev;

if (!driver_match_device(drv, dev))
return 0;

+ if (drv->async_probe)
+ data->have_async = true;
+
+ if (data->check_async && drv->async_probe != data->want_async)
+ return 0;
+
return driver_probe_device(drv, dev);
}

-/**
- * device_attach - try to attach device to a driver.
- * @dev: device.
- *
- * Walk the list of drivers that the bus has and call
- * driver_probe_device() for each pair. If a compatible
- * pair is found, break out and return.
- *
- * Returns 1 if the device was bound to a driver;
- * 0 if no matching driver was found;
- * -ENODEV if the device is not registered.
- *
- * When called for a USB interface, @dev->parent lock must be held.
- */
-int device_attach(struct device *dev)
+static void __device_attach_async_helper(void *_dev, async_cookie_t cookie)
+{
+ struct device *dev = _dev;
+ struct device_attach_data data = {
+ .dev = dev,
+ .check_async = true,
+ .want_async = true,
+ };
+
+ device_lock(dev);
+
+ bus_for_each_drv(dev->bus, NULL, &data, __device_attach_driver);
+ dev_dbg(dev, "async probe completed\n");
+
+ pm_request_idle(dev);
+
+ device_unlock(dev);
+
+ put_device(dev);
+}
+
+int __device_attach(struct device *dev, bool allow_async)
{
int ret = 0;

@@ -444,15 +465,59 @@ int device_attach(struct device *dev)
ret = 0;
}
} else {
- ret = bus_for_each_drv(dev->bus, NULL, dev, __device_attach);
- pm_request_idle(dev);
+ struct device_attach_data data = {
+ .dev = dev,
+ .check_async = allow_async,
+ .want_async = false,
+ };
+
+ ret = bus_for_each_drv(dev->bus, NULL, &data,
+ __device_attach_driver);
+ if (!ret && allow_async && data.have_async) {
+ /*
+ * If we could not find appropriate driver
+ * synchronously and we are allowed to do
+ * async probes and there are drivers that
+ * want to probe asynchronously, we'll
+ * try them.
+ */
+ dev_dbg(dev, "scheduling asynchronous probe\n");
+ get_device(dev);
+ async_schedule(__device_attach_async_helper, dev);
+ } else {
+ pm_request_idle(dev);
+ }
}
out_unlock:
device_unlock(dev);
return ret;
}
+
+/**
+ * device_attach - try to attach device to a driver.
+ * @dev: device.
+ *
+ * Walk the list of drivers that the bus has and call
+ * driver_probe_device() for each pair. If a compatible
+ * pair is found, break out and return.
+ *
+ * Returns 1 if the device was bound to a driver;
+ * 0 if no matching driver was found;
+ * -ENODEV if the device is not registered.
+ *
+ * When called for a USB interface, @dev->parent lock must be held.
+ */
+int device_attach(struct device *dev)
+{
+ return __device_attach(dev, false);
+}
EXPORT_SYMBOL_GPL(device_attach);

+void device_initial_probe(struct device *dev)
+{
+ __device_attach(dev, true);
+}
+
static int __driver_attach(struct device *dev, void *data)
{
struct device_driver *drv = data;
@@ -507,6 +572,9 @@ static void __device_release_driver(struct device *dev)

drv = dev->driver;
if (drv) {
+ if (drv->async_probe)
+ async_synchronize_full();
+
pm_runtime_get_sync(dev);

driver_sysfs_remove(dev);
diff --git a/include/linux/device.h b/include/linux/device.h
index 43d183a..c6fa2e7 100644
--- a/include/linux/device.h
+++ b/include/linux/device.h
@@ -233,6 +233,7 @@ struct device_driver {
const char *mod_name; /* used for built-in modules */

bool suppress_bind_attrs; /* disables bind/unbind via sysfs */
+ bool async_probe;

const struct of_device_id *of_match_table;
const struct acpi_device_id *acpi_match_table;
@@ -966,6 +967,7 @@ extern int __must_check device_bind_driver(struct device *dev);
extern void device_release_driver(struct device *dev);
extern int __must_check device_attach(struct device *dev);
extern int __must_check driver_attach(struct device_driver *drv);
+extern void device_initial_probe(struct device *dev);
extern int __must_check device_reprobe(struct device *dev);

/*
Luis R. Rodriguez
2014-10-20 23:43:25 UTC
Permalink
Post by Luis R. Rodriguez
diff --git a/drivers/base/bus.c b/drivers/base/bus.c
index 83e910a..49fe573 100644
--- a/drivers/base/bus.c
+++ b/drivers/base/bus.c
@@ -10,6 +10,7 @@
*
*/
+#include <linux/async.h>
#include <linux/device.h>
#include <linux/module.h>
#include <linux/errno.h>
@@ -547,15 +548,12 @@ void bus_probe_device(struct device *dev)
{
struct bus_type *bus = dev->bus;
struct subsys_interface *sif;
- int ret;
if (!bus)
return;
- if (bus->p->drivers_autoprobe) {
- ret = device_attach(dev);
- WARN_ON(ret < 0);
- }
+ if (bus->p->drivers_autoprobe)
+ device_initial_probe(dev);
mutex_lock(&bus->p->mutex);
list_for_each_entry(sif, &bus->p->interfaces, node)
@@ -657,6 +655,17 @@ static ssize_t uevent_store(struct device_driver *drv, const char *buf,
}
static DRIVER_ATTR_WO(uevent);
Based on my review with my latest changes this is what I was missing,
I'll be sure to address this.

Luis

Loading...