All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] nvme: Boot as soon as the boot controller has been probed
@ 2020-11-08  4:09 Bart Van Assche
  2020-11-08  8:24 ` Greg KH
                   ` (3 more replies)
  0 siblings, 4 replies; 18+ messages in thread
From: Bart Van Assche @ 2020-11-08  4:09 UTC (permalink / raw)
  To: Keith Busch, Sagi Grimberg
  Cc: Greg KH, Mikulas Patocka, Christoph Hellwig, linux-nvme, Bart Van Assche

The following two issues have been introduced by commit 1811977568e0
("nvme/pci: Use async_schedule for initial reset work"):
- The boot process waits until all NVMe controllers have been probed
  instead of only waiting until the boot controller has been probed.
  This slows down the boot process.
- Some of the controller probing work happens asynchronously without
  the device core being aware of this.

Hence this patch that makes all probing work happen from nvme_probe()
and that tells the device core to probe multiple NVMe controllers
concurrently by setting PROBE_PREFER_ASYNCHRONOUS.

Cc: Mikulas Patocka <mpatocka@redhat.com>
Cc: Keith Busch <keith.busch@intel.com>
Cc: Greg KH <gregkh@linuxfoundation.org>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/nvme/host/pci.c | 27 +++++++++++++--------------
 1 file changed, 13 insertions(+), 14 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 0578ff253c47..703bb3aee817 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -6,7 +6,6 @@
 
 #include <linux/acpi.h>
 #include <linux/aer.h>
-#include <linux/async.h>
 #include <linux/blkdev.h>
 #include <linux/blk-mq.h>
 #include <linux/blk-mq-pci.h>
@@ -2821,15 +2820,6 @@ static inline bool nvme_acpi_storage_d3(struct pci_dev *dev)
 }
 #endif /* CONFIG_ACPI */
 
-static void nvme_async_probe(void *data, async_cookie_t cookie)
-{
-	struct nvme_dev *dev = data;
-
-	flush_work(&dev->ctrl.reset_work);
-	flush_work(&dev->ctrl.scan_work);
-	nvme_put_ctrl(&dev->ctrl);
-}
-
 static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 {
 	int node, result = -ENOMEM;
@@ -2903,8 +2893,16 @@ static int nvme_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 
 	dev_info(dev->ctrl.device, "pci function %s\n", dev_name(&pdev->dev));
 
-	nvme_reset_ctrl(&dev->ctrl);
-	async_schedule(nvme_async_probe, dev);
+	if (nvme_reset_ctrl(&dev->ctrl) == 0) {
+		/*
+		 * Since reset_work is scheduled on the context of
+		 * nvme_reset_wq and since that workqueue is not used for
+		 * probing devices, waiting until reset_work from nvme_probe()
+		 * is fine.
+		 */
+		flush_work(&dev->ctrl.reset_work);
+	}
+	nvme_put_ctrl(&dev->ctrl);
 
 	return 0;
 
@@ -3221,11 +3219,12 @@ static struct pci_driver nvme_driver = {
 	.probe		= nvme_probe,
 	.remove		= nvme_remove,
 	.shutdown	= nvme_shutdown,
-#ifdef CONFIG_PM_SLEEP
 	.driver		= {
+#ifdef CONFIG_PM_SLEEP
 		.pm	= &nvme_dev_pm_ops,
-	},
 #endif
+		.probe_type	= PROBE_PREFER_ASYNCHRONOUS,
+	},
 	.sriov_configure = pci_sriov_configure_simple,
 	.err_handler	= &nvme_err_handler,
 };

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH] nvme: Boot as soon as the boot controller has been probed
  2020-11-08  4:09 [PATCH] nvme: Boot as soon as the boot controller has been probed Bart Van Assche
