* [PATCH V6 0/3] blk-mq: fix blk_mq_alloc_request_hctx @ 2021-07-22 9:52 Ming Lei 2021-07-22 9:52 ` [PATCH V6 1/3] genirq: add device_has_managed_msi_irq Ming Lei ` (4 more replies) 0 siblings, 5 replies; 14+ messages in thread From: Ming Lei @ 2021-07-22 9:52 UTC (permalink / raw) To: Jens Axboe, Christoph Hellwig, linux-block, Thomas Gleixner Cc: Greg Kroah-Hartman, John Garry, Sagi Grimberg, Daniel Wagner, Wen Xiong, Hannes Reinecke, Ming Lei Hi, blk_mq_alloc_request_hctx() is used by NVMe fc/rdma/tcp/loop to connect io queue. Also the sw ctx is chosen as the 1st online cpu in hctx->cpumask. However, all cpus in hctx->cpumask may be offline. This usage model isn't well supported by blk-mq which supposes allocator is always done on one online CPU in hctx->cpumask. This assumption is related with managed irq, which also requires blk-mq to drain inflight request in this hctx when the last cpu in hctx->cpumask is going to offline. However, NVMe fc/rdma/tcp/loop don't use managed irq, so we should allow them to ask for request allocation when the specified hctx is inactive (all cpus in hctx->cpumask are offline). Fix blk_mq_alloc_request_hctx() by allowing to allocate request when all CPUs of this hctx are offline. Wen Xiong has verified V4 in her nvmef test. V6: - move device_has_managed_msi_irq() into kernel/irq/msi.c V5: - take John Garry's suggestion to replace device field with new helper of device_has_managed_msi_irq() V4: - remove patches for cleanup queue map helpers - take Christoph's suggestion to add field into 'struct device' for describing if managed irq is allocated from one device V3: - cleanup map queues helpers, and remove pci/virtio/rdma queue helpers - store use managed irq info into qmap V2: - use flag of BLK_MQ_F_MANAGED_IRQ - pass BLK_MQ_F_MANAGED_IRQ from driver explicitly - kill BLK_MQ_F_STACKING Ming Lei (3): genirq: add device_has_managed_msi_irq blk-mq: mark if one queue map uses managed irq blk-mq: don't deactivate hctx if managed irq isn't used block/blk-mq-pci.c | 2 ++ block/blk-mq-rdma.c | 7 +++++++ block/blk-mq-virtio.c | 2 ++ block/blk-mq.c | 27 ++++++++++++++++---------- block/blk-mq.h | 8 ++++++++ drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 1 + include/linux/blk-mq.h | 3 ++- include/linux/msi.h | 5 +++++ kernel/irq/msi.c | 18 +++++++++++++++++ 9 files changed, 62 insertions(+), 11 deletions(-) -- 2.31.1 ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH V6 1/3] genirq: add device_has_managed_msi_irq 2021-07-22 9:52 [PATCH V6 0/3] blk-mq: fix blk_mq_alloc_request_hctx Ming Lei @ 2021-07-22 9:52 ` Ming Lei 2021-07-22 13:05 ` Christoph Hellwig 2021-07-22 9:52 ` [PATCH V6 2/3] blk-mq: mark if one queue map uses managed irq Ming Lei ` (3 subsequent siblings) 4 siblings, 1 reply; 14+ messages in thread From: Ming Lei @ 2021-07-22 9:52 UTC (permalink / raw) To: Jens Axboe, Christoph Hellwig, linux-block, Thomas Gleixner Cc: Greg Kroah-Hartman, John Garry, Sagi Grimberg, Daniel Wagner, Wen Xiong, Hannes Reinecke, Ming Lei irq vector allocation with managed affinity may be used by driver, and blk-mq needs this info for draining queue because genirq core will shutdown managed irq when all CPUs in the affinity mask are offline. The info of using managed irq is often produced by drivers, and it is consumed by blk-mq, so different subsystems are involved in this info flow. Address this issue by adding one helper of device_has_managed_msi_irq() which is suggested by John Garry. Suggested-by: John Garry <john.garry@huawei.com> Signed-off-by: Ming Lei <ming.lei@redhat.com> --- include/linux/msi.h | 5 +++++ kernel/irq/msi.c | 18 ++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/include/linux/msi.h b/include/linux/msi.h index 6aff469e511d..2d0f3962d3c3 100644 --- a/include/linux/msi.h +++ b/include/linux/msi.h @@ -59,10 +59,15 @@ struct platform_msi_priv_data; void __get_cached_msi_msg(struct msi_desc *entry, struct msi_msg *msg); #ifdef CONFIG_GENERIC_MSI_IRQ void get_cached_msi_msg(unsigned int irq, struct msi_msg *msg); +bool device_has_managed_msi_irq(struct device *dev); #else static inline void get_cached_msi_msg(unsigned int irq, struct msi_msg *msg) { } +static inline bool device_has_managed_msi_irq(struct device *dev) +{ + return false; +} #endif typedef void (*irq_write_msi_msg_t)(struct msi_desc *desc, diff --git a/kernel/irq/msi.c b/kernel/irq/msi.c index c41965e348b5..01aae22246c8 100644 --- a/kernel/irq/msi.c +++ b/kernel/irq/msi.c @@ -69,6 +69,24 @@ void get_cached_msi_msg(unsigned int irq, struct msi_msg *msg) } EXPORT_SYMBOL_GPL(get_cached_msi_msg); +/** + * device_has_managed_msi_irq - Query if device has managed irq entry + * @dev: Pointer to the device for which we want to query + * + * Return true if there is managed irq vector allocated on this device + */ +bool device_has_managed_msi_irq(struct device *dev) +{ + struct msi_desc *desc; + + for_each_msi_entry(desc, dev) { + if (desc->affinity && desc->affinity->is_managed) + return true; + } + return false; +} +EXPORT_SYMBOL_GPL(device_has_managed_msi_irq); + #ifdef CONFIG_GENERIC_MSI_IRQ_DOMAIN static inline void irq_chip_write_msi_msg(struct irq_data *data, struct msi_msg *msg) -- 2.31.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH V6 1/3] genirq: add device_has_managed_msi_irq 2021-07-22 9:52 ` [PATCH V6 1/3] genirq: add device_has_managed_msi_irq Ming Lei @ 2021-07-22 13:05 ` Christoph Hellwig 0 siblings, 0 replies; 14+ messages in thread From: Christoph Hellwig @ 2021-07-22 13:05 UTC (permalink / raw) To: Ming Lei Cc: Jens Axboe, Christoph Hellwig, linux-block, Thomas Gleixner, Greg Kroah-Hartman, John Garry, Sagi Grimberg, Daniel Wagner, Wen Xiong, Hannes Reinecke On Thu, Jul 22, 2021 at 05:52:44PM +0800, Ming Lei wrote: > irq vector allocation with managed affinity may be used by driver, and > blk-mq needs this info for draining queue because genirq core will shutdown > managed irq when all CPUs in the affinity mask are offline. > > The info of using managed irq is often produced by drivers, and it is > consumed by blk-mq, so different subsystems are involved in this info flow. > > Address this issue by adding one helper of device_has_managed_msi_irq() > which is suggested by John Garry. This implies non-MSI irqs can't be managed, which is true right now. If the irq maintaines are fine with that: Reviewed-by: Christoph Hellwig <hch@lst.de> ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH V6 2/3] blk-mq: mark if one queue map uses managed irq 2021-07-22 9:52 [PATCH V6 0/3] blk-mq: fix blk_mq_alloc_request_hctx Ming Lei 2021-07-22 9:52 ` [PATCH V6 1/3] genirq: add device_has_managed_msi_irq Ming Lei @ 2021-07-22 9:52 ` Ming Lei 2021-07-22 13:06 ` Christoph Hellwig 2021-07-22 9:52 ` [PATCH V6 3/3] blk-mq: don't deactivate hctx if managed irq isn't used Ming Lei ` (2 subsequent siblings) 4 siblings, 1 reply; 14+ messages in thread From: Ming Lei @ 2021-07-22 9:52 UTC (permalink / raw) To: Jens Axboe, Christoph Hellwig, linux-block, Thomas Gleixner Cc: Greg Kroah-Hartman, John Garry, Sagi Grimberg, Daniel Wagner, Wen Xiong, Hannes Reinecke, Ming Lei Retrieve this info via new added helper of device_has_managed_msi_irq, then we can decide if one hctx needs to be drained before all its CPUs become offline. Reviewed-by: Christoph Hellwig <hch@lst.de> Reviewed-by: John Garry <john.garry@huawei.com> Signed-off-by: Ming Lei <ming.lei@redhat.com> --- block/blk-mq-pci.c | 2 ++ block/blk-mq-rdma.c | 7 +++++++ block/blk-mq-virtio.c | 2 ++ drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 1 + include/linux/blk-mq.h | 3 ++- 5 files changed, 14 insertions(+), 1 deletion(-) diff --git a/block/blk-mq-pci.c b/block/blk-mq-pci.c index b595a94c4d16..e452cda0896a 100644 --- a/block/blk-mq-pci.c +++ b/block/blk-mq-pci.c @@ -8,6 +8,7 @@ #include <linux/blk-mq-pci.h> #include <linux/pci.h> #include <linux/module.h> +#include <linux/msi.h> #include "blk-mq.h" @@ -37,6 +38,7 @@ int blk_mq_pci_map_queues(struct blk_mq_queue_map *qmap, struct pci_dev *pdev, for_each_cpu(cpu, mask) qmap->mq_map[cpu] = qmap->queue_offset + queue; } + qmap->use_managed_irq = device_has_managed_msi_irq(&pdev->dev); return 0; diff --git a/block/blk-mq-rdma.c b/block/blk-mq-rdma.c index 14f968e58b8f..19ad31c44eab 100644 --- a/block/blk-mq-rdma.c +++ b/block/blk-mq-rdma.c @@ -36,6 +36,13 @@ int blk_mq_rdma_map_queues(struct blk_mq_queue_map *map, map->mq_map[cpu] = map->queue_offset + queue; } + /* + * RDMA doesn't use managed irq, and nvme rdma driver can allocate + * and submit requests on specified hctx via + * blk_mq_alloc_request_hctx + */ + map->use_managed_irq = false; + return 0; fallback: diff --git a/block/blk-mq-virtio.c b/block/blk-mq-virtio.c index 7b8a42c35102..2ce39fb77dce 100644 --- a/block/blk-mq-virtio.c +++ b/block/blk-mq-virtio.c @@ -7,6 +7,7 @@ #include <linux/blk-mq-virtio.h> #include <linux/virtio_config.h> #include <linux/module.h> +#include <linux/msi.h> #include "blk-mq.h" /** @@ -38,6 +39,7 @@ int blk_mq_virtio_map_queues(struct blk_mq_queue_map *qmap, for_each_cpu(cpu, mask) qmap->mq_map[cpu] = qmap->queue_offset + queue; } + qmap->use_managed_irq = device_has_managed_msi_irq(&vdev->dev); return 0; fallback: diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c index b0b2361e63fe..7d7df261d346 100644 --- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c +++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c @@ -3562,6 +3562,7 @@ static int map_queues_v2_hw(struct Scsi_Host *shost) for_each_cpu(cpu, mask) qmap->mq_map[cpu] = qmap->queue_offset + queue; } + qmap->use_managed_irq = true; return 0; diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h index 1d18447ebebc..d54a795ec971 100644 --- a/include/linux/blk-mq.h +++ b/include/linux/blk-mq.h @@ -192,7 +192,8 @@ struct blk_mq_hw_ctx { struct blk_mq_queue_map { unsigned int *mq_map; unsigned int nr_queues; - unsigned int queue_offset; + unsigned int queue_offset:31; + unsigned int use_managed_irq:1; }; /** -- 2.31.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH V6 2/3] blk-mq: mark if one queue map uses managed irq 2021-07-22 9:52 ` [PATCH V6 2/3] blk-mq: mark if one queue map uses managed irq Ming Lei @ 2021-07-22 13:06 ` Christoph Hellwig 2021-07-22 15:40 ` Ming Lei 0 siblings, 1 reply; 14+ messages in thread From: Christoph Hellwig @ 2021-07-22 13:06 UTC (permalink / raw) To: Ming Lei Cc: Jens Axboe, Christoph Hellwig, linux-block, Thomas Gleixner, Greg Kroah-Hartman, John Garry, Sagi Grimberg, Daniel Wagner, Wen Xiong, Hannes Reinecke On Thu, Jul 22, 2021 at 05:52:45PM +0800, Ming Lei wrote: and nvme rdma driver can allocate > + * and submit requests on specified hctx via > + * blk_mq_alloc_request_hctx Why does that matter for this setting? ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V6 2/3] blk-mq: mark if one queue map uses managed irq 2021-07-22 13:06 ` Christoph Hellwig @ 2021-07-22 15:40 ` Ming Lei 2021-07-22 16:06 ` Christoph Hellwig 0 siblings, 1 reply; 14+ messages in thread From: Ming Lei @ 2021-07-22 15:40 UTC (permalink / raw) To: Christoph Hellwig Cc: Jens Axboe, linux-block, Thomas Gleixner, Greg Kroah-Hartman, John Garry, Sagi Grimberg, Daniel Wagner, Wen Xiong, Hannes Reinecke On Thu, Jul 22, 2021 at 03:06:09PM +0200, Christoph Hellwig wrote: > On Thu, Jul 22, 2021 at 05:52:45PM +0800, Ming Lei wrote: > and nvme rdma driver can allocate > > + * and submit requests on specified hctx via > > + * blk_mq_alloc_request_hctx > > Why does that matter for this setting? blk_mq_alloc_request_hctx() has been broken for long time, which can only work if the hctx isn't driven by non-managed irq. Thanks, Ming ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V6 2/3] blk-mq: mark if one queue map uses managed irq 2021-07-22 15:40 ` Ming Lei @ 2021-07-22 16:06 ` Christoph Hellwig 0 siblings, 0 replies; 14+ messages in thread From: Christoph Hellwig @ 2021-07-22 16:06 UTC (permalink / raw) To: Ming Lei Cc: Christoph Hellwig, Jens Axboe, linux-block, Thomas Gleixner, Greg Kroah-Hartman, John Garry, Sagi Grimberg, Daniel Wagner, Wen Xiong, Hannes Reinecke, James Smart On Thu, Jul 22, 2021 at 11:40:19PM +0800, Ming Lei wrote: > On Thu, Jul 22, 2021 at 03:06:09PM +0200, Christoph Hellwig wrote: > > On Thu, Jul 22, 2021 at 05:52:45PM +0800, Ming Lei wrote: > > and nvme rdma driver can allocate > > > + * and submit requests on specified hctx via > > > + * blk_mq_alloc_request_hctx > > > > Why does that matter for this setting? > > blk_mq_alloc_request_hctx() has been broken for long time, which > can only work if the hctx isn't driven by non-managed irq. The qla2xxx and lpfc drivers uses managed interrupt. I'm not quite sure if the nvme-fc queues directly map to those interrupts, though. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH V6 3/3] blk-mq: don't deactivate hctx if managed irq isn't used 2021-07-22 9:52 [PATCH V6 0/3] blk-mq: fix blk_mq_alloc_request_hctx Ming Lei 2021-07-22 9:52 ` [PATCH V6 1/3] genirq: add device_has_managed_msi_irq Ming Lei 2021-07-22 9:52 ` [PATCH V6 2/3] blk-mq: mark if one queue map uses managed irq Ming Lei @ 2021-07-22 9:52 ` Ming Lei 2021-08-18 9:38 ` John Garry 2021-07-22 13:12 ` [PATCH V6 0/3] blk-mq: fix blk_mq_alloc_request_hctx Daniel Wagner [not found] ` <OFDADF39F5.DDB99A55-ON0025871A.00794382-0025871A.00797A2E@ibm.com> 4 siblings, 1 reply; 14+ messages in thread From: Ming Lei @ 2021-07-22 9:52 UTC (permalink / raw) To: Jens Axboe, Christoph Hellwig, linux-block, Thomas Gleixner Cc: Greg Kroah-Hartman, John Garry, Sagi Grimberg, Daniel Wagner, Wen Xiong, Hannes Reinecke, Ming Lei blk-mq deactivates one hctx when the last CPU in hctx->cpumask become offline by draining all requests originated from this hctx and moving new allocation to other active hctx. This way is for avoiding inflight IO in case of managed irq because managed irq is shutdown when the last CPU in the irq's affinity becomes offline. However, lots of drivers(nvme fc, rdma, tcp, loop, ...) don't use managed irq, so they needn't to deactivate hctx when the last CPU becomes offline. Also, some of them are the only user of blk_mq_alloc_request_hctx() which is used for connecting io queue. And their requirement is that the connect request needs to be submitted successfully via one specified hctx even though all CPUs in this hctx->cpumask have become offline. Addressing the requirement for nvme fc/rdma/loop by allowing to allocate request from one hctx when all CPUs in this hctx are offline, since these drivers don't use managed irq. Finally don't deactivate one hctx when it doesn't use managed irq. Reviewed-by: Christoph Hellwig <hch@lst.de> Signed-off-by: Ming Lei <ming.lei@redhat.com> --- block/blk-mq.c | 27 +++++++++++++++++---------- block/blk-mq.h | 8 ++++++++ 2 files changed, 25 insertions(+), 10 deletions(-) diff --git a/block/blk-mq.c b/block/blk-mq.c index 2c4ac51e54eb..591ab07c64d8 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -427,6 +427,15 @@ struct request *blk_mq_alloc_request(struct request_queue *q, unsigned int op, } EXPORT_SYMBOL(blk_mq_alloc_request); +static inline int blk_mq_first_mapped_cpu(struct blk_mq_hw_ctx *hctx) +{ + int cpu = cpumask_first_and(hctx->cpumask, cpu_online_mask); + + if (cpu >= nr_cpu_ids) + cpu = cpumask_first(hctx->cpumask); + return cpu; +} + struct request *blk_mq_alloc_request_hctx(struct request_queue *q, unsigned int op, blk_mq_req_flags_t flags, unsigned int hctx_idx) { @@ -468,7 +477,10 @@ struct request *blk_mq_alloc_request_hctx(struct request_queue *q, data.hctx = q->queue_hw_ctx[hctx_idx]; if (!blk_mq_hw_queue_mapped(data.hctx)) goto out_queue_exit; - cpu = cpumask_first_and(data.hctx->cpumask, cpu_online_mask); + + WARN_ON_ONCE(blk_mq_hctx_use_managed_irq(data.hctx)); + + cpu = blk_mq_first_mapped_cpu(data.hctx); data.ctx = __blk_mq_get_ctx(q, cpu); if (!q->elevator) @@ -1501,15 +1513,6 @@ static void __blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx) hctx_unlock(hctx, srcu_idx); } -static inline int blk_mq_first_mapped_cpu(struct blk_mq_hw_ctx *hctx) -{ - int cpu = cpumask_first_and(hctx->cpumask, cpu_online_mask); - - if (cpu >= nr_cpu_ids) - cpu = cpumask_first(hctx->cpumask); - return cpu; -} - /* * It'd be great if the workqueue API had a way to pass * in a mask and had some smarts for more clever placement. @@ -2556,6 +2559,10 @@ static int blk_mq_hctx_notify_offline(unsigned int cpu, struct hlist_node *node) struct blk_mq_hw_ctx *hctx = hlist_entry_safe(node, struct blk_mq_hw_ctx, cpuhp_online); + /* hctx needn't to be deactivated in case managed irq isn't used */ + if (!blk_mq_hctx_use_managed_irq(hctx)) + return 0; + if (!cpumask_test_cpu(cpu, hctx->cpumask) || !blk_mq_last_cpu_in_hctx(cpu, hctx)) return 0; diff --git a/block/blk-mq.h b/block/blk-mq.h index d08779f77a26..7333b659d8f5 100644 --- a/block/blk-mq.h +++ b/block/blk-mq.h @@ -119,6 +119,14 @@ static inline struct blk_mq_hw_ctx *blk_mq_map_queue(struct request_queue *q, return ctx->hctxs[type]; } +static inline bool blk_mq_hctx_use_managed_irq(struct blk_mq_hw_ctx *hctx) +{ + if (hctx->type == HCTX_TYPE_POLL) + return false; + + return hctx->queue->tag_set->map[hctx->type].use_managed_irq; +} + /* * sysfs helpers */ -- 2.31.1 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH V6 3/3] blk-mq: don't deactivate hctx if managed irq isn't used 2021-07-22 9:52 ` [PATCH V6 3/3] blk-mq: don't deactivate hctx if managed irq isn't used Ming Lei @ 2021-08-18 9:38 ` John Garry 2021-08-18 10:32 ` Ming Lei 0 siblings, 1 reply; 14+ messages in thread From: John Garry @ 2021-08-18 9:38 UTC (permalink / raw) To: Ming Lei, Jens Axboe, Christoph Hellwig, linux-block, Thomas Gleixner Cc: Greg Kroah-Hartman, Sagi Grimberg, Daniel Wagner, Wen Xiong, Hannes Reinecke On 22/07/2021 10:52, Ming Lei wrote: > blk-mq deactivates one hctx when the last CPU in hctx->cpumask become > offline by draining all requests originated from this hctx and moving new > allocation to other active hctx. This way is for avoiding inflight IO in > case of managed irq because managed irq is shutdown when the last CPU in > the irq's affinity becomes offline. > > However, lots of drivers(nvme fc, rdma, tcp, loop, ...) don't use managed > irq, so they needn't to deactivate hctx when the last CPU becomes offline. > Also, some of them are the only user of blk_mq_alloc_request_hctx() which > is used for connecting io queue. And their requirement is that the connect > request needs to be submitted successfully via one specified hctx even though > all CPUs in this hctx->cpumask have become offline. > > Addressing the requirement for nvme fc/rdma/loop by allowing to > allocate request from one hctx when all CPUs in this hctx are offline, > since these drivers don't use managed irq. > > Finally don't deactivate one hctx when it doesn't use managed irq. > > Reviewed-by: Christoph Hellwig<hch@lst.de> > Signed-off-by: Ming Lei<ming.lei@redhat.com> Sorry for lateness. Just a few minor comments below, regardless: Reviewed-by: John Garry <john.garry@huawei.com> > --- > block/blk-mq.c | 27 +++++++++++++++++---------- > block/blk-mq.h | 8 ++++++++ > 2 files changed, 25 insertions(+), 10 deletions(-) > > diff --git a/block/blk-mq.c b/block/blk-mq.c > index 2c4ac51e54eb..591ab07c64d8 100644 > --- a/block/blk-mq.c > +++ b/block/blk-mq.c > @@ -427,6 +427,15 @@ struct request *blk_mq_alloc_request(struct request_queue *q, unsigned int op, > } > EXPORT_SYMBOL(blk_mq_alloc_request); > > +static inline int blk_mq_first_mapped_cpu(struct blk_mq_hw_ctx *hctx) I don't see why it needs to be inline. I think generally we leave it to the compiler to decide. > +{ > + int cpu = cpumask_first_and(hctx->cpumask, cpu_online_mask); > + > + if (cpu >= nr_cpu_ids) > + cpu = cpumask_first(hctx->cpumask); > + return cpu; > +} > + > struct request *blk_mq_alloc_request_hctx(struct request_queue *q, > unsigned int op, blk_mq_req_flags_t flags, unsigned int hctx_idx) > { > @@ -468,7 +477,10 @@ struct request *blk_mq_alloc_request_hctx(struct request_queue *q, > data.hctx = q->queue_hw_ctx[hctx_idx]; > if (!blk_mq_hw_queue_mapped(data.hctx)) > goto out_queue_exit; > - cpu = cpumask_first_and(data.hctx->cpumask, cpu_online_mask); > + > + WARN_ON_ONCE(blk_mq_hctx_use_managed_irq(data.hctx)); > + > + cpu = blk_mq_first_mapped_cpu(data.hctx); > data.ctx = __blk_mq_get_ctx(q, cpu); > > if (!q->elevator) > @@ -1501,15 +1513,6 @@ static void __blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx) > hctx_unlock(hctx, srcu_idx); > } > > -static inline int blk_mq_first_mapped_cpu(struct blk_mq_hw_ctx *hctx) > -{ > - int cpu = cpumask_first_and(hctx->cpumask, cpu_online_mask); > - > - if (cpu >= nr_cpu_ids) > - cpu = cpumask_first(hctx->cpumask); > - return cpu; > -} > - > /* > * It'd be great if the workqueue API had a way to pass > * in a mask and had some smarts for more clever placement. > @@ -2556,6 +2559,10 @@ static int blk_mq_hctx_notify_offline(unsigned int cpu, struct hlist_node *node) > struct blk_mq_hw_ctx *hctx = hlist_entry_safe(node, > struct blk_mq_hw_ctx, cpuhp_online); > > + /* hctx needn't to be deactivated in case managed irq isn't used */ > + if (!blk_mq_hctx_use_managed_irq(hctx)) > + return 0; In a previous version (v2) I think that we just didn't register the cpu hotplug handlers, so not sure why that changed. > + > if (!cpumask_test_cpu(cpu, hctx->cpumask) || > !blk_mq_last_cpu_in_hctx(cpu, hctx)) > return 0; > diff --git a/block/blk-mq.h b/block/blk-mq.h > index d08779f77a26..7333b659d8f5 100644 > --- a/block/blk-mq.h > +++ b/block/blk-mq.h > @@ -119,6 +119,14 @@ static inline struct blk_mq_hw_ctx *blk_mq_map_queue(struct request_queue *q, > return ctx->hctxs[type]; > } > > +static inline bool blk_mq_hctx_use_managed_irq(struct blk_mq_hw_ctx *hctx) > +{ > + if (hctx->type == HCTX_TYPE_POLL) > + return false; > + > + return hctx->queue->tag_set->map[hctx->type].use_managed_irq; > +} This function only seems to be used in blk-mq.c, so not sure why you put it in .h file. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V6 3/3] blk-mq: don't deactivate hctx if managed irq isn't used 2021-08-18 9:38 ` John Garry @ 2021-08-18 10:32 ` Ming Lei 0 siblings, 0 replies; 14+ messages in thread From: Ming Lei @ 2021-08-18 10:32 UTC (permalink / raw) To: John Garry Cc: Jens Axboe, Christoph Hellwig, linux-block, Thomas Gleixner, Greg Kroah-Hartman, Sagi Grimberg, Daniel Wagner, Wen Xiong, Hannes Reinecke On Wed, Aug 18, 2021 at 10:38:15AM +0100, John Garry wrote: > On 22/07/2021 10:52, Ming Lei wrote: > > blk-mq deactivates one hctx when the last CPU in hctx->cpumask become > > offline by draining all requests originated from this hctx and moving new > > allocation to other active hctx. This way is for avoiding inflight IO in > > case of managed irq because managed irq is shutdown when the last CPU in > > the irq's affinity becomes offline. > > > > However, lots of drivers(nvme fc, rdma, tcp, loop, ...) don't use managed > > irq, so they needn't to deactivate hctx when the last CPU becomes offline. > > Also, some of them are the only user of blk_mq_alloc_request_hctx() which > > is used for connecting io queue. And their requirement is that the connect > > request needs to be submitted successfully via one specified hctx even though > > all CPUs in this hctx->cpumask have become offline. > > > > Addressing the requirement for nvme fc/rdma/loop by allowing to > > allocate request from one hctx when all CPUs in this hctx are offline, > > since these drivers don't use managed irq. > > > > Finally don't deactivate one hctx when it doesn't use managed irq. > > > > Reviewed-by: Christoph Hellwig<hch@lst.de> > > Signed-off-by: Ming Lei<ming.lei@redhat.com> > > Sorry for lateness. Just a few minor comments below, regardless: > > Reviewed-by: John Garry <john.garry@huawei.com> > > > --- > > block/blk-mq.c | 27 +++++++++++++++++---------- > > block/blk-mq.h | 8 ++++++++ > > 2 files changed, 25 insertions(+), 10 deletions(-) > > > > diff --git a/block/blk-mq.c b/block/blk-mq.c > > index 2c4ac51e54eb..591ab07c64d8 100644 > > --- a/block/blk-mq.c > > +++ b/block/blk-mq.c > > @@ -427,6 +427,15 @@ struct request *blk_mq_alloc_request(struct request_queue *q, unsigned int op, > > } > > EXPORT_SYMBOL(blk_mq_alloc_request); > > +static inline int blk_mq_first_mapped_cpu(struct blk_mq_hw_ctx *hctx) > > I don't see why it needs to be inline. I think generally we leave it to the > compiler to decide. Usually if one function is called more than two times, it will not be inlined. blk_mq_first_mapped_cpu() is called in fast path, so we need to inline it. > > > +{ > > + int cpu = cpumask_first_and(hctx->cpumask, cpu_online_mask); > > + > > + if (cpu >= nr_cpu_ids) > > + cpu = cpumask_first(hctx->cpumask); > > + return cpu; > > +} > > + > > struct request *blk_mq_alloc_request_hctx(struct request_queue *q, > > unsigned int op, blk_mq_req_flags_t flags, unsigned int hctx_idx) > > { > > @@ -468,7 +477,10 @@ struct request *blk_mq_alloc_request_hctx(struct request_queue *q, > > data.hctx = q->queue_hw_ctx[hctx_idx]; > > if (!blk_mq_hw_queue_mapped(data.hctx)) > > goto out_queue_exit; > > - cpu = cpumask_first_and(data.hctx->cpumask, cpu_online_mask); > > + > > + WARN_ON_ONCE(blk_mq_hctx_use_managed_irq(data.hctx)); > > + > > + cpu = blk_mq_first_mapped_cpu(data.hctx); > > data.ctx = __blk_mq_get_ctx(q, cpu); > > if (!q->elevator) > > @@ -1501,15 +1513,6 @@ static void __blk_mq_run_hw_queue(struct blk_mq_hw_ctx *hctx) > > hctx_unlock(hctx, srcu_idx); > > } > > -static inline int blk_mq_first_mapped_cpu(struct blk_mq_hw_ctx *hctx) > > -{ > > - int cpu = cpumask_first_and(hctx->cpumask, cpu_online_mask); > > - > > - if (cpu >= nr_cpu_ids) > > - cpu = cpumask_first(hctx->cpumask); > > - return cpu; > > -} > > - > > /* > > * It'd be great if the workqueue API had a way to pass > > * in a mask and had some smarts for more clever placement. > > @@ -2556,6 +2559,10 @@ static int blk_mq_hctx_notify_offline(unsigned int cpu, struct hlist_node *node) > > struct blk_mq_hw_ctx *hctx = hlist_entry_safe(node, > > struct blk_mq_hw_ctx, cpuhp_online); > > + /* hctx needn't to be deactivated in case managed irq isn't used */ > > + if (!blk_mq_hctx_use_managed_irq(hctx)) > > + return 0; > > In a previous version (v2) I think that we just didn't register the cpu > hotplug handlers, so not sure why that changed. Sorry, forget to mention the change, inside blk_mq_realloc_hw_ctxs() we may grab one cached hctx without initializing it, so use managed irq info change may not be visible in blk_mq_init_hctx(). Given blk_mq_hctx_notify_offline() isn't in fast path, I'd rather to check in blk_mq_hctx_notify_offline(), which is more reliable than running the check in blk_mq_init_hctx(). > > > + > > if (!cpumask_test_cpu(cpu, hctx->cpumask) || > > !blk_mq_last_cpu_in_hctx(cpu, hctx)) > > return 0; > > diff --git a/block/blk-mq.h b/block/blk-mq.h > > index d08779f77a26..7333b659d8f5 100644 > > --- a/block/blk-mq.h > > +++ b/block/blk-mq.h > > @@ -119,6 +119,14 @@ static inline struct blk_mq_hw_ctx *blk_mq_map_queue(struct request_queue *q, > > return ctx->hctxs[type]; > > } > > +static inline bool blk_mq_hctx_use_managed_irq(struct blk_mq_hw_ctx *hctx) > > +{ > > + if (hctx->type == HCTX_TYPE_POLL) > > + return false; > > + > > + return hctx->queue->tag_set->map[hctx->type].use_managed_irq; > > +} > > This function only seems to be used in blk-mq.c, so not sure why you put it > in .h file. oops, the helper must be used in early version, I will send a new version soon. Thanks, Ming ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V6 0/3] blk-mq: fix blk_mq_alloc_request_hctx 2021-07-22 9:52 [PATCH V6 0/3] blk-mq: fix blk_mq_alloc_request_hctx Ming Lei ` (2 preceding siblings ...) 2021-07-22 9:52 ` [PATCH V6 3/3] blk-mq: don't deactivate hctx if managed irq isn't used Ming Lei @ 2021-07-22 13:12 ` Daniel Wagner [not found] ` <OFDADF39F5.DDB99A55-ON0025871A.00794382-0025871A.00797A2E@ibm.com> 4 siblings, 0 replies; 14+ messages in thread From: Daniel Wagner @ 2021-07-22 13:12 UTC (permalink / raw) To: Ming Lei Cc: Jens Axboe, Christoph Hellwig, linux-block, Thomas Gleixner, Greg Kroah-Hartman, John Garry, Sagi Grimberg, Wen Xiong, Hannes Reinecke On Thu, Jul 22, 2021 at 05:52:43PM +0800, Ming Lei wrote: > Wen Xiong has verified V4 in her nvmef test. FWIW, I am testing this series right now. I observe hanging I/Os again, but I am not totally sure if my test setup is working properly. Working on it. I'll keep you posted. ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <OFDADF39F5.DDB99A55-ON0025871A.00794382-0025871A.00797A2E@ibm.com>]
* Re: [PATCH V6 0/3] blk-mq: fix blk_mq_alloc_request_hctx [not found] ` <OFDADF39F5.DDB99A55-ON0025871A.00794382-0025871A.00797A2E@ibm.com> @ 2021-07-23 8:16 ` Ming Lei [not found] ` <OF2C4681CD.AF20CBB4-ON0025871E.004D37B6-0025871E.004E3B9F@ibm.com> 1 sibling, 0 replies; 14+ messages in thread From: Ming Lei @ 2021-07-23 8:16 UTC (permalink / raw) To: Wen Xiong Cc: dwagner, axboe, gregkh, hare, hch, john.garry, linux-block, sagi, tglx On Thu, Jul 22, 2021 at 10:06:51PM +0000, Wen Xiong wrote: > > Wen Xiong has verified V4 in her nvmef test. > > >>FWIW, I am testing this series right now. I observe hanging I/Os > >>again, but I am not totally sure if my test setup is working > >>properly. Working on it. I'll keep you posted. > > > I built the latest upstream(v5.14-rc2) + Ming's V6 + Daniel's V3: didn't work. > Hi Wenxiong, V6 is basically same with V4, can you figure out where the failure comes?(v5.14-rc2, V6 or Daniel's V3) I verified that V6 works as expected by tracing blk_mq_update_queue_map(): #!/usr/bin/bpftrace #include <linux/blk-mq.h> kprobe:blk_mq_update_queue_map { @in_map[tid]=1; @set[tid] = (struct blk_mq_tag_set *)arg0; @ms[tid] = kstack; } kretprobe:blk_mq_update_queue_map /@in_map[tid]==1/ { $s = @set[tid]; $m = (struct blk_mq_queue_map *)$s->map; printf("%s %d: %s queues %d use_managed_irq %d\n", comm, pid, @ms[tid], $s->nr_hw_queues, $m->use_managed_irq); } Thanks, Ming ^ permalink raw reply [flat|nested] 14+ messages in thread
[parent not found: <OF2C4681CD.AF20CBB4-ON0025871E.004D37B6-0025871E.004E3B9F@ibm.com>]
* Re: [PATCH V6 0/3] blk-mq: fix blk_mq_alloc_request_hctx [not found] ` <OF2C4681CD.AF20CBB4-ON0025871E.004D37B6-0025871E.004E3B9F@ibm.com> @ 2021-07-26 17:06 ` Daniel Wagner 2021-08-18 9:11 ` Daniel Wagner 0 siblings, 1 reply; 14+ messages in thread From: Daniel Wagner @ 2021-07-26 17:06 UTC (permalink / raw) To: Wen Xiong Cc: ming.lei, axboe, gregkh, hare, hch, john.garry, linux-block, sagi, tglx Hi, On Mon, Jul 26, 2021 at 02:14:30PM +0000, Wen Xiong wrote: > >>V6 is basically same with V4, can you figure out where the failure > >>comes?(v5.14-rc2, V6 or Daniel's V3) > > Looks 3/3 was not patched cleanly in v5.14-rc2 last week. I made the changes > in block/blk-mq.c.rej manually but still missed the last part of 3/3 patch. Sorry for the long delay on my side. It took a while to get my test setup running again. The qla2xxx driver really doesn't like 'fast' remote port toggling. But that's a different story. Anyway, it turns out that my patch series is still not working correctly. When I tested the series I deliberate forced to execute the 'revising io queue count' path in nvme_fc_recreate_io_queues by doing: --- a/drivers/nvme/host/fc.c +++ b/drivers/nvme/host/fc.c @@ -2954,7 +2954,7 @@ nvme_fc_recreate_io_queues(struct nvme_fc_ctrl *ctrl) if (ctrl->ctrl.queue_count == 1) return 0; - if (prior_ioq_cnt != nr_io_queues) { + if (prior_ioq_cnt != nr_io_queues - 1) { dev_info(ctrl->ctrl.device, "reconnect: revising io queue count from %d to %d\n", prior_ioq_cnt, nr_io_queues); With this change I can't observe any I/O hanging. Without the change it hangs in the first iteration. In Wen's setup we observed in earlier debugging sessions that the nr_io_queues does change. So this explains why Wen doesn't see any hanging I/Os. @James, I think we need to look at bit more at the freeze code. BTW, my initial patch which just added a nvme_start_freeze() in nvme_fc_delete_association() doesn't work either for the 'prior_ioq_cnt == nr_io_queues' case. So I think Ming series can be merged as the hanging I/Os are clearly not caused by the series. Feel free to add Tested-by: Daniel Wagner <dwagner@suse.de> Thanks, Daniel ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH V6 0/3] blk-mq: fix blk_mq_alloc_request_hctx 2021-07-26 17:06 ` Daniel Wagner @ 2021-08-18 9:11 ` Daniel Wagner 0 siblings, 0 replies; 14+ messages in thread From: Daniel Wagner @ 2021-08-18 9:11 UTC (permalink / raw) To: axboe Cc: ming.lei, axboe, gregkh, hare, hch, john.garry, linux-block, sagi, tglx, Wen Xiong Any chance to queue this series? ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2021-08-18 10:33 UTC | newest] Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-07-22 9:52 [PATCH V6 0/3] blk-mq: fix blk_mq_alloc_request_hctx Ming Lei 2021-07-22 9:52 ` [PATCH V6 1/3] genirq: add device_has_managed_msi_irq Ming Lei 2021-07-22 13:05 ` Christoph Hellwig 2021-07-22 9:52 ` [PATCH V6 2/3] blk-mq: mark if one queue map uses managed irq Ming Lei 2021-07-22 13:06 ` Christoph Hellwig 2021-07-22 15:40 ` Ming Lei 2021-07-22 16:06 ` Christoph Hellwig 2021-07-22 9:52 ` [PATCH V6 3/3] blk-mq: don't deactivate hctx if managed irq isn't used Ming Lei 2021-08-18 9:38 ` John Garry 2021-08-18 10:32 ` Ming Lei 2021-07-22 13:12 ` [PATCH V6 0/3] blk-mq: fix blk_mq_alloc_request_hctx Daniel Wagner [not found] ` <OFDADF39F5.DDB99A55-ON0025871A.00794382-0025871A.00797A2E@ibm.com> 2021-07-23 8:16 ` Ming Lei [not found] ` <OF2C4681CD.AF20CBB4-ON0025871E.004D37B6-0025871E.004E3B9F@ibm.com> 2021-07-26 17:06 ` Daniel Wagner 2021-08-18 9:11 ` Daniel Wagner
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.