| Message ID | 20200220024441.11558-3-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Eight patches related to changing the number of hardware queues | expand |
On Wed, Feb 19, 2020 at 06:44:35PM -0800, Bart Van Assche wrote: > blk_mq_map_queues() and multiple .map_queues() implementations expect that > set->map[HCTX_TYPE_DEFAULT].nr_queues is set to the number of hardware Only single queue mapping expects set->map[HCTX_TYPE_DEFAULT].nr_queues to be set->nr_hw_queues. For multiple mapping, set->nr_hw_queues should be sum of each mapping's nr_queue. Thanks, Ming
On 2/20/20 2:05 AM, Ming Lei wrote: > On Wed, Feb 19, 2020 at 06:44:35PM -0800, Bart Van Assche wrote: >> blk_mq_map_queues() and multiple .map_queues() implementations expect that >> set->map[HCTX_TYPE_DEFAULT].nr_queues is set to the number of hardware > > Only single queue mapping expects set->map[HCTX_TYPE_DEFAULT].nr_queues > to be set->nr_hw_queues. For multiple mapping, set->nr_hw_queues should > be sum of each mapping's nr_queue. That's how it should work but that doesn't match what I found in the kernel tree. Here is an example of a .map_queues() implementation that depends on .nr_queues being set before it is called: static int scsi_map_queues(struct blk_mq_tag_set *set) { struct Scsi_Host *shost = container_of(set, struct Scsi_Host, tag_set); if (shost->hostt->map_queues) return shost->hostt->map_queues(shost); return blk_mq_map_queues(&set->map[HCTX_TYPE_DEFAULT]); } If shost->hostt->map_queues == NULL, the above function only works correctly if .nr_queues is set before this function is called. Additionally, scsi_map_queues() may call e.g. the following function: static int qla2xxx_map_queues(struct Scsi_Host *shost) { int rc; scsi_qla_host_t *vha = (scsi_qla_host_t *)shost->hostdata; struct blk_mq_queue_map *qmap = &shost->tag_set.map[HCTX_TYPE_DEFAULT]; if (USER_CTRL_IRQ(vha->hw) || !vha->hw->mqiobase) rc = blk_mq_map_queues(qmap); else rc = blk_mq_pci_map_queues(qmap, vha->hw->pdev, vha->irq_offset); return rc; } Both blk_mq_map_queues() and blk_mq_pci_map_queues() assume that blk_mq_queue_map.nr_queues is set before these functions are called. Bart.
On Thu, Feb 20, 2020 at 08:14:11AM -0800, Bart Van Assche wrote: > On 2/20/20 2:05 AM, Ming Lei wrote: > > On Wed, Feb 19, 2020 at 06:44:35PM -0800, Bart Van Assche wrote: > > > blk_mq_map_queues() and multiple .map_queues() implementations expect that > > > set->map[HCTX_TYPE_DEFAULT].nr_queues is set to the number of hardware > > > > Only single queue mapping expects set->map[HCTX_TYPE_DEFAULT].nr_queues > > to be set->nr_hw_queues. For multiple mapping, set->nr_hw_queues should > > be sum of each mapping's nr_queue. > > That's how it should work but that doesn't match what I found in the kernel > tree. Here is an example of a .map_queues() implementation that depends on > .nr_queues being set before it is called: > > static int scsi_map_queues(struct blk_mq_tag_set *set) > { > struct Scsi_Host *shost = container_of(set, struct Scsi_Host, > tag_set); > > if (shost->hostt->map_queues) > return shost->hostt->map_queues(shost); > return blk_mq_map_queues(&set->map[HCTX_TYPE_DEFAULT]); > } > > If shost->hostt->map_queues == NULL, the above function only works correctly > if .nr_queues is set before this function is called. Additionally, > scsi_map_queues() may call e.g. the following function: > > static int qla2xxx_map_queues(struct Scsi_Host *shost) > { > int rc; > scsi_qla_host_t *vha = (scsi_qla_host_t *)shost->hostdata; > struct blk_mq_queue_map *qmap = > &shost->tag_set.map[HCTX_TYPE_DEFAULT]; > > if (USER_CTRL_IRQ(vha->hw) || !vha->hw->mqiobase) > rc = blk_mq_map_queues(qmap); > else > rc = blk_mq_pci_map_queues(qmap, vha->hw->pdev, > vha->irq_offset); > return rc; > } > > Both blk_mq_map_queues() and blk_mq_pci_map_queues() assume that > blk_mq_queue_map.nr_queues is set before these functions are called. Actually, I suggested to do the following way: if (set->nr_maps == 1) set->map[HCTX_TYPE_DEFAULT].nr_queues = set->nr_hw_queues; then people won't be confused wrt. relation between set->nr_hw_queues and .nr_queues of each mapping. Thanks, Ming
On 2020-02-20 12:47, Ming Lei wrote: > Actually, I suggested to do the following way: > > if (set->nr_maps == 1) > set->map[HCTX_TYPE_DEFAULT].nr_queues = set->nr_hw_queues; > > then people won't be confused wrt. relation between set->nr_hw_queues > and .nr_queues of each mapping. Ah, thanks for the clarification. I will make that change. Bart.
diff --git a/block/blk-mq.c b/block/blk-mq.c index f298500e6dda..7f996e427733 100644 --- a/block/blk-mq.c +++ b/block/blk-mq.c @@ -3023,6 +3023,13 @@ static int blk_mq_alloc_rq_maps(struct blk_mq_tag_set *set) static int blk_mq_update_queue_map(struct blk_mq_tag_set *set) { + /* + * blk_mq_map_queues() and multiple .map_queues() implementations + * expect that set->map[HCTX_TYPE_DEFAULT].nr_queues is set to the + * number of hardware queues. + */ + set->map[HCTX_TYPE_DEFAULT].nr_queues = set->nr_hw_queues; + if (set->ops->map_queues && !is_kdump_kernel()) { int i;
blk_mq_map_queues() and multiple .map_queues() implementations expect that set->map[HCTX_TYPE_DEFAULT].nr_queues is set to the number of hardware queues. Hence set .nr_queues before calling these functions. This patch fixes the following kernel warning: WARNING: CPU: 0 PID: 2501 at include/linux/cpumask.h:137 Call Trace: blk_mq_run_hw_queue+0x19d/0x350 block/blk-mq.c:1508 blk_mq_run_hw_queues+0x112/0x1a0 block/blk-mq.c:1525 blk_mq_requeue_work+0x502/0x780 block/blk-mq.c:775 process_one_work+0x9af/0x1740 kernel/workqueue.c:2269 worker_thread+0x98/0xe40 kernel/workqueue.c:2415 kthread+0x361/0x430 kernel/kthread.c:255 Cc: Christoph Hellwig <hch@infradead.org> Cc: Ming Lei <ming.lei@redhat.com> Cc: Hannes Reinecke <hare@suse.com> Cc: Johannes Thumshirn <jth@kernel.org> Reported-by: syzbot+d44e1b26ce5c3e77458d@syzkaller.appspotmail.com Fixes: ed76e329d74a ("blk-mq: abstract out queue map") # v5.0 Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- block/blk-mq.c | 7 +++++++ 1 file changed, 7 insertions(+)