@ 2020-11-08  8:24 ` Greg KH
  2020-11-08 22:31   ` Keith Busch
  2020-11-08 19:13 ` Sagi Grimberg
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 18+ messages in thread
From: Greg KH @ 2020-11-08  8:24 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Keith Busch, Mikulas Patocka, Sagi Grimberg, linux-nvme,
	Christoph Hellwig

On Sat, Nov 07, 2020 at 08:09:03PM -0800, Bart Van Assche wrote:
> The following two issues have been introduced by commit 1811977568e0
> ("nvme/pci: Use async_schedule for initial reset work"):
> - The boot process waits until all NVMe controllers have been probed
>   instead of only waiting until the boot controller has been probed.
>   This slows down the boot process.
> - Some of the controller probing work happens asynchronously without
>   the device core being aware of this.
> 
> Hence this patch that makes all probing work happen from nvme_probe()
> and that tells the device core to probe multiple NVMe controllers
> concurrently by setting PROBE_PREFER_ASYNCHRONOUS.
> 
> Cc: Mikulas Patocka <mpatocka@redhat.com>
> Cc: Keith Busch <keith.busch@intel.com>
> Cc: Greg KH <gregkh@linuxfoundation.org>
> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---

A fixes: tag?

thanks,

greg k-h

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] nvme: Boot as soon as the boot controller has been probed
  2020-11-08  4:09 [PATCH] nvme: Boot as soon as the boot controller has been probed Bart Van Assche
  2020-11-08  8:24 ` Greg KH
@ 2020-11-08 19:13 ` Sagi Grimberg
  2020-11-08 19:18   ` Greg KH
  2020-11-08 23:00   ` Bart Van Assche
  2020-11-09  3:04 ` Keith Busch
  2020-11-11 19:19 ` Keith Busch
  3 siblings, 2 replies; 18+ messages in thread
From: Sagi Grimberg @ 2020-11-08 19:13 UTC (permalink / raw)
  To: Bart Van Assche, Keith Busch
  Cc: Greg KH, Mikulas Patocka, Christoph Hellwig, linux-nvme


> The following two issues have been introduced by commit 1811977568e0
> ("nvme/pci: Use async_schedule for initial reset work"):
> - The boot process waits until all NVMe controllers have been probed
>    instead of only waiting until the boot controller has been probed.
>    This slows down the boot process.
> - Some of the controller probing work happens asynchronously without
>    the device core being aware of this.
> 
> Hence this patch that makes all probing work happen from nvme_probe()
> and that tells the device core to probe multiple NVMe controllers
> concurrently by setting PROBE_PREFER_ASYNCHRONOUS.

This would make the controller instance inconsistent across reboots
which is annoying for some users.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] nvme: Boot as soon as the boot controller has been probed
  2020-11-08 19:13 ` Sagi Grimberg
@ 2020-11-08 19:18   ` Greg KH
  2020-11-08 23:00   ` Bart Van Assche
  1 sibling, 0 replies; 18+ messages in thread
From: Greg KH @ 2020-11-08 19:18 UTC (permalink / raw)
  To: Sagi Grimberg
  Cc: Keith Busch, Mikulas Patocka, Bart Van Assche, linux-nvme,
	Christoph Hellwig

On Sun, Nov 08, 2020 at 11:13:11AM -0800, Sagi Grimberg wrote:
> 
> > The following two issues have been introduced by commit 1811977568e0
> > ("nvme/pci: Use async_schedule for initial reset work"):
> > - The boot process waits until all NVMe controllers have been probed
> >    instead of only waiting until the boot controller has been probed.
> >    This slows down the boot process.
> > - Some of the controller probing work happens asynchronously without
> >    the device core being aware of this.
> > 
> > Hence this patch that makes all probing work happen from nvme_probe()
> > and that tells the device core to probe multiple NVMe controllers
> > concurrently by setting PROBE_PREFER_ASYNCHRONOUS.
> 
> This would make the controller instance inconsistent across reboots
> which is annoying for some users.

Then they should use persistent device names, as that is what they are
there for :)

Bus naming persistance is not a thing, we have BIOSes that love to
renumber PCI busses every other boot quite frequently.  If you have such
whacky hardware, fix this in userspace which is where it has been done
for a while now.

thanks,

