All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH RESENT] nvme-pci: suspend queues based on online_queues
@ 2018-02-12 13:05 ` Jianchao Wang
  0 siblings, 0 replies; 10+ messages in thread
From: Jianchao Wang @ 2018-02-12 13:05 UTC (permalink / raw)
  To: keith.busch, axboe, hch, sagi; +Cc: linux-nvme, linux-kernel

nvme cq irq is freed based on queue_count. When the sq/cq creation
fails, irq will not be setup. free_irq will warn 'Try to free
already-free irq'.

To fix it, we only increase online_queues when adminq/sq/cq are
created and associated irq is setup. Then suspend queues based
on online_queues.

Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com>
---
 drivers/nvme/host/pci.c | 41 ++++++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 6e5d2ca..9b3cc2c 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1315,9 +1315,6 @@ static int nvme_suspend_queue(struct nvme_queue *nvmeq)
 	nvmeq->cq_vector = -1;
 	spin_unlock_irq(&nvmeq->q_lock);
 
-	if (!nvmeq->qid && nvmeq->dev->ctrl.admin_q)
-		blk_mq_quiesce_queue(nvmeq->dev->ctrl.admin_q);
-
 	pci_free_irq(to_pci_dev(nvmeq->dev->dev), vector, nvmeq);
 
 	return 0;
@@ -1461,13 +1458,14 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid)
 	nvme_init_queue(nvmeq, qid);
 	result = queue_request_irq(nvmeq);
 	if (result < 0)
-		goto release_sq;
+		goto offline;
 
 	return result;
 
- release_sq:
+offline:
+	dev->online_queues--;
 	adapter_delete_sq(dev, qid);
- release_cq:
+release_cq:
 	adapter_delete_cq(dev, qid);
 	return result;
 }
@@ -1607,6 +1605,7 @@ static int nvme_pci_configure_admin_queue(struct nvme_dev *dev)
 	result = queue_request_irq(nvmeq);
 	if (result) {
 		nvmeq->cq_vector = -1;
+		dev->online_queues--;
 		return result;
 	}
 
@@ -1954,6 +1953,7 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
 	result = queue_request_irq(adminq);
 	if (result) {
 		adminq->cq_vector = -1;
+		dev->online_queues--;
 		return result;
 	}
 	return nvme_create_io_queues(dev);
@@ -2167,6 +2167,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 	int i;
 	bool dead = true;
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
+	int onlines;
 
 	mutex_lock(&dev->shutdown_lock);
 	if (pci_is_enabled(pdev)) {
@@ -2175,8 +2176,11 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 		if (dev->ctrl.state == NVME_CTRL_LIVE ||
 		    dev->ctrl.state == NVME_CTRL_RESETTING)
 			nvme_start_freeze(&dev->ctrl);
-		dead = !!((csts & NVME_CSTS_CFS) || !(csts & NVME_CSTS_RDY) ||
-			pdev->error_state  != pci_channel_io_normal);
+
+		dead = !!((csts & NVME_CSTS_CFS) ||
+				!(csts & NVME_CSTS_RDY) ||
+				(pdev->error_state  != pci_channel_io_normal) ||
+				(dev->online_queues == 0));
 	}
 
 	/*
@@ -2200,9 +2204,14 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 		nvme_disable_io_queues(dev);
 		nvme_disable_admin_queue(dev, shutdown);
 	}
-	for (i = dev->ctrl.queue_count - 1; i >= 0; i--)
+
+	onlines = dev->online_queues;
+	for (i = onlines - 1; i >= 0; i--)
 		nvme_suspend_queue(&dev->queues[i]);
 
+	if (dev->ctrl.admin_q)
+		blk_mq_quiesce_queue(dev->ctrl.admin_q);
+
 	nvme_pci_disable(dev);
 
 	blk_mq_tagset_busy_iter(&dev->tagset, nvme_cancel_request, &dev->ctrl);
@@ -2341,16 +2350,18 @@ static void nvme_reset_work(struct work_struct *work)
 	if (result)
 		goto out;
 
-	/*
-	 * Keep the controller around but remove all namespaces if we don't have
-	 * any working I/O queue.
-	 */
-	if (dev->online_queues < 2) {
+
+	/* In case of online_queues is zero, it has gone to out */
+	if (dev->online_queues == 1) {
+		/*
+		 * Keep the controller around but remove all namespaces if we
+		 * don't have any working I/O queue.
+		 */
 		dev_warn(dev->ctrl.device, "IO queues not created\n");
 		nvme_kill_queues(&dev->ctrl);
 		nvme_remove_namespaces(&dev->ctrl);
 		new_state = NVME_CTRL_ADMIN_ONLY;
-	} else {
+	} else if (dev->online_queues > 1) {
 		nvme_start_queues(&dev->ctrl);
 		nvme_wait_freeze(&dev->ctrl);
 		/* hit this only when allocate tagset fails */
-- 
2.7.4

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

* [PATCH RESENT] nvme-pci: suspend queues based on online_queues
@ 2018-02-12 13:05 ` Jianchao Wang
  0 siblings, 0 replies; 10+ messages in thread
From: Jianchao Wang @ 2018-02-12 13:05 UTC (permalink / raw)


nvme cq irq is freed based on queue_count. When the sq/cq creation
fails, irq will not be setup. free_irq will warn 'Try to free
already-free irq'.

