| Message ID | 20210702150555.2401722-4-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | blk-mq: fix blk_mq_alloc_request_hctx | expand |
On 02/07/2021 16:05, Ming Lei wrote: > blk-mq needs this information of using managed irq for improving > deactivating hctx, so add such flag to 'struct Scsi_Host', then > drivers can pass such flag to blk-mq via scsi_mq_setup_tags(). > > The rule is that driver has to tell blk-mq if managed irq is used. > > Signed-off-by: Ming Lei<ming.lei@redhat.com> As was said before, can we have something like this instead of relying on the LLDs to do the setting: --------->8------------ diff --git a/block/blk-mq-pci.c b/block/blk-mq-pci.c index b595a94c4d16..2037a5b69fe1 100644 --- a/block/blk-mq-pci.c +++ b/block/blk-mq-pci.c @@ -37,7 +37,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->drain_hwq = 1; return 0; fallback: diff --git a/block/blk-mq.c b/block/blk-mq.c index 46fe5b45a8b8..7b610e680e08 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -3408,7 +3408,8 @@ static int blk_mq_update_queue_map(struct blk_mq_tag_set *set) set->map[HCTX_TYPE_DEFAULT].nr_queues = set->nr_hw_queues; if (set->ops->map_queues && !is_kdump_kernel()) { - int i; + struct blk_mq_queue_map *qmap = &set->map[HCTX_TYPE_DEFAULT]; + int i, ret; /* * transport .map_queues is usually done in the following @@ -3427,7 +3428,12 @@ static int blk_mq_update_queue_map(struct blk_mq_tag_set *set) for (i = 0; i < set->nr_maps; i++) blk_mq_clear_mq_map(&set->map[i]); - return set->ops->map_queues(set); + ret = set->ops->map_queues(set); + if (ret) + return ret; + if (qmap->drain_hwq) + set->flags |= BLK_MQ_F_MANAGED_IRQ; + return 0; } else { BUG_ON(set->nr_maps > 1); return blk_mq_map_queues(&set->map[HCTX_TYPE_DEFAULT]); diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h index 65ed99b3f2f0..2b2e71ff43e0 100644 --- a/include/linux/blk-mq.h +++ b/include/linux/blk-mq.h @@ -193,6 +193,7 @@ struct blk_mq_queue_map { unsigned int *mq_map; unsigned int nr_queues; unsigned int queue_offset; + bool drain_hwq; };
On Mon, Jul 05, 2021 at 10:25:38AM +0100, John Garry wrote: > On 02/07/2021 16:05, Ming Lei wrote: > > blk-mq needs this information of using managed irq for improving > > deactivating hctx, so add such flag to 'struct Scsi_Host', then > > drivers can pass such flag to blk-mq via scsi_mq_setup_tags(). > > > > The rule is that driver has to tell blk-mq if managed irq is used. > > > > Signed-off-by: Ming Lei<ming.lei@redhat.com> > > As was said before, can we have something like this instead of relying on > the LLDs to do the setting: > > --------->8------------ > > diff --git a/block/blk-mq-pci.c b/block/blk-mq-pci.c > index b595a94c4d16..2037a5b69fe1 100644 > --- a/block/blk-mq-pci.c > +++ b/block/blk-mq-pci.c > @@ -37,7 +37,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->drain_hwq = 1; The thing is that blk_mq_pci_map_queues() is allowed to be called for non-managed irqs. Also some managed irq consumers don't use blk_mq_pci_map_queues(). So this way just provides hint about managed irq uses, but we really need to get this flag set if driver uses managed irq. Thanks, Ming
On Mon, Jul 05, 2021 at 05:55:49PM +0800, Ming Lei wrote: > The thing is that blk_mq_pci_map_queues() is allowed to be called for > non-managed irqs. Also some managed irq consumers don't use blk_mq_pci_map_queues(). > > So this way just provides hint about managed irq uses, but we really > need to get this flag set if driver uses managed irq. blk_mq_pci_map_queues is absolutely intended to only be used by managed irqs. I wonder if we can enforce that somehow?
On Tue, Jul 06, 2021 at 07:37:19AM +0200, Christoph Hellwig wrote: > On Mon, Jul 05, 2021 at 05:55:49PM +0800, Ming Lei wrote: > > The thing is that blk_mq_pci_map_queues() is allowed to be called for > > non-managed irqs. Also some managed irq consumers don't use blk_mq_pci_map_queues(). > > > > So this way just provides hint about managed irq uses, but we really > > need to get this flag set if driver uses managed irq. > > blk_mq_pci_map_queues is absolutely intended to only be used by > managed irqs. I wonder if we can enforce that somehow? It may break some scsi drivers. And blk_mq_pci_map_queues() just calls pci_irq_get_affinity() to retrieve the irq's affinity, and the irq can be one non-managed irq, which affinity is set via either irq_set_affinity_hint() from kernel or /proc/irq/. Thanks, Ming
On 7/6/21 9:41 AM, Ming Lei wrote: > On Tue, Jul 06, 2021 at 07:37:19AM +0200, Christoph Hellwig wrote: >> On Mon, Jul 05, 2021 at 05:55:49PM +0800, Ming Lei wrote: >>> The thing is that blk_mq_pci_map_queues() is allowed to be called for >>> non-managed irqs. Also some managed irq consumers don't use blk_mq_pci_map_queues(). >>> >>> So this way just provides hint about managed irq uses, but we really >>> need to get this flag set if driver uses managed irq. >> >> blk_mq_pci_map_queues is absolutely intended to only be used by >> managed irqs. I wonder if we can enforce that somehow? > > It may break some scsi drivers. > > And blk_mq_pci_map_queues() just calls pci_irq_get_affinity() to > retrieve the irq's affinity, and the irq can be one non-managed irq, > which affinity is set via either irq_set_affinity_hint() from kernel > or /proc/irq/. > But that's static, right? IE blk_mq_pci_map_queues() will be called once during module init; so what happens if the user changes the mapping later on? How will that be transferred to the driver? Cheers, Hannes
On Tue, Jul 06, 2021 at 12:32:27PM +0200, Hannes Reinecke wrote: > On 7/6/21 9:41 AM, Ming Lei wrote: > > On Tue, Jul 06, 2021 at 07:37:19AM +0200, Christoph Hellwig wrote: > > > On Mon, Jul 05, 2021 at 05:55:49PM +0800, Ming Lei wrote: > > > > The thing is that blk_mq_pci_map_queues() is allowed to be called for > > > > non-managed irqs. Also some managed irq consumers don't use blk_mq_pci_map_queues(). > > > > > > > > So this way just provides hint about managed irq uses, but we really > > > > need to get this flag set if driver uses managed irq. > > > > > > blk_mq_pci_map_queues is absolutely intended to only be used by > > > managed irqs. I wonder if we can enforce that somehow? > > > > It may break some scsi drivers. > > > > And blk_mq_pci_map_queues() just calls pci_irq_get_affinity() to > > retrieve the irq's affinity, and the irq can be one non-managed irq, > > which affinity is set via either irq_set_affinity_hint() from kernel > > or /proc/irq/. > > > But that's static, right? IE blk_mq_pci_map_queues() will be called once > during module init; so what happens if the user changes the mapping later > on? How will that be transferred to the driver? Yeah, that may not work well enough, but still works since non-managed irq supports migration. And there are several SCSI drivers which provide module parameter to enable/disable managed irq, meantime blk_mq_pci_map_queues() is always called for mapping queues. Thanks, Ming
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c index 532304d42f00..743df8e824b9 100644 --- a/drivers/scsi/scsi_lib.c +++ b/drivers/scsi/scsi_lib.c @@ -1915,6 +1915,8 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost) { unsigned int cmd_size, sgl_size; struct blk_mq_tag_set *tag_set = &shost->tag_set; + unsigned long flags = BLK_MQ_F_SHOULD_MERGE | + BLK_ALLOC_POLICY_TO_MQ_FLAG(shost->hostt->tag_alloc_policy); sgl_size = max_t(unsigned int, sizeof(struct scatterlist), scsi_mq_inline_sgl_size(shost)); @@ -1933,12 +1935,12 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost) tag_set->queue_depth = shost->can_queue; tag_set->cmd_size = cmd_size; tag_set->numa_node = NUMA_NO_NODE; - tag_set->flags = BLK_MQ_F_SHOULD_MERGE; - tag_set->flags |= - BLK_ALLOC_POLICY_TO_MQ_FLAG(shost->hostt->tag_alloc_policy); - tag_set->driver_data = shost; if (shost->host_tagset) - tag_set->flags |= BLK_MQ_F_TAG_HCTX_SHARED; + flags |= BLK_MQ_F_TAG_HCTX_SHARED; + if (shost->use_managed_irq) + flags |= BLK_MQ_F_MANAGED_IRQ; + tag_set->flags = flags; + tag_set->driver_data = shost; return blk_mq_alloc_tag_set(tag_set); } diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h index d0bf88d77f02..3ac589ae9592 100644 --- a/include/scsi/scsi_host.h +++ b/include/scsi/scsi_host.h @@ -657,6 +657,9 @@ struct Scsi_Host { /* True if the host uses host-wide tagspace */ unsigned host_tagset:1; + /* True if the host uses managed irq */ + unsigned use_managed_irq:1; + /* Host responded with short (<36 bytes) INQUIRY result */ unsigned short_inquiry:1;
blk-mq needs this information of using managed irq for improving deactivating hctx, so add such flag to 'struct Scsi_Host', then drivers can pass such flag to blk-mq via scsi_mq_setup_tags(). The rule is that driver has to tell blk-mq if managed irq is used. Signed-off-by: Ming Lei <ming.lei@redhat.com> --- drivers/scsi/scsi_lib.c | 12 +++++++----- include/scsi/scsi_host.h | 3 +++ 2 files changed, 10 insertions(+), 5 deletions(-)