greg k-h

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] nvme: Boot as soon as the boot controller has been probed
  2020-11-08  8:24 ` Greg KH
@ 2020-11-08 22:31   ` Keith Busch
  2020-11-08 23:35     ` Bart Van Assche
  0 siblings, 1 reply; 18+ messages in thread
From: Keith Busch @ 2020-11-08 22:31 UTC (permalink / raw)
  To: Greg KH
  Cc: Bart Van Assche, linux-nvme, Keith Busch, Mikulas Patocka,
	Christoph Hellwig, Sagi Grimberg

On Sun, Nov 08, 2020 at 09:24:03AM +0100, Greg KH wrote:
> On Sat, Nov 07, 2020 at 08:09:03PM -0800, Bart Van Assche wrote:
> > The following two issues have been introduced by commit 1811977568e0
> > ("nvme/pci: Use async_schedule for initial reset work"):
> > - The boot process waits until all NVMe controllers have been probed
> >   instead of only waiting until the boot controller has been probed.
> >   This slows down the boot process.
> > - Some of the controller probing work happens asynchronously without
> >   the device core being aware of this.
> > 
> > Hence this patch that makes all probing work happen from nvme_probe()
> > and that tells the device core to probe multiple NVMe controllers
> > concurrently by setting PROBE_PREFER_ASYNCHRONOUS.
> > 
> > Cc: Mikulas Patocka <mpatocka@redhat.com>
> > Cc: Keith Busch <keith.busch@intel.com>
> > Cc: Greg KH <gregkh@linuxfoundation.org>
> > Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> > ---
> 
> A fixes: tag?

Why? Whether this is an improvement or not, the current code isn't
broken.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] nvme: Boot as soon as the boot controller has been probed
  2020-11-08 19:13 ` Sagi Grimberg
  2020-11-08 19:18   ` Greg KH
@ 2020-11-08 23:00   ` Bart Van Assche
  1 sibling, 0 replies; 18+ messages in thread
From: Bart Van Assche @ 2020-11-08 23:00 UTC (permalink / raw)
  To: Sagi Grimberg, Keith Busch
  Cc: Greg KH, Mikulas Patocka, Christoph Hellwig, linux-nvme

On 11/8/20 11:13 AM, Sagi Grimberg wrote:
> This would make the controller instance inconsistent across reboots
> which is annoying for some users.

Hi Sagi,

In addition to Greg's reply: a similar patch has been accepted upstream
for the SCSI sd driver more than a year ago. Only a single user ever
complained about inconsistencies across reboots. That user had not yet
heard about persistent device names but was happy to switch to
persistent device names after having learned about persistent device
names. See also commit f049cf1a7b67 ("scsi: sd: Rely on the driver core
for asynchronous probing").

Thanks,

Bart.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] nvme: Boot as soon as the boot controller has been probed
  2020-11-08 22:31   ` Keith Busch
@ 2020-11-08 23:35     ` Bart Van Assche
  2020-11-09  2:35       ` Keith Busch
  0 siblings, 1 reply; 18+ messages in thread
From: Bart Van Assche @ 2020-11-08 23:35 UTC (permalink / raw)
  To: Keith Busch, Greg KH
  Cc: Mikulas Patocka, Sagi Grimberg, linux-nvme, Christoph Hellwig

On 11/8/20 2:31 PM, Keith Busch wrote:
> On Sun, Nov 08, 2020 at 09:24:03AM +0100, Greg KH wrote:
>> On Sat, Nov 07, 2020 at 08:09:03PM -0800, Bart Van Assche wrote:
>>> The following two issues have been introduced by commit 1811977568e0
>>> ("nvme/pci: Use async_schedule for initial reset work"):
>>> - The boot process waits until all NVMe controllers have been probed
>>>   instead of only waiting until the boot controller has been probed.
>>>   This slows down the boot process.
>>> - Some of the controller probing work happens asynchronously without
>>>   the device core being aware of this.
>>>
>>> Hence this patch that makes all probing work happen from nvme_probe()
>>> and that tells the device core to probe multiple NVMe controllers
>>> concurrently by setting PROBE_PREFER_ASYNCHRONOUS.
>>>
>>> Cc: Mikulas Patocka <mpatocka@redhat.com>
>>> Cc: Keith Busch <keith.busch@intel.com>
>>> Cc: Greg KH <gregkh@linuxfoundation.org>
>>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>>> ---
>>
>> A fixes: tag?
> 
> Why? Whether this is an improvement or not, the current code isn't
> broken.

Hi Keith,

My understanding is that the device driver core generates a 'bind'
uevent after nvme_probe() returns (see also driver_bound() in
drivers/base/dd.c). Do you agree that emitting KOBJ_BIND while
nvme_reset_work() is in progress triggers a race condition between
nvme_reset_work() and udev rules that depend on the result of code in
nvme_reset_work(), e.g. NVMe queue creation or nvme_init_identify()?

Bart.


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] nvme: Boot as soon as the boot controller has been probed
  2020-11-08 23:35     ` Bart Van Assche