To fix it, we only increase online_queues when adminq/sq/cq are
created and associated irq is setup. Then suspend queues based
on online_queues.

Signed-off-by: Jianchao Wang <jianchao.w.wang at oracle.com>
---
 drivers/nvme/host/pci.c | 41 ++++++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 6e5d2ca..9b3cc2c 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1315,9 +1315,6 @@ static int nvme_suspend_queue(struct nvme_queue *nvmeq)
 	nvmeq->cq_vector = -1;
 	spin_unlock_irq(&nvmeq->q_lock);
 
-	if (!nvmeq->qid && nvmeq->dev->ctrl.admin_q)
-		blk_mq_quiesce_queue(nvmeq->dev->ctrl.admin_q);
-
 	pci_free_irq(to_pci_dev(nvmeq->dev->dev), vector, nvmeq);
 
 	return 0;
@@ -1461,13 +1458,14 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid)
 	nvme_init_queue(nvmeq, qid);
 	result = queue_request_irq(nvmeq);
 	if (result < 0)
-		goto release_sq;
+		goto offline;
 
 	return result;
 
- release_sq:
+offline:
+	dev->online_queues--;
 	adapter_delete_sq(dev, qid);
- release_cq:
+release_cq:
 	adapter_delete_cq(dev, qid);
 	return result;
 }
@@ -1607,6 +1605,7 @@ static int nvme_pci_configure_admin_queue(struct nvme_dev *dev)
 	result = queue_request_irq(nvmeq);
 	if (result) {
 		nvmeq->cq_vector = -1;
+		dev->online_queues--;
 		return result;
 	}
 
@@ -1954,6 +1953,7 @@ static int nvme_setup_io_queues(struct nvme_dev *dev)
 	result = queue_request_irq(adminq);
 	if (result) {
 		adminq->cq_vector = -1;
+		dev->online_queues--;
 		return result;
 	}
 	return nvme_create_io_queues(dev);
@@ -2167,6 +2167,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 	int i;
 	bool dead = true;
 	struct pci_dev *pdev = to_pci_dev(dev->dev);
