Re: [PATCH RESENT] nvme-pci: suspend queues based on online_queues
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 */ >> -- >
Re: [PATCH RESENT] nvme-pci: suspend queues based on online_queues
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 */ > --
Re: [PATCH RESENT] nvme-pci: suspend queues based on online_queues
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 > > 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:c14d8240fc10 EFLAGS: 00010086 [ 2269.961559] RAX: RBX: 0083 RCX: [ 2269.961563] RDX: 0002 RSI: b56dd5e1 RDI: 0001 [ 2269.961567] RBP: 9cd03aed04d0 R08: 0001 R09: [ 2269.961570] R10: c14d8240fb88 R11: b46f7b64 R12: 0083 [ 2269.961574] R13: 9cd0626ab5d8 R14: 9cd0626ab4a8 R15: 9cd0626ab400 [ 2269.961578] FS: () GS:9cd0a2c8() knlGS: [ 2269.961582] CS: 0010 DS: ES: CR0: 80050033 [ 2269.961586] CR2: 01e7b9a0 CR3: 00020ae0f005 CR4: 003606e0 [ 2269.961590] DR0: DR1: DR2: [ 2269.961594] DR3: DR6: fffe0ff0 DR7: 0400 [ 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-n...@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= >
Re: [PATCH RESENT] nvme-pci: suspend queues based on online_queues
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 Can I get a review for this?
[PATCH RESENT] nvme-pci: suspend queues based on online_queues
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 --- 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