@ 2020-11-09  2:35       ` Keith Busch
  2020-11-09  4:00         ` Bart Van Assche
  0 siblings, 1 reply; 18+ messages in thread
From: Keith Busch @ 2020-11-09  2:35 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Greg KH, Mikulas Patocka, Sagi Grimberg, linux-nvme, Christoph Hellwig

On Sun, Nov 08, 2020 at 03:35:27PM -0800, Bart Van Assche wrote:
> On 11/8/20 2:31 PM, Keith Busch wrote:
> > On Sun, Nov 08, 2020 at 09:24:03AM +0100, Greg KH wrote:
> >> On Sat, Nov 07, 2020 at 08:09:03PM -0800, Bart Van Assche wrote:
> >>> The following two issues have been introduced by commit 1811977568e0
> >>> ("nvme/pci: Use async_schedule for initial reset work"):
> >>> - The boot process waits until all NVMe controllers have been probed
> >>>   instead of only waiting until the boot controller has been probed.
> >>>   This slows down the boot process.
> >>> - Some of the controller probing work happens asynchronously without
> >>>   the device core being aware of this.
> >>>
> >>> Hence this patch that makes all probing work happen from nvme_probe()
> >>> and that tells the device core to probe multiple NVMe controllers
> >>> concurrently by setting PROBE_PREFER_ASYNCHRONOUS.
> >>>
> >>> Cc: Mikulas Patocka <mpatocka@redhat.com>
> >>> Cc: Keith Busch <keith.busch@intel.com>
> >>> Cc: Greg KH <gregkh@linuxfoundation.org>
> >>> Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> >>> ---
> >>
> >> A fixes: tag?
> > 
> > Why? Whether this is an improvement or not, the current code isn't
> > broken.
> 
> Hi Keith,
> 
> My understanding is that the device driver core generates a 'bind'
> uevent after nvme_probe() returns (see also driver_bound() in
> drivers/base/dd.c). Do you agree that emitting KOBJ_BIND while
> nvme_reset_work() is in progress triggers a race condition between
> nvme_reset_work() and udev rules that depend on the result of code in
> nvme_reset_work(), e.g. NVMe queue creation or nvme_init_identify()?

The only user space visible artifact the driver creates on bind is the
controller character device, and access to that during a reset is
handled by the driver's state machine.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] nvme: Boot as soon as the boot controller has been probed
  2020-11-08  4:09 [PATCH] nvme: Boot as soon as the boot controller has been probed Bart Van Assche
  2020-11-08  8:24 ` Greg KH
  2020-11-08 19:13 ` Sagi Grimberg
@ 2020-11-09  3:04 ` Keith Busch
  2020-11-11 19:19 ` Keith Busch
  3 siblings, 0 replies; 18+ messages in thread
From: Keith Busch @ 2020-11-09  3:04 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Sagi Grimberg, Greg KH, linux-nvme, Keith Busch, Mikulas Patocka,
	Christoph Hellwig

On Sat, Nov 07, 2020 at 08:09:03PM -0800, Bart Van Assche wrote:
> The following two issues have been introduced by commit 1811977568e0
> ("nvme/pci: Use async_schedule for initial reset work"):
> - The boot process waits until all NVMe controllers have been probed
>   instead of only waiting until the boot controller has been probed.
>   This slows down the boot process.

Prior to that commit, the boot process didn't wait for anything at all
and missed the boot controller entirely.

> - Some of the controller probing work happens asynchronously without
>   the device core being aware of this.

Why is that an "issue"?

> Hence this patch that makes all probing work happen from nvme_probe()
> and that tells the device core to probe multiple NVMe controllers
> concurrently by setting PROBE_PREFER_ASYNCHRONOUS.