+	int onlines;
 
 	mutex_lock(&dev->shutdown_lock);
 	if (pci_is_enabled(pdev)) {
@@ -2175,8 +2176,11 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 		if (dev->ctrl.state == NVME_CTRL_LIVE ||
 		    dev->ctrl.state == NVME_CTRL_RESETTING)
 			nvme_start_freeze(&dev->ctrl);
-		dead = !!((csts & NVME_CSTS_CFS) || !(csts & NVME_CSTS_RDY) ||
-			pdev->error_state  != pci_channel_io_normal);
+
+		dead = !!((csts & NVME_CSTS_CFS) ||
+				!(csts & NVME_CSTS_RDY) ||
+				(pdev->error_state  != pci_channel_io_normal) ||
+				(dev->online_queues == 0));
 	}
 
 	/*
@@ -2200,9 +2204,14 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
 		nvme_disable_io_queues(dev);
 		nvme_disable_admin_queue(dev, shutdown);
 	}
-	for (i = dev->ctrl.queue_count - 1; i >= 0; i--)
+
+	onlines = dev->online_queues;
+	for (i = onlines - 1; i >= 0; i--)
 		nvme_suspend_queue(&dev->queues[i]);
 
+	if (dev->ctrl.admin_q)
+		blk_mq_quiesce_queue(dev->ctrl.admin_q);
+
 	nvme_pci_disable(dev);
 
 	blk_mq_tagset_busy_iter(&dev->tagset, nvme_cancel_request, &dev->ctrl);
@@ -2341,16 +2350,18 @@ static void nvme_reset_work(struct work_struct *work)
 	if (result)
 		goto out;
 
-	/*
-	 * Keep the controller around but remove all namespaces if we don't have
-	 * any working I/O queue.
-	 */
-	if (dev->online_queues < 2) {
+
+	/* In case of online_queues is zero, it has gone to out */
+	if (dev->online_queues == 1) {
+		/*
+		 * Keep the controller around but remove all namespaces if we
+		 * don't have any working I/O queue.
+		 */
 		dev_warn(dev->ctrl.device, "IO queues not created\n");
 		nvme_kill_queues(&dev->ctrl);
 		nvme_remove_namespaces(&dev->ctrl);
 		new_state = NVME_CTRL_ADMIN_ONLY;
-	} else {
+	} else if (dev->online_queues > 1) {
 		nvme_start_queues(&dev->ctrl);
 		nvme_wait_freeze(&dev->ctrl);
 		/* hit this only when allocate tagset fails */
-- 
2.7.4

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

* Re: [PATCH RESENT] nvme-pci: suspend queues based on online_queues
  2018-02-12 13:05 ` Jianchao Wang
@ 2018-02-12 18:37   ` Sagi Grimberg
  -1 siblings, 0 replies; 10+ messages in thread
From: Sagi Grimberg @ 2018-02-12 18:37 UTC (permalink / raw)
  To: Jianchao Wang, keith.busch, axboe, hch; +Cc: linux-nvme, linux-kernel


> nvme cq irq is freed based on queue_count. When the sq/cq creation
> fails, irq will not be setup. free_irq will warn 'Try to free
> already-free irq'.
> 
> To fix it, we only increase online_queues when adminq/sq/cq are
> created and associated irq is setup. Then suspend queues based
> on online_queues.
> 
> Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com>

Can I get a review for this?

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

* [PATCH RESENT] nvme-pci: suspend queues based on online_queues
@ 2018-02-12 18:37   ` Sagi Grimberg
  0 siblings, 0 replies; 10+ messages in thread
From: Sagi Grimberg @ 2018-02-12 18:37 UTC (permalink / raw)



> nvme cq irq is freed based on queue_count. When the sq/cq creation
> fails, irq will not be setup. free_irq will warn 'Try to free
> already-free irq'.
> 
> To fix it, we only increase online_queues when adminq/sq/cq are
> created and associated irq is setup. Then suspend queues based
> on online_queues.
> 
> Signed-off-by: Jianchao Wang <jianchao.w.wang at oracle.com>

Can I get a review for this?

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

* Re: [PATCH RESENT] nvme-pci: suspend queues based on online_queues
  2018-02-12 18:37   ` Sagi Grimberg
@ 2018-02-13  1:41     ` jianchao.wang
  -1 siblings, 0 replies; 10+ messages in thread
From: jianchao.wang @ 2018-02-13  1:41 UTC (permalink / raw)
  To: Sagi Grimberg, keith.busch, axboe, hch; +Cc: linux-kernel, linux-nvme

Hi Sagi

Thanks for your kindly response.

On 02/13/2018 02:37 AM, Sagi Grimberg wrote:
> 
>> nvme cq irq is freed based on queue_count. When the sq/cq creation
>> fails, irq will not be setup. free_irq will warn 'Try to free
>> already-free irq'.
>>
>> To fix it, we only increase online_queues when adminq/sq/cq are
>> created and associated irq is setup. Then suspend queues based
>> on online_queues.
>>
>> Signed-off-by: Jianchao Wang <jianchao.w.wang@oracle.com>
> 
> Can I get a review for this?
> 
Here is the log

[ 2269.936597] nvme nvme0: Removing after probe failure status: -4
[ 2269.961238] ------------[ cut here ]------------
[ 2269.961279] Trying to free already-free IRQ 131
[ 2269.961299] WARNING: CPU: 2 PID: 134 at /home/will/u04/source_code/linux-block/kernel/irq/manage.c:1546 __free_irq+0xa6/0x2a0
[ 2269.961302] Modules linked in: nls_iso8859_1 snd_hda_codec_hdmi snd_hda_codec_realtek intel_rapl x86_pkg_temp_thermal snd_hda_codec_generic intel_powerclamp coretemp kvm_intel snd_hda_intel kvm snd_hda_codec snd_hda_core snd_hwdep snd_pcm irqbypass snd_seq_midi snd_seq_midi_event crct10dif_pclmul crc32_pclmul input_leds ghash_clmulni_intel pcbc snd_rawmidi snd_seq aesni_intel aes_x86_64 crypto_simd snd_seq_device glue_helper snd_timer cryptd snd intel_cstate soundcore intel_rapl_perf mei_me wmi_bmof intel_wmi_thunderbolt acpi_pad tpm_crb shpchp mei mac_hid ib_iser rdma_cm iw_cm ib_cm ib_core parport_pc ppdev lp parport autofs4 i915 i2c_algo_bit drm_kms_helper hid_generic syscopyarea sysfillrect sysimgblt usbhid fb_sys_fops drm hid e1000e ptp ahci pps_core libahci wmi video
[ 2269.961525] CPU: 2 PID: 134 Comm: kworker/u16:2 Not tainted 4.15.0-rc9+ #68
[ 2269.961529] Hardware name: LENOVO 10MLS0E339/3106, BIOS M1AKT22A 06/27/2017
[ 2269.961537] Workqueue: nvme-reset-wq nvme_reset_work
[ 2269.961548] RIP: 0010:__free_irq+0xa6/0x2a0
[ 2269.961552] RSP: 0018:ffffc14d8240fc10 EFLAGS: 00010086
[ 2269.961559] RAX: 0000000000000000 RBX: 0000000000000083 RCX: 0000000000000000
[ 2269.961563] RDX: 0000000000000002 RSI: ffffffffb56dd5e1 RDI: 0000000000000001
[ 2269.961567] RBP: ffff9cd03aed04d0 R08: 0000000000000001 R09: 0000000000000000
[ 2269.961570] R10: ffffc14d8240fb88 R11: ffffffffb46f7b64 R12: 0000000000000083
[ 2269.961574] R13: ffff9cd0626ab5d8 R14: ffff9cd0626ab4a8 R15: ffff9cd0626ab400
[ 2269.961578] FS:  0000000000000000(0000) GS:ffff9cd0a2c80000(0000) knlGS:0000000000000000
[ 2269.961582] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 2269.961586] CR2: 0000000001e7b9a0 CR3: 000000020ae0f005 CR4: 00000000003606e0
[ 2269.961590] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 2269.961594] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 2269.961597] Call Trace:
[ 2269.961616]  free_irq+0x30/0x60
[ 2269.961624]  pci_free_irq+0x18/0x30
[ 2269.961630]  nvme_dev_disable+0x35b/0x4f0
[ 2269.961639]  ? __nvme_submit_sync_cmd+0xa2/0xd0
[ 2269.961651]  ? dev_warn+0x64/0x80
[ 2269.961670]  nvme_reset_work+0x198/nvme-pci: fixes on nvme_timeout and nvme_dev_disable patchset0x15d0
[ 2269.961715]  process_one_work+0x1e9/0x6f0
[ 2269.961732]  worker_thread+0x4a/0x430
[ 2269.961749]  kthread+0x100/0x140
[ 2269.961757]  ? process_one_work+0x6f0/0x6f0
[ 2269.961763]  ? kthread_delayed_work_timer_fn+0x80/0x80
[ 2269.961773]  ? kthread_delayed_work_timer_fn+0x80/0x80
[ 2269.961781]  ret_from_fork+0x24/0x30


After this patch, I've never seen this again.
On the other hand, even though it was seen with my nvme-pci: fixes on nvme_timeout and nvme_dev_disable patchset,
but this issue should also exist on current source code.

Because the Chinese Spring Festival Vacation is coming and looks like some more talking is still need on the patchset
of nvme-pci: fixes on nvme_timeout and nvme_dev_disable. So I send out some of the relatively independent patches of
that patchset, including this one.

Sincerely
Jianchao 

> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme@lists.infradead.org
> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.infradead.org_mailman_listinfo_linux-2Dnvme&d=DwICAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=7WdAxUBeiTUTCy8v-7zXyr4qk7sx26ATvfo6QSTvZyQ&m=YBEprmLALFZHeJ5S3c_TM8FQwXgZhi2GaUYn3i4T7DA&s=pN0FrPI10CfrgET0crnpV8EJs8sHN5MKaB7fZ6OWGHQ&e=
> 

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

* [PATCH RESENT] nvme-pci: suspend queues based on online_queues
@ 2018-02-13  1:41     ` jianchao.wang
  0 siblings, 0 replies; 10+ messages in thread
From: jianchao.wang @ 2018-02-13  1:41 UTC (permalink / raw)


Hi Sagi

Thanks for your kindly response.

On 02/13/2018 02:37 AM, Sagi Grimberg wrote:
> 
>> nvme cq irq is freed based on queue_count. When the sq/cq creation
>> fails, irq will not be setup. free_irq will warn 'Try to free
>> already-free irq'.
>>
>> To fix it, we only increase online_queues when adminq/sq/cq are
>> created and associated irq is setup. Then suspend queues based
>> on online_queues.
>>
>> Signed-off-by: Jianchao Wang <jianchao.w.wang at oracle.com>
> 
> Can I get a review for this?
> 
Here is the log

[ 2269.936597] nvme nvme0: Removing after probe failure status: -4
[ 2269.961238] ------------[ cut here ]------------
[ 2269.961279] Trying to free already-free IRQ 131
[ 2269.961299] WARNING: CPU: 2 PID: 134 at /home/will/u04/source_code/linux-block/kernel/irq/manage.c:1546 __free_irq+0xa6/0x2a0
[ 2269.961302] Modules linked in: nls_iso8859_1 snd_hda_codec_hdmi snd_hda_codec_realtek intel_rapl x86_pkg_temp_thermal snd_hda_codec_generic intel_powerclamp coretemp kvm_intel snd_hda_intel kvm snd_hda_codec snd_hda_core snd_hwdep snd_pcm irqbypass snd_seq_midi snd_seq_midi_event crct10dif_pclmul crc32_pclmul input_leds ghash_clmulni_intel pcbc snd_rawmidi snd_seq aesni_intel aes_x86_64 crypto_simd snd_seq_device glue_helper snd_timer cryptd snd intel_cstate soundcore intel_rapl_perf mei_me wmi_bmof intel_wmi_thunderbolt acpi_pad tpm_crb shpchp mei mac_hid ib_iser rdma_cm iw_cm ib_cm ib_core parport_pc ppdev lp parport autofs4 i915 i2c_algo_bit drm_kms_helper hid_generic syscopyarea sysfillrect sysimgblt usbhid fb_sys_fops drm hid e1000e ptp ahci pps_core libahci wmi video
[ 2269.961525] CPU: 2 PID: 134 Comm: kworker/u16:2 Not tainted 4.15.0-rc9+ #68
[ 2269.961529] Hardware name: LENOVO 10MLS0E339/3106, BIOS M1AKT22A 06/27/2017
[ 2269.961537] Workqueue: nvme-reset-wq nvme_reset_work
[ 2269.961548] RIP: 0010:__free_irq+0xa6/0x2a0
[ 2269.961552] RSP: 0018:ffffc14d8240fc10 EFLAGS: 00010086
[ 2269.961559] RAX: 0000000000000000 RBX: 0000000000000083 RCX: 0000000000000000
[ 2269.961563] RDX: 0000000000000002 RSI: ffffffffb56dd5e1 RDI: 0000000000000001
[ 2269.961567] RBP: ffff9cd03aed04d0 R08: 0000000000000001 R09: 0000000000000000
[ 2269.961570] R10: ffffc14d8240fb88 R11: ffffffffb46f7b64 R12: 0000000000000083
[ 2269.961574] R13: ffff9cd0626ab5d8 R14: ffff9cd0626ab4a8 R15: ffff9cd0626ab400
[ 2269.961578] FS:  0000000000000000(0000) GS:ffff9cd0a2c80000(0000) knlGS:0000000000000000
[ 2269.961582] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 2269.961586] CR2: 0000000001e7b9a0 CR3: 000000020ae0f005 CR4: 00000000003606e0
[ 2269.961590] DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
[ 2269.961594] DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
[ 2269.961597] Call Trace:
[ 2269.961616]  free_irq+0x30/0x60
[ 2269.961624]  pci_free_irq+0x18/0x30
[ 2269.961630]  nvme_dev_disable+0x35b/0x4f0
[ 2269.961639]  ? __nvme_submit_sync_cmd+0xa2/0xd0
[ 2269.961651]  ? dev_warn+0x64/0x80
[ 2269.961670]  nvme_reset_work+0x198/nvme-pci: fixes on nvme_timeout and nvme_dev_disable patchset0x15d0
[ 2269.961715]  process_one_work+0x1e9/0x6f0
[ 2269.961732]  worker_thread+0x4a/0x430
[ 2269.961749]  kthread+0x100/0x140
[ 2269.961757]  ? process_one_work+0x6f0/0x6f0
[ 2269.961763]  ? kthread_delayed_work_timer_fn+0x80/0x80
[ 2269.961773]  ? kthread_delayed_work_timer_fn+0x80/0x80
[ 2269.961781]  ret_from_fork+0x24/0x30


After this patch, I've never seen this again.
On the other hand, even though it was seen with my nvme-pci: fixes on nvme_timeout and nvme_dev_disable patchset,
but this issue should also exist on current source code.

Because the Chinese Spring Festival Vacation is coming and looks like some more talking is still need on the patchset
of nvme-pci: fixes on nvme_timeout and nvme_dev_disable. So I send out some of the relatively independent patches of
that patchset, including this one.

Sincerely
Jianchao 

> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme at lists.infradead.org
> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.infradead.org_mailman_listinfo_linux-2Dnvme&d=DwICAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=7WdAxUBeiTUTCy8v-7zXyr4qk7sx26ATvfo6QSTvZyQ&m=YBEprmLALFZHeJ5S3c_TM8FQwXgZhi2GaUYn3i4T7DA&s=pN0FrPI10CfrgET0crnpV8EJs8sHN5MKaB7fZ6OWGHQ&e=
> 

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

* Re: [PATCH RESENT] nvme-pci: suspend queues based on online_queues
  2018-02-12 13:05 ` Jianchao Wang
@ 2018-02-13 21:52   ` Keith Busch
  -1 siblings, 0 replies; 10+ messages in thread
From: Keith Busch @ 2018-02-13 21:52 UTC (permalink / raw)
  To: Jianchao Wang; +Cc: axboe, hch, sagi, linux-nvme, linux-kernel

On Mon, Feb 12, 2018 at 09:05:13PM +0800, Jianchao Wang wrote:
> @@ -1315,9 +1315,6 @@ static int nvme_suspend_queue(struct nvme_queue *nvmeq)
>  	nvmeq->cq_vector = -1;
>  	spin_unlock_irq(&nvmeq->q_lock);
>  
> -	if (!nvmeq->qid && nvmeq->dev->ctrl.admin_q)
> -		blk_mq_quiesce_queue(nvmeq->dev->ctrl.admin_q);
> -

This doesn't look related to this patch.

>  	pci_free_irq(to_pci_dev(nvmeq->dev->dev), vector, nvmeq);
>  
>  	return 0;
> @@ -1461,13 +1458,14 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid)
>  	nvme_init_queue(nvmeq, qid);
>  	result = queue_request_irq(nvmeq);
>  	if (result < 0)
> -		goto release_sq;
> +		goto offline;
>  
>  	return result;
>  
> - release_sq:
> +offline:
> +	dev->online_queues--;
>  	adapter_delete_sq(dev, qid);

