Skip to contentSkip to conversation

nvme-pci: only set nr_maps to 2 if poll queues are supported

PATCH New Christoph Hellwig 3 participants 10 messages Dec 14, 2018 – Dec 15, 2018 NVMe

lore
Share & tools
lore

Christoph Hellwig

author maintainer
#0 lore raw

PATCH 1/1 nvme-pci: only set nr_maps to 2 if poll queues are supported

The block layer now enables polling support on a queue if nr_maps includes the poll map, so we should only set that if we actually support poll queues.

Fixes 6544d229bf ("block: enable polling by default if a poll map is initalized")
Signed-off-by Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/pci.c | 3 +++
 1 file changed, 3 insertions(+)
drivers/nvme/host/pci.c +3 -0
index fb9d8270f32c..8b62d0075e48 100644
@@ -2293,6 +2293,9 @@ static int nvme_dev_add(struct nvme_dev *dev)
 	if (!dev->ctrl.tagset) {
 		dev->tagset.ops = &nvme_mq_ops;
 		dev->tagset.nr_hw_queues = dev->online_queues - 1;
+		dev->tagset.nr_maps = 2; /* default + read */
+		if (dev->io_queues[HCTX_TYPE_POLL])
+			dev->tagset.nr_maps++;
 		dev->tagset.nr_maps = HCTX_MAX_TYPES;
 		dev->tagset.timeout = NVME_IO_TIMEOUT;
 		dev->tagset.numa_node = dev_to_node(dev->dev);
Signature · 1 line
2.19.2
maintainer
#1 lore raw

PATCH 1/1 nvme-pci: only set nr_maps to 2 if poll queues are supported

Quoted 3 lines · Christoph Hellwig

The block layer now enables polling support on a queue if nr_maps includes the poll map, so we should only set that if we actually support poll queues.

Looks good to me. Do you want to carry it? If so:

Reviewed-by Jens Axboe <axboe at kernel.dk>

or I can take it directly. Let me know.

Signature · 1 line
Jens Axboe
author maintainer
#2 lore raw

PATCH 1/1 nvme-pci: only set nr_maps to 2 if poll queues are supported

Quoted 10 lines · Jens Axboe
Quoted 3 lines · Christoph Hellwig

The block layer now enables polling support on a queue if nr_maps includes the poll map, so we should only set that if we actually support poll queues.

Looks good to me. Do you want to carry it? If so:

Reviewed-by Jens Axboe <axboe at kernel.dk>

or I can take it directly. Let me know.

I can carry it, we've get a few more nvme bits piling up anyway.

maintainer
#3 lore raw

PATCH 1/1 nvme-pci: only set nr_maps to 2 if poll queues are supported

Quoted 6 lines · Christoph Hellwig
Quoted 5 lines · Jens Axboe
Quoted 3 lines · Christoph Hellwig

The block layer now enables polling support on a queue if nr_maps includes the poll map, so we should only set that if we actually support poll queues.

Quoted 7 lines
Quoted 5 lines

Looks good to me. Do you want to carry it? If so:

Reviewed-by Jens Axboe <axboe at kernel.dk>

or I can take it directly. Let me know.

I can carry it, we've get a few more nvme bits piling up anyway.

Sounds good, thanks.

Signature · 1 line
Jens Axboe
maintainer
#4 lore raw

PATCH 1/1 nvme-pci: only set nr_maps to 2 if poll queues are supported

In reply to Christoph Hellwig
Quoted 6 lines

The block layer now enables polling support on a queue if nr_maps includes the poll map, so we should only set that if we actually support poll queues.

Fixes 6544d229bf ("block: enable polling by default if a poll map is initalized")

What does it fix? is there a bug when setting 3 queue maps?

maintainer
#5 lore raw

PATCH 1/1 nvme-pci: only set nr_maps to 2 if poll queues are supported

Quoted 8 lines · Sagi Grimberg
Quoted 6 lines

The block layer now enables polling support on a queue if nr_maps includes the poll map, so we should only set that if we actually support poll queues.

Fixes 6544d229bf ("block: enable polling by default if a poll map is initalized")

What does it fix? is there a bug when setting 3 queue maps?

After the recent changes, blk_mq_init_allocated_queue() does this:

if (set->nr_maps > HCTX_TYPE_POLL)

	blk_queue_flag_set(QUEUE_FLAG_POLL, q);

since the core doesn't have any notion of how many queues are used for what. We only support polling if you have explicit poll queues now, hence the above is wrong if nvme says nr_maps is 3 and it doesn't have any poll queues.

Signature · 1 line
Jens Axboe
maintainer
#6 lore raw

PATCH 1/1 nvme-pci: only set nr_maps to 2 if poll queues are supported

Quoted 11 lines
Quoted 8 lines
Quoted 6 lines

The block layer now enables polling support on a queue if nr_maps includes the poll map, so we should only set that if we actually support poll queues.

Fixes 6544d229bf ("block: enable polling by default if a poll map is initalized")

What does it fix? is there a bug when setting 3 queue maps?

After the recent changes, blk_mq_init_allocated_queue() does this:

Quoted 7 lines

if (set->nr_maps > HCTX_TYPE_POLL)

	blk_queue_flag_set(QUEUE_FLAG_POLL, q);

since the core doesn't have any notion of how many queues are used for what. We only support polling if you have explicit poll queues now, hence the above is wrong if nvme says nr_maps is 3 and it doesn't have any poll queues.

Yea, but its not broken because the map shares the default queue map right? Just wanted to understand if something is broken rather than just wrong.

maintainer
#7 lore raw

PATCH 1/1 nvme-pci: only set nr_maps to 2 if poll queues are supported

Quoted 14 lines · Sagi Grimberg
Quoted 14 lines
Quoted 8 lines
Quoted 6 lines

The block layer now enables polling support on a queue if nr_maps includes the poll map, so we should only set that if we actually support poll queues.

Fixes 6544d229bf ("block: enable polling by default if a poll map is initalized")

What does it fix? is there a bug when setting 3 queue maps?

After the recent changes, blk_mq_init_allocated_queue() does this:

if (set->nr_maps > HCTX_TYPE_POLL)

	blk_queue_flag_set(QUEUE_FLAG_POLL, q);
Quoted 8 lines
Quoted 4 lines

since the core doesn't have any notion of how many queues are used for what. We only support polling if you have explicit poll queues now, hence the above is wrong if nvme says nr_maps is 3 and it doesn't have any poll queues.

Yea, but its not broken because the map shares the default queue map right? Just wanted to understand if something is broken rather than just wrong.

It's broken since we say we support polling by setting the QUEUE_FLAG_POLL flag, but we do not. It's not broken in terms of maps on the nvme side. As you mention, the map sharing means it's perfectly legit to not have special queues for each map. Polling is just special since we absolutely do require special poll queues without interrupts. It's not like reads vs writes, where we can share and nobody cares.

Signature · 1 line
Jens Axboe
maintainer
#8 lore raw

PATCH 1/1 nvme-pci: only set nr_maps to 2 if poll queues are supported

Quoted 10 lines
Quoted 9 lines
Quoted 5 lines

since the core doesn't have any notion of how many queues are used for what. We only support polling if you have explicit poll queues now, hence the above is wrong if nvme says nr_maps is 3 and it doesn't have any poll queues.

Yea, but its not broken because the map shares the default queue map right? Just wanted to understand if something is broken rather than just wrong.

Quoted 6 lines

It's broken since we say we support polling by setting the QUEUE_FLAG_POLL flag, but we do not. It's not broken in terms of maps on the nvme side. As you mention, the map sharing means it's perfectly legit to not have special queues for each map. Polling is just special since we absolutely do require special poll queues without interrupts. It's not like reads vs writes, where we can share and nobody cares.

Yea... I fixed that for rdma as well..