Afaics, that setting just may let driver core do an async_schedule() for
the asynchronous part, but that's what we're already doing. ?

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] nvme: Boot as soon as the boot controller has been probed
  2020-11-09  2:35       ` Keith Busch
@ 2020-11-09  4:00         ` Bart Van Assche
  2020-11-09 15:05           ` Keith Busch
  0 siblings, 1 reply; 18+ messages in thread
From: Bart Van Assche @ 2020-11-09  4:00 UTC (permalink / raw)
  To: Keith Busch
  Cc: Greg KH, Mikulas Patocka, Sagi Grimberg, linux-nvme, Christoph Hellwig

On 11/8/20 6:35 PM, Keith Busch wrote:
> The only user space visible artifact the driver creates on bind is the
> controller character device, and access to that during a reset is
> handled by the driver's state machine.

Hi Keith,

Is nvme_dev_fops associated with that character device? If so, in
nvme_dev_open() I found the following:

	switch (ctrl->state) {
	case NVME_CTRL_LIVE:
		break;
	default:
		return -EWOULDBLOCK;
	}

Does this mean that nvme-cli will succeed or fail depending on whether
or not nvme_reset_work() has finished? Isn't that something that can
cause trouble if nvme-cli is invoked from a udev rule?

Thanks,

Bart.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] nvme: Boot as soon as the boot controller has been probed
  2020-11-09  4:00         ` Bart Van Assche
@ 2020-11-09 15:05           ` Keith Busch
  2020-11-10  2:51             ` Bart Van Assche
  0 siblings, 1 reply; 18+ messages in thread
From: Keith Busch @ 2020-11-09 15:05 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Greg KH, Mikulas Patocka, Sagi Grimberg, linux-nvme, Christoph Hellwig

On Sun, Nov 08, 2020 at 08:00:42PM -0800, Bart Van Assche wrote:
> On 11/8/20 6:35 PM, Keith Busch wrote:
> > The only user space visible artifact the driver creates on bind is the
> > controller character device, and access to that during a reset is
> > handled by the driver's state machine.
> 
> Hi Keith,
> 
> Is nvme_dev_fops associated with that character device? If so, in
> nvme_dev_open() I found the following:
> 
> 	switch (ctrl->state) {
> 	case NVME_CTRL_LIVE:
> 		break;
> 	default:
> 		return -EWOULDBLOCK;
> 	}
> 
> Does this mean that nvme-cli will succeed or fail depending on whether
> or not nvme_reset_work() has finished? Isn't that something that can
> cause trouble if nvme-cli is invoked from a udev rule?

Resets can happen at any time, so user space should always have been
prepared for this. Is there actually a real case that is not working for
you?

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] nvme: Boot as soon as the boot controller has been probed
  2020-11-09 15:05           ` Keith Busch
@ 2020-11-10  2:51             ` Bart Van Assche
  2020-11-10  3:12               ` Keith Busch
  0 siblings, 1 reply; 18+ messages in thread
From: Bart Van Assche @ 2020-11-10  2:51 UTC (permalink / raw)
  To: Keith Busch
  Cc: Greg KH, Mikulas Patocka, Sagi Grimberg, linux-nvme, Christoph Hellwig

On 11/9/20 7:05 AM, Keith Busch wrote:
> Resets can happen at any time, so user space should always have been
> prepared for this. Is there actually a real case that is not working for
> you?

Hi Keith,

The use case of running nvme-cli from inside a udev rule is not
important to me personally. But reducing the boot time for systems with
multiple NVMe controllers matters to me. Do you want to recommend an
approach or do you perhaps expect me to explore the possible
alternatives further?

Thanks,

Bart.



_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] nvme: Boot as soon as the boot controller has been probed
  2020-11-10  2:51             ` Bart Van Assche