In addition to the above, set nvmeq->cq_vector to -1.

Everything else that follows doesn't appear to be necessary.

> @@ -2167,6 +2167,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
>  	int i;
>  	bool dead = true;
>  	struct pci_dev *pdev = to_pci_dev(dev->dev);
> +	int onlines;
>  
>  	mutex_lock(&dev->shutdown_lock);
>  	if (pci_is_enabled(pdev)) {
> @@ -2175,8 +2176,11 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
>  		if (dev->ctrl.state == NVME_CTRL_LIVE ||
>  		    dev->ctrl.state == NVME_CTRL_RESETTING)
>  			nvme_start_freeze(&dev->ctrl);
> -		dead = !!((csts & NVME_CSTS_CFS) || !(csts & NVME_CSTS_RDY) ||
> -			pdev->error_state  != pci_channel_io_normal);
> +
> +		dead = !!((csts & NVME_CSTS_CFS) ||
> +				!(csts & NVME_CSTS_RDY) ||
> +				(pdev->error_state  != pci_channel_io_normal) ||
> +				(dev->online_queues == 0));
>  	}
>  
>  	/*
> @@ -2200,9 +2204,14 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
>  		nvme_disable_io_queues(dev);
>  		nvme_disable_admin_queue(dev, shutdown);
>  	}
> -	for (i = dev->ctrl.queue_count - 1; i >= 0; i--)
> +
> +	onlines = dev->online_queues;
> +	for (i = onlines - 1; i >= 0; i--)
>  		nvme_suspend_queue(&dev->queues[i]);
>  
> +	if (dev->ctrl.admin_q)
> +		blk_mq_quiesce_queue(dev->ctrl.admin_q);
> +
>  	nvme_pci_disable(dev);
>  
>  	blk_mq_tagset_busy_iter(&dev->tagset, nvme_cancel_request, &dev->ctrl);
> @@ -2341,16 +2350,18 @@ static void nvme_reset_work(struct work_struct *work)
>  	if (result)
>  		goto out;
>  
> -	/*
> -	 * Keep the controller around but remove all namespaces if we don't have
> -	 * any working I/O queue.
> -	 */
> -	if (dev->online_queues < 2) {
> +
> +	/* In case of online_queues is zero, it has gone to out */
> +	if (dev->online_queues == 1) {
> +		/*
> +		 * Keep the controller around but remove all namespaces if we
> +		 * don't have any working I/O queue.
> +		 */
>  		dev_warn(dev->ctrl.device, "IO queues not created\n");
>  		nvme_kill_queues(&dev->ctrl);
>  		nvme_remove_namespaces(&dev->ctrl);
>  		new_state = NVME_CTRL_ADMIN_ONLY;
> -	} else {
> +	} else if (dev->online_queues > 1) {
>  		nvme_start_queues(&dev->ctrl);
>  		nvme_wait_freeze(&dev->ctrl);
>  		/* hit this only when allocate tagset fails */
> -- 

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

* [PATCH RESENT] nvme-pci: suspend queues based on online_queues
@ 2018-02-13 21:52   ` Keith Busch
  0 siblings, 0 replies; 10+ messages in thread
From: Keith Busch @ 2018-02-13 21:52 UTC (permalink / raw)


On Mon, Feb 12, 2018@09:05:13PM +0800, Jianchao Wang wrote:
> @@ -1315,9 +1315,6 @@ static int nvme_suspend_queue(struct nvme_queue *nvmeq)
>  	nvmeq->cq_vector = -1;
>  	spin_unlock_irq(&nvmeq->q_lock);
>  
> -	if (!nvmeq->qid && nvmeq->dev->ctrl.admin_q)
> -		blk_mq_quiesce_queue(nvmeq->dev->ctrl.admin_q);
> -

This doesn't look related to this patch.

>  	pci_free_irq(to_pci_dev(nvmeq->dev->dev), vector, nvmeq);
>  
>  	return 0;
> @@ -1461,13 +1458,14 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid)
>  	nvme_init_queue(nvmeq, qid);
>  	result = queue_request_irq(nvmeq);
>  	if (result < 0)
> -		goto release_sq;
> +		goto offline;
>  
>  	return result;
>  
> - release_sq:
> +offline:
> +	dev->online_queues--;
>  	adapter_delete_sq(dev, qid);

In addition to the above, set nvmeq->cq_vector to -1.

Everything else that follows doesn't appear to be necessary.

> @@ -2167,6 +2167,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
>  	int i;
>  	bool dead = true;
>  	struct pci_dev *pdev = to_pci_dev(dev->dev);
> +	int onlines;
>  
>  	mutex_lock(&dev->shutdown_lock);
>  	if (pci_is_enabled(pdev)) {
> @@ -2175,8 +2176,11 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
>  		if (dev->ctrl.state == NVME_CTRL_LIVE ||
>  		    dev->ctrl.state == NVME_CTRL_RESETTING)
>  			nvme_start_freeze(&dev->ctrl);
> -		dead = !!((csts & NVME_CSTS_CFS) || !(csts & NVME_CSTS_RDY) ||
> -			pdev->error_state  != pci_channel_io_normal);
> +
> +		dead = !!((csts & NVME_CSTS_CFS) ||
> +				!(csts & NVME_CSTS_RDY) ||
> +				(pdev->error_state  != pci_channel_io_normal) ||
> +				(dev->online_queues == 0));
>  	}
>  
>  	/*
> @@ -2200,9 +2204,14 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
>  		nvme_disable_io_queues(dev);
>  		nvme_disable_admin_queue(dev, shutdown);
>  	}
> -	for (i = dev->ctrl.queue_count - 1; i >= 0; i--)
> +
> +	onlines = dev->online_queues;
> +	for (i = onlines - 1; i >= 0; i--)
>  		nvme_suspend_queue(&dev->queues[i]);
>  
> +	if (dev->ctrl.admin_q)
> +		blk_mq_quiesce_queue(dev->ctrl.admin_q);
> +
>  	nvme_pci_disable(dev);
>  
>  	blk_mq_tagset_busy_iter(&dev->tagset, nvme_cancel_request, &dev->ctrl);
> @@ -2341,16 +2350,18 @@ static void nvme_reset_work(struct work_struct *work)
>  	if (result)
>  		goto out;
>  
> -	/*
> -	 * Keep the controller around but remove all namespaces if we don't have
> -	 * any working I/O queue.
> -	 */
> -	if (dev->online_queues < 2) {
> +
> +	/* In case of online_queues is zero, it has gone to out */
> +	if (dev->online_queues == 1) {
> +		/*
> +		 * Keep the controller around but remove all namespaces if we
> +		 * don't have any working I/O queue.
> +		 */
>  		dev_warn(dev->ctrl.device, "IO queues not created\n");
>  		nvme_kill_queues(&dev->ctrl);
>  		nvme_remove_namespaces(&dev->ctrl);
>  		new_state = NVME_CTRL_ADMIN_ONLY;
> -	} else {
> +	} else if (dev->online_queues > 1) {
>  		nvme_start_queues(&dev->ctrl);
>  		nvme_wait_freeze(&dev->ctrl);
>  		/* hit this only when allocate tagset fails */
> -- 

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

* Re: [PATCH RESENT] nvme-pci: suspend queues based on online_queues
  2018-02-13 21:52   ` Keith Busch
@ 2018-02-15  0:33     ` jianchao.wang
  -1 siblings, 0 replies; 10+ messages in thread
From: jianchao.wang @ 2018-02-15  0:33 UTC (permalink / raw)
  To: Keith Busch; +Cc: axboe, hch, sagi, linux-nvme, linux-kernel

Hi Keith

Thanks for your kindly response and directive.
And 恭喜发财 大吉大利!!

On 02/14/2018 05:52 AM, Keith Busch wrote:
> On Mon, Feb 12, 2018 at 09:05:13PM +0800, Jianchao Wang wrote:
>> @@ -1315,9 +1315,6 @@ static int nvme_suspend_queue(struct nvme_queue *nvmeq)
>>  	nvmeq->cq_vector = -1;
>>  	spin_unlock_irq(&nvmeq->q_lock);
>>  
>> -	if (!nvmeq->qid && nvmeq->dev->ctrl.admin_q)
>> -		blk_mq_quiesce_queue(nvmeq->dev->ctrl.admin_q);
>> -
> 
> This doesn't look related to this patch.
> 
>>  	pci_free_irq(to_pci_dev(nvmeq->dev->dev), vector, nvmeq);
>>  
>>  	return 0;
>> @@ -1461,13 +1458,14 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid)
>>  	nvme_init_queue(nvmeq, qid);
>>  	result = queue_request_irq(nvmeq);
>>  	if (result < 0)
>> -		goto release_sq;
>> +		goto offline;
>>  
>>  	return result;
>>  
>> - release_sq:
>> +offline:
>> +	dev->online_queues--;
>>  	adapter_delete_sq(dev, qid);
> 
> In addition to the above, set nvmeq->cq_vector to -1.

Yes, absolutely.

> 
> Everything else that follows doesn't appear to be necessary.

Yes, nvme_suspend_queues will return when it finds the cq_vector is -1.


Sincerely
Jianchao

> 
>> @@ -2167,6 +2167,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
>>  	int i;
>>  	bool dead = true;
>>  	struct pci_dev *pdev = to_pci_dev(dev->dev);
>> +	int onlines;
>>  
>>  	mutex_lock(&dev->shutdown_lock);
>>  	if (pci_is_enabled(pdev)) {
>> @@ -2175,8 +2176,11 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
>>  		if (dev->ctrl.state == NVME_CTRL_LIVE ||
>>  		    dev->ctrl.state == NVME_CTRL_RESETTING)
>>  			nvme_start_freeze(&dev->ctrl);
>> -		dead = !!((csts & NVME_CSTS_CFS) || !(csts & NVME_CSTS_RDY) ||
>> -			pdev->error_state  != pci_channel_io_normal);
>> +
>> +		dead = !!((csts & NVME_CSTS_CFS) ||
>> +				!(csts & NVME_CSTS_RDY) ||
>> +				(pdev->error_state  != pci_channel_io_normal) ||
>> +				(dev->online_queues == 0));
>>  	}
>>  
>>  	/*
>> @@ -2200,9 +2204,14 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
>>  		nvme_disable_io_queues(dev);
>>  		nvme_disable_admin_queue(dev, shutdown);
>>  	}
>> -	for (i = dev->ctrl.queue_count - 1; i >= 0; i--)
>> +
>> +	onlines = dev->online_queues;
>> +	for (i = onlines - 1; i >= 0; i--)
>>  		nvme_suspend_queue(&dev->queues[i]);
>>  
>> +	if (dev->ctrl.admin_q)
>> +		blk_mq_quiesce_queue(dev->ctrl.admin_q);
>> +
>>  	nvme_pci_disable(dev);
>>  
>>  	blk_mq_tagset_busy_iter(&dev->tagset, nvme_cancel_request, &dev->ctrl);
>> @@ -2341,16 +2350,18 @@ static void nvme_reset_work(struct work_struct *work)
>>  	if (result)
>>  		goto out;
>>  
>> -	/*
>> -	 * Keep the controller around but remove all namespaces if we don't have
>> -	 * any working I/O queue.
>> -	 */
>> -	if (dev->online_queues < 2) {
>> +
>> +	/* In case of online_queues is zero, it has gone to out */
>> +	if (dev->online_queues == 1) {
>> +		/*
>> +		 * Keep the controller around but remove all namespaces if we
>> +		 * don't have any working I/O queue.
>> +		 */
>>  		dev_warn(dev->ctrl.device, "IO queues not created\n");
>>  		nvme_kill_queues(&dev->ctrl);
>>  		nvme_remove_namespaces(&dev->ctrl);
>>  		new_state = NVME_CTRL_ADMIN_ONLY;
>> -	} else {
>> +	} else if (dev->online_queues > 1) {
>>  		nvme_start_queues(&dev->ctrl);
>>  		nvme_wait_freeze(&dev->ctrl);
>>  		/* hit this only when allocate tagset fails */
>> -- 
> 

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

* [PATCH RESENT] nvme-pci: suspend queues based on online_queues
@ 2018-02-15  0:33     ` jianchao.wang
  0 siblings, 0 replies; 10+ messages in thread
From: jianchao.wang @ 2018-02-15  0:33 UTC (permalink / raw)


Hi Keith

Thanks for your kindly response and directive.
And ???? ??????

On 02/14/2018 05:52 AM, Keith Busch wrote:
> On Mon, Feb 12, 2018@09:05:13PM +0800, Jianchao Wang wrote:
>> @@ -1315,9 +1315,6 @@ static int nvme_suspend_queue(struct nvme_queue *nvmeq)
>>  	nvmeq->cq_vector = -1;
>>  	spin_unlock_irq(&nvmeq->q_lock);
>>  
>> -	if (!nvmeq->qid && nvmeq->dev->ctrl.admin_q)
>> -		blk_mq_quiesce_queue(nvmeq->dev->ctrl.admin_q);
>> -
> 
> This doesn't look related to this patch.
> 
>>  	pci_free_irq(to_pci_dev(nvmeq->dev->dev), vector, nvmeq);
>>  
>>  	return 0;
>> @@ -1461,13 +1458,14 @@ static int nvme_create_queue(struct nvme_queue *nvmeq, int qid)
>>  	nvme_init_queue(nvmeq, qid);
>>  	result = queue_request_irq(nvmeq);
>>  	if (result < 0)
>> -		goto release_sq;
>> +		goto offline;
>>  
>>  	return result;
>>  
>> - release_sq:
>> +offline:
>> +	dev->online_queues--;
>>  	adapter_delete_sq(dev, qid);
> 
> In addition to the above, set nvmeq->cq_vector to -1.

Yes, absolutely.

> 
> Everything else that follows doesn't appear to be necessary.

Yes, nvme_suspend_queues will return when it finds the cq_vector is -1.


Sincerely
Jianchao

> 
>> @@ -2167,6 +2167,7 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
>>  	int i;
>>  	bool dead = true;
>>  	struct pci_dev *pdev = to_pci_dev(dev->dev);
>> +	int onlines;
>>  
>>  	mutex_lock(&dev->shutdown_lock);
>>  	if (pci_is_enabled(pdev)) {
>> @@ -2175,8 +2176,11 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
>>  		if (dev->ctrl.state == NVME_CTRL_LIVE ||
>>  		    dev->ctrl.state == NVME_CTRL_RESETTING)
>>  			nvme_start_freeze(&dev->ctrl);
>> -		dead = !!((csts & NVME_CSTS_CFS) || !(csts & NVME_CSTS_RDY) ||
>> -			pdev->error_state  != pci_channel_io_normal);
>> +
>> +		dead = !!((csts & NVME_CSTS_CFS) ||
>> +				!(csts & NVME_CSTS_RDY) ||
>> +				(pdev->error_state  != pci_channel_io_normal) ||
>> +				(dev->online_queues == 0));
>>  	}
>>  
>>  	/*
>> @@ -2200,9 +2204,14 @@ static void nvme_dev_disable(struct nvme_dev *dev, bool shutdown)
>>  		nvme_disable_io_queues(dev);
>>  		nvme_disable_admin_queue(dev, shutdown);
>>  	}
>> -	for (i = dev->ctrl.queue_count - 1; i >= 0; i--)
>> +
>> +	onlines = dev->online_queues;
>> +	for (i = onlines - 1; i >= 0; i--)
>>  		nvme_suspend_queue(&dev->queues[i]);
>>  
>> +	if (dev->ctrl.admin_q)
>> +		blk_mq_quiesce_queue(dev->ctrl.admin_q);
>> +
>>  	nvme_pci_disable(dev);
>>  
>>  	blk_mq_tagset_busy_iter(&dev->tagset, nvme_cancel_request, &dev->ctrl);
>> @@ -2341,16 +2350,18 @@ static void nvme_reset_work(struct work_struct *work)
>>  	if (result)
>>  		goto out;
>>  
>> -	/*
>> -	 * Keep the controller around but remove all namespaces if we don't have
>> -	 * any working I/O queue.
>> -	 */
>> -	if (dev->online_queues < 2) {
>> +
>> +	/* In case of online_queues is zero, it has gone to out */
>> +	if (dev->online_queues == 1) {
>> +		/*
>> +		 * Keep the controller around but remove all namespaces if we
>> +		 * don't have any working I/O queue.
>> +		 */
>>  		dev_warn(dev->ctrl.device, "IO queues not created\n");
>>  		nvme_kill_queues(&dev->ctrl);
>>  		nvme_remove_namespaces(&dev->ctrl);
>>  		new_state = NVME_CTRL_ADMIN_ONLY;
>> -	} else {
>> +	} else if (dev->online_queues > 1) {
>>  		nvme_start_queues(&dev->ctrl);
>>  		nvme_wait_freeze(&dev->ctrl);
>>  		/* hit this only when allocate tagset fails */
>> -- 
> 

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

end of thread, other threads:[~2018-02-15  0:38 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-02-12 13:05 [PATCH RESENT] nvme-pci: suspend queues based on online_queues Jianchao Wang
2018-02-12 13:05 ` Jianchao Wang
2018-02-12 18:37 ` Sagi Grimberg
2018-02-12 18:37   ` Sagi Grimberg
2018-02-13  1:41   ` jianchao.wang
2018-02-13  1:41     ` jianchao.wang
2018-02-13 21:52 ` Keith Busch
2018-02-13 21:52   ` Keith Busch
2018-02-15  0:33   ` jianchao.wang
2018-02-15  0:33     ` jianchao.wang

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.