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

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

[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.

Signed-off-by: Christoph Hellwig <hch at lst.de>
---
 drivers/nvme/host/pci.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index fb9d8270f32c..8b62d0075e48 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -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);
-- 
2.19.2

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

[Jens Axboe]

On 12/14/18 8:42 AM, Christoph Hellwig wrote:
> 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, but needs a Fixes line.

-- 
Jens Axboe

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

[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.

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(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index fb9d8270f32c..8b62d0075e48 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -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);
-- 
2.19.2

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

[Jens Axboe]

On 12/14/18 9:20 AM, Christoph Hellwig wrote:
> 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.

-- 
Jens Axboe

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

[Christoph Hellwig]

On Fri, Dec 14, 2018@09:47:38AM -0700, Jens Axboe wrote:
> On 12/14/18 9:20 AM, Christoph Hellwig wrote:
> > 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.

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

[Jens Axboe]

On 12/14/18 9:49 AM, Christoph Hellwig wrote:
> On Fri, Dec 14, 2018@09:47:38AM -0700, Jens Axboe wrote:
>> On 12/14/18 9:20 AM, Christoph Hellwig wrote:
>>> 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.

Sounds good, thanks.

-- 
Jens Axboe

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

[Sagi Grimberg]

> 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?

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

[Jens Axboe]

On 12/14/18 11:43 AM, Sagi Grimberg wrote:
> 
>> 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.

-- 
Jens Axboe

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

[Sagi Grimberg]

>>> 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.

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.

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

[Jens Axboe]

On 12/14/18 12:01 PM, Sagi Grimberg wrote:
> 
>>>> 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.
> 
> 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.

-- 
Jens Axboe

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

[Sagi Grimberg]

>>> 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.

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

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

[Sagi Grimberg]

Reviewed-by: Sagi Grimberg <sagi at grimberg.me>