@ 2020-11-10  3:12               ` Keith Busch
  0 siblings, 0 replies; 18+ messages in thread
From: Keith Busch @ 2020-11-10  3:12 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Greg KH, Mikulas Patocka, Sagi Grimberg, linux-nvme, Christoph Hellwig

On Mon, Nov 09, 2020 at 06:51:48PM -0800, Bart Van Assche wrote:
> On 11/9/20 7:05 AM, Keith Busch wrote:
> > Resets can happen at any time, so user space should always have been
> > prepared for this. Is there actually a real case that is not working for
> > you?
> 
> Hi Keith,
> 
> The use case of running nvme-cli from inside a udev rule is not
> important to me personally. But reducing the boot time for systems with
> multiple NVMe controllers matters to me. Do you want to recommend an
> approach or do you perhaps expect me to explore the possible
> alternatives further?

I only mentioned the changelog doesn't require a fixes tag. I generally
don't have a problem with the patch if it really does improve boot
times, but I'd like to personally try it out through a few scenarios
first. I should be able to complete that this week.

The only comments on the code I have are that you can call
nvme_reset_ctrl_sync() instead of duplicating that, and you also need to
flush the scan_work after to ensure the boot namespace is available when
probe returns.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] nvme: Boot as soon as the boot controller has been probed
  2020-11-08  4:09 [PATCH] nvme: Boot as soon as the boot controller has been probed Bart Van Assche
                   ` (2 preceding siblings ...)
  2020-11-09  3:04 ` Keith Busch
@ 2020-11-11 19:19 ` Keith Busch
  2020-11-12  4:26   ` Bart Van Assche
  3 siblings, 1 reply; 18+ messages in thread
From: Keith Busch @ 2020-11-11 19:19 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Sagi Grimberg, Greg KH, linux-nvme, Keith Busch, Mikulas Patocka,
	Christoph Hellwig

On Sat, Nov 07, 2020 at 08:09:03PM -0800, Bart Van Assche wrote:
> The following two issues have been introduced by commit 1811977568e0
> ("nvme/pci: Use async_schedule for initial reset work"):
> - The boot process waits until all NVMe controllers have been probed
>   instead of only waiting until the boot controller has been probed.
>   This slows down the boot process.
> - Some of the controller probing work happens asynchronously without
>   the device core being aware of this.
> 
> Hence this patch that makes all probing work happen from nvme_probe()
> and that tells the device core to probe multiple NVMe controllers
> concurrently by setting PROBE_PREFER_ASYNCHRONOUS.

I am finding that this setting probes devices in parallel on boot up,
but serially for devices added after boot. That's making this a rather
unappealing patch.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] nvme: Boot as soon as the boot controller has been probed
  2020-11-11 19:19 ` Keith Busch
@ 2020-11-12  4:26   ` Bart Van Assche
  2020-11-12 14:20     ` Keith Busch
  0 siblings, 1 reply; 18+ messages in thread
From: Bart Van Assche @ 2020-11-12  4:26 UTC (permalink / raw)
  To: Keith Busch
  Cc: Greg KH, Mikulas Patocka, Sagi Grimberg, linux-nvme, Christoph Hellwig

On 11/11/20 11:19 AM, Keith Busch wrote:
> On Sat, Nov 07, 2020 at 08:09:03PM -0800, Bart Van Assche wrote:
>> The following two issues have been introduced by commit 1811977568e0
>> ("nvme/pci: Use async_schedule for initial reset work"):
>> - The boot process waits until all NVMe controllers have been probed
>>   instead of only waiting until the boot controller has been probed.
>>   This slows down the boot process.
>> - Some of the controller probing work happens asynchronously without
>>   the device core being aware of this.
>>
>> Hence this patch that makes all probing work happen from nvme_probe()
>> and that tells the device core to probe multiple NVMe controllers
>> concurrently by setting PROBE_PREFER_ASYNCHRONOUS.
> 
> I am finding that this setting probes devices in parallel on boot up,
> but serially for devices added after boot. That's making this a rather
> unappealing patch.

Hi Keith,

Thanks for having verified this. However, to me it is unexpected that my
patch results in serial probing of NVMe devices added after boot. It
would be appreciated if more information could be shared about how that
test was run. I'm wondering whether the serialization perhaps was the
result of how the test was run? My understanding of the driver core is
that the decision whether or not to probe concurrently is taken inside
__driver_attach() and also that PROBE_PREFER_ASYNCHRONOUS should trigger
a call of the following code:

		async_schedule_dev(__driver_attach_async_helper, dev);

I expect that calling the above code should result in concurrent probing.

Thanks,

Bart.



_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] nvme: Boot as soon as the boot controller has been probed
  2020-11-12  4:26   ` Bart Van Assche
@ 2020-11-12 14:20     ` Keith Busch
  2020-11-19  3:08       ` Bart Van Assche
  0 siblings, 1 reply; 18+ messages in thread
From: Keith Busch @ 2020-11-12 14:20 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Greg KH, Mikulas Patocka, Sagi Grimberg, linux-nvme, Christoph Hellwig

On Wed, Nov 11, 2020 at 08:26:12PM -0800, Bart Van Assche wrote:
> On 11/11/20 11:19 AM, Keith Busch wrote:
> > On Sat, Nov 07, 2020 at 08:09:03PM -0800, Bart Van Assche wrote:
> >> The following two issues have been introduced by commit 1811977568e0
> >> ("nvme/pci: Use async_schedule for initial reset work"):
> >> - The boot process waits until all NVMe controllers have been probed
> >>   instead of only waiting until the boot controller has been probed.
> >>   This slows down the boot process.
> >> - Some of the controller probing work happens asynchronously without
> >>   the device core being aware of this.
> >>
> >> Hence this patch that makes all probing work happen from nvme_probe()
> >> and that tells the device core to probe multiple NVMe controllers
> >> concurrently by setting PROBE_PREFER_ASYNCHRONOUS.
> > 
> > I am finding that this setting probes devices in parallel on boot up,
> > but serially for devices added after boot. That's making this a rather
> > unappealing patch.
> 
> Hi Keith,
> 
> Thanks for having verified this. However, to me it is unexpected that my
> patch results in serial probing of NVMe devices added after boot. It
> would be appreciated if more information could be shared about how that
> test was run. I'm wondering whether the serialization perhaps was the
> result of how the test was run? My understanding of the driver core is
> that the decision whether or not to probe concurrently is taken inside
> __driver_attach() and also that PROBE_PREFER_ASYNCHRONOUS should trigger
> a call of the following code:
> 
> 		async_schedule_dev(__driver_attach_async_helper, dev);
> 
> I expect that calling the above code should result in concurrent probing.

The easiest check is something like this (as long as you're not actively
using your nvme namespaces):

  # echo 1 | tee /sys/class/nvme/nvme*/device/remove
  # echo 1 > /sys/bus/pci/rescan

The subsequent probes don't go through the async_schedule. They have
this stack instead:

[   44.528906] Call Trace:
[   44.530512]  dump_stack+0x6d/0x88
[   44.532373]  nvme_probe+0x2f/0x59b [nvme]
[   44.534466]  local_pci_probe+0x3d/0x70
[   44.536471]  pci_device_probe+0x107/0x1b0
[   44.538562]  really_probe+0x1be/0x410
[   44.540564]  driver_probe_device+0xe1/0x150
[   44.542733]  ? driver_allows_async_probing+0x50/0x50
[   44.545170]  bus_for_each_drv+0x7b/0xc0
[   44.547172]  __device_attach+0xeb/0x170
[   44.549163]  pci_bus_add_device+0x4a/0x70
[   44.551182]  pci_bus_add_devices+0x2c/0x70
[   44.553229]  pci_rescan_bus+0x25/0x30
[   44.555126]  rescan_store+0x61/0x90
[   44.556975]  kernfs_fop_write+0xcb/0x1b0
[   44.558940]  vfs_write+0xbe/0x200
[   44.560710]  ksys_write+0x5f/0xe0
[   44.562483]  do_syscall_64+0x2d/0x40
[   44.564354]  entry_SYSCALL_64_after_hwframe+0x44/0xa9

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] nvme: Boot as soon as the boot controller has been probed
  2020-11-12 14:20     ` Keith Busch
@ 2020-11-19  3:08       ` Bart Van Assche
  2020-11-19 20:18         ` Keith Busch
  0 siblings, 1 reply; 18+ messages in thread
From: Bart Van Assche @ 2020-11-19  3:08 UTC (permalink / raw)
  To: Keith Busch
  Cc: Greg KH, Mikulas Patocka, Sagi Grimberg, linux-nvme, Christoph Hellwig

On 11/12/20 6:20 AM, Keith Busch wrote:
> The easiest check is something like this (as long as you're not actively
> using your nvme namespaces):
> 
>   # echo 1 | tee /sys/class/nvme/nvme*/device/remove
>   # echo 1 > /sys/bus/pci/rescan
> 
> The subsequent probes don't go through the async_schedule. They have
> this stack instead:
> 
> [   44.528906] Call Trace:
> [   44.530512]  dump_stack+0x6d/0x88
> [   44.532373]  nvme_probe+0x2f/0x59b [nvme]
> [   44.534466]  local_pci_probe+0x3d/0x70
> [   44.536471]  pci_device_probe+0x107/0x1b0
> [   44.538562]  really_probe+0x1be/0x410
> [   44.540564]  driver_probe_device+0xe1/0x150
> [   44.542733]  ? driver_allows_async_probing+0x50/0x50
> [   44.545170]  bus_for_each_drv+0x7b/0xc0
> [   44.547172]  __device_attach+0xeb/0x170
> [   44.549163]  pci_bus_add_device+0x4a/0x70
> [   44.551182]  pci_bus_add_devices+0x2c/0x70
> [   44.553229]  pci_rescan_bus+0x25/0x30
> [   44.555126]  rescan_store+0x61/0x90
> [   44.556975]  kernfs_fop_write+0xcb/0x1b0
> [   44.558940]  vfs_write+0xbe/0x200
> [   44.560710]  ksys_write+0x5f/0xe0
> [   44.562483]  do_syscall_64+0x2d/0x40
> [   44.564354]  entry_SYSCALL_64_after_hwframe+0x44/0xa9

Hi Keith,

Thanks for the feedback. I will look further into this but I wouldn't
mind if someone else would come up with a solution before I have a
solution ready.

Bart.



_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH] nvme: Boot as soon as the boot controller has been probed
  2020-11-19  3:08       ` Bart Van Assche
@ 2020-11-19 20:18         ` Keith Busch
  0 siblings, 0 replies; 18+ messages in thread
From: Keith Busch @ 2020-11-19 20:18 UTC (permalink / raw)
  To: Bart Van Assche
  Cc: Greg KH, Mikulas Patocka, Sagi Grimberg, linux-nvme, Christoph Hellwig

On Wed, Nov 18, 2020 at 07:08:49PM -0800, Bart Van Assche wrote:
> On 11/12/20 6:20 AM, Keith Busch wrote:
> > The easiest check is something like this (as long as you're not actively
> > using your nvme namespaces):
> > 
> >   # echo 1 | tee /sys/class/nvme/nvme*/device/remove
> >   # echo 1 > /sys/bus/pci/rescan
> > 
> > The subsequent probes don't go through the async_schedule. They have
> > this stack instead:
> > 
> > [   44.528906] Call Trace:
> > [   44.530512]  dump_stack+0x6d/0x88
> > [   44.532373]  nvme_probe+0x2f/0x59b [nvme]
> > [   44.534466]  local_pci_probe+0x3d/0x70
> > [   44.536471]  pci_device_probe+0x107/0x1b0
> > [   44.538562]  really_probe+0x1be/0x410
> > [   44.540564]  driver_probe_device+0xe1/0x150
> > [   44.542733]  ? driver_allows_async_probing+0x50/0x50
> > [   44.545170]  bus_for_each_drv+0x7b/0xc0
> > [   44.547172]  __device_attach+0xeb/0x170
> > [   44.549163]  pci_bus_add_device+0x4a/0x70
> > [   44.551182]  pci_bus_add_devices+0x2c/0x70
> > [   44.553229]  pci_rescan_bus+0x25/0x30
> > [   44.555126]  rescan_store+0x61/0x90
> > [   44.556975]  kernfs_fop_write+0xcb/0x1b0
> > [   44.558940]  vfs_write+0xbe/0x200
> > [   44.560710]  ksys_write+0x5f/0xe0
> > [   44.562483]  do_syscall_64+0x2d/0x40
> > [   44.564354]  entry_SYSCALL_64_after_hwframe+0x44/0xa9
> 
> Hi Keith,
> 
> Thanks for the feedback. I will look further into this but I wouldn't
> mind if someone else would come up with a solution before I have a
> solution ready.

I suppose you could just add the PROBE_PREFER_ASYNCHRONOUS probe_type,
and let nvme_probe() continue to attempt its own async initialization.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2020-11-19 20:18 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-08  4:09 [PATCH] nvme: Boot as soon as the boot controller has been probed Bart Van Assche
2020-11-08  8:24 ` Greg KH
2020-11-08 22:31   ` Keith Busch
2020-11-08 23:35     ` Bart Van Assche
2020-11-09  2:35       ` Keith Busch
2020-11-09  4:00         ` Bart Van Assche
2020-11-09 15:05           ` Keith Busch
2020-11-10  2:51             ` Bart Van Assche
2020-11-10  3:12               ` Keith Busch
2020-11-08 19:13 ` Sagi Grimberg
2020-11-08 19:18   ` Greg KH
2020-11-08 23:00   ` Bart Van Assche
2020-11-09  3:04 ` Keith Busch
2020-11-11 19:19 ` Keith Busch
2020-11-12  4:26   ` Bart Van Assche
2020-11-12 14:20     ` Keith Busch
2020-11-19  3:08       ` Bart Van Assche
2020-11-19 20:18         ` Keith Busch

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.