Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1234673 > unrolled thread

[PATCH 0/3] blk-mq & nvme: introduce .map_changed

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/3] blk-mq & nvme: introduce .map_changed Ming Lei <tom.leiming@gmail.com> - 2015-09-29 05:30 +0200
    [PATCH 2/3] block: nvme: use map_changed to set irq affinity hint Ming Lei <tom.leiming@gmail.com> - 2015-09-29 05:30 +0200
    Re: [PATCH 0/3] blk-mq & nvme: introduce .map_changed Christoph Hellwig <hch@lst.de> - 2015-09-29 09:10 +0200
      Re: [PATCH 0/3] blk-mq & nvme: introduce .map_changed Ming Lei <tom.leiming@gmail.com> - 2015-09-30 00:00 +0200
    Re: [PATCH 0/3] blk-mq & nvme: introduce .map_changed Keith Busch <keith.busch@intel.com> - 2015-09-29 16:30 +0200
      Re: [PATCH 0/3] blk-mq & nvme: introduce .map_changed Jens Axboe <axboe@kernel.dk> - 2015-09-29 16:50 +0200
        Re: [PATCH 0/3] blk-mq & nvme: introduce .map_changed Ming Lei <tom.leiming@gmail.com> - 2015-09-30 00:20 +0200
          Re: [PATCH 0/3] blk-mq & nvme: introduce .map_changed Keith Busch <keith.busch@intel.com> - 2015-09-30 00:50 +0200
            Re: [PATCH 0/3] blk-mq & nvme: introduce .map_changed Ming Lei <tom.leiming@gmail.com> - 2015-09-30 02:10 +0200

#1234673 — [PATCH 0/3] blk-mq & nvme: introduce .map_changed

This patchset introduces .map_changed callback into 'struct blk_mq_ops',
and use this callback to get NVMe notified about the mapping changed event,
then NVMe can update the irq affinity hint for its queues.

Also the 'cpumask' in 'struct blk_mq_tags' isn't needed any more, so remove
that and related kernel interface.

Thanks,

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1234674 — [PATCH 2/3] block: nvme: use map_changed to set irq affinity hint

This patch uses the .map_changed callback to set irq affinity
hint, then the irq affinity can be updated when CPU topo
is changed.

Signed-off-by: Ming Lei <tom.leiming@gmail.com>
---
 drivers/block/nvme-core.c | 53 ++++++++++++++++++++++++++++++-----------------
 1 file changed, 34 insertions(+), 19 deletions(-)

diff --git a/drivers/block/nvme-core.c b/drivers/block/nvme-core.c
index b97fc3f..cac16a6f 100644
--- a/drivers/block/nvme-core.c
+++ b/drivers/block/nvme-core.c
@@ -105,6 +105,8 @@ struct nvme_queue {
 	struct device *q_dmadev;
 	struct nvme_dev *dev;
 	char irqname[24];	/* nvme4294967295-65535\0 */
+	unsigned long mapped:1;
+	unsigned long irq_affinity_set:1;
 	spinlock_t q_lock;
 	struct nvme_command *sq_cmds;
 	struct nvme_command __iomem *sq_cmds_io;
@@ -232,6 +234,37 @@ static int nvme_init_hctx(struct blk_mq_hw_ctx *hctx, void *data,
 	return 0;
 }
 
+/*
+ * Since namespaces shared tagset and the 'hctx' with same
+ * index shared one same nvme queue & tag, also the mapping
+ * between sw queue and hw queue is global and only depends
+ * on CPUs topo, this callback only sets irq affinity once
+ * by using the cpumask from one of hctx.
+ * */
+static void nvme_map_changed(struct blk_mq_hw_ctx *hctx,
+		unsigned int hctx_idx, bool mapped)
+{
+	struct nvme_queue *nvmeq = hctx->driver_data;
+	struct nvme_dev *dev = nvmeq->dev;
+	unsigned int irq;
+
+	if (nvmeq->mapped != mapped)
+		nvmeq->irq_affinity_set = 0;
+
+	nvmeq->mapped = mapped;
+
+	if (nvmeq->irq_affinity_set)
+		return;
+
+	irq = dev->entry[nvmeq->cq_vector].vector;
+	if (mapped)
+		irq_set_affinity_hint(irq, hctx->cpumask);
+	else
+		irq_set_affinity_hint(irq, NULL);
+
+	nvmeq->irq_affinity_set = 1;
+}
+
 static int nvme_init_request(void *data, struct request *req,
 				unsigned int hctx_idx, unsigned int rq_idx,
 				unsigned int numa_node)
@@ -1664,6 +1697,7 @@ static struct blk_mq_ops nvme_mq_ops = {
 	.queue_rq	= nvme_queue_rq,
 	.map_queue	= blk_mq_map_queue,
 	.init_hctx	= nvme_init_hctx,
+	.map_changed    = nvme_map_changed,
 	.init_request	= nvme_init_request,
 	.timeout	= nvme_timeout,
 };
@@ -2953,22 +2987,6 @@ static const struct file_operations nvme_dev_fops = {
 	.compat_ioctl	= nvme_dev_ioctl,
 };
 
-static void nvme_set_irq_hints(struct nvme_dev *dev)
-{
-	struct nvme_queue *nvmeq;
-	int i;
-
-	for (i = 0; i < dev->online_queues; i++) {
-		nvmeq = dev->queues[i];
-
-		if (!nvmeq->tags || !(*nvmeq->tags))
-			continue;
-
-		irq_set_affinity_hint(dev->entry[nvmeq->cq_vector].vector,
-					blk_mq_tags_cpumask(*nvmeq->tags));
-	}
-}
-
 static int nvme_dev_start(struct nvme_dev *dev)
 {
 	int result;
@@ -3010,8 +3028,6 @@ static int nvme_dev_start(struct nvme_dev *dev)
 	if (result)
 		goto free_tags;
 
-	nvme_set_irq_hints(dev);
-
 	dev->event_limit = 1;
 	return result;
 
@@ -3062,7 +3078,6 @@ static int nvme_dev_resume(struct nvme_dev *dev)
 	} else {
 		nvme_unfreeze_queues(dev);
 		nvme_dev_add(dev);
-		nvme_set_irq_hints(dev);
 	}
 	return 0;
 }
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1234760

Hi Ming,

from a quick gance this looks very useful.  Please split the callback
into two instead of overloading them with the bool mapped parameter.

Please keep Akinobu Mita in the loop, as this should go on top of his blk-mq
fixes.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1235574

On Tue, Sep 29, 2015 at 3:07 PM, Christoph Hellwig <hch@lst.de> wrote:
> Hi Ming,
>
> from a quick gance this looks very useful.  Please split the callback
> into two instead of overloading them with the bool mapped parameter.

The mapped parameter can be removed since it can be figured out by
checking 'hctx->tags'.

IMO, it isn't good to split it into two callback, such as,
map_activate/map_deactivate or other names, and the two callback
can't be symmetrical at all.  When CPU hotplug happened, it is just
the mapping changed, neigher setuping nor tearing down since it is
1:N mapping between hw queue and percpu sw queue.

So I suggest to keep it as .map_changed(), or do you have better idea?

>
> Please keep Akinobu Mita in the loop, as this should go on top of his blk-mq
> fixes.

OK.

-- 
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1235154

On Mon, 28 Sep 2015, Ming Lei wrote:
> This patchset introduces .map_changed callback into 'struct blk_mq_ops',
> and use this callback to get NVMe notified about the mapping changed event,
> then NVMe can update the irq affinity hint for its queues.

I think this is going the wrong direction. Shouldn't we provide blk-mq
the vectors in the tag set so that layer can manage the irq hints?

This could lead to more cpu-queue assignment optimizations from using
that information. For example, two h/w contexts sharing the same vector
shouldn't be assigned to cpus on different NUMA nodes.

> Also the 'cpumask' in 'struct blk_mq_tags' isn't needed any more, so remove
> that and related kernel interface.

It was added to the tags because the cpu mask is an artifact of the
tags rather that duplicating it across all the h/w contexts sharing the
same set. It also doesn't let a h/w context from one namespace overwrite
another's cpu affinity mask when they share the same vector.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1235171

On 09/29/2015 08:26 AM, Keith Busch wrote:
> On Mon, 28 Sep 2015, Ming Lei wrote:
>> This patchset introduces .map_changed callback into 'struct blk_mq_ops',
>> and use this callback to get NVMe notified about the mapping changed
>> event,
>> then NVMe can update the irq affinity hint for its queues.
>
> I think this is going the wrong direction. Shouldn't we provide blk-mq
> the vectors in the tag set so that layer can manage the irq hints?
>
> This could lead to more cpu-queue assignment optimizations from using
> that information. For example, two h/w contexts sharing the same vector
> shouldn't be assigned to cpus on different NUMA nodes.

I agree, this is moving in the wrong direction. Currently the sw <->hw 
queue mappings are in blk-mq, and this is the exact same information 
base we need for IRQ affinity handling. We need to move in the direction 
of having blk-mq helpers handle that part too, not pass notifications to 
the lower level driver to update its IRQ mappings.

>> Also the 'cpumask' in 'struct blk_mq_tags' isn't needed any more, so
>> remove
>> that and related kernel interface.
>
> It was added to the tags because the cpu mask is an artifact of the
> tags rather that duplicating it across all the h/w contexts sharing the
> same set. It also doesn't let a h/w context from one namespace overwrite
> another's cpu affinity mask when they share the same vector.

So having the mask in the tags is really odd, it should be in some 
per-device type data instead.

-- 
Jens Axboe

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1235585

On Tue, Sep 29, 2015 at 10:47 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 09/29/2015 08:26 AM, Keith Busch wrote:
>>
>> On Mon, 28 Sep 2015, Ming Lei wrote:
>>>
>>> This patchset introduces .map_changed callback into 'struct blk_mq_ops',
>>> and use this callback to get NVMe notified about the mapping changed
>>> event,
>>> then NVMe can update the irq affinity hint for its queues.
>>
>>
>> I think this is going the wrong direction. Shouldn't we provide blk-mq
>> the vectors in the tag set so that layer can manage the irq hints?
>>
>> This could lead to more cpu-queue assignment optimizations from using
>> that information. For example, two h/w contexts sharing the same vector
>> shouldn't be assigned to cpus on different NUMA nodes.
>
>
> I agree, this is moving in the wrong direction. Currently the sw <->hw queue
> mappings are in blk-mq, and this is the exact same information base we need
> for IRQ affinity handling. We need to move in the direction of having blk-mq
> helpers handle that part too, not pass notifications to the lower level
> driver to update its IRQ mappings.

Yes, I thought of that before, but it has the following cons:

- some drivers/devices may need different IRQ affinity policy, such as virtio
devices which has its own set affinity handler(see virtqueue_set_affinity()),
and it is offten not efficient to handle the virt queue's irq on more
than one CPU.

- block core has to get the irq vector information which has to be
setup/finalized
before blk-mq uses that for setting irq affinity, for example, in case
NVMe's admin
queue, its vector can be changed after admin queue's initialization.

That is why I said this approach is more flexible.

>
>>> Also the 'cpumask' in 'struct blk_mq_tags' isn't needed any more, so
>>> remove
>>> that and related kernel interface.
>>
>>
>> It was added to the tags because the cpu mask is an artifact of the
>> tags rather that duplicating it across all the h/w contexts sharing the
>> same set. It also doesn't let a h/w context from one namespace overwrite
>> another's cpu affinity mask when they share the same vector.
>
>
> So having the mask in the tags is really odd, it should be in some
> per-device type data instead.

Agree, removing the mask in tags is one of this patchset's motivation.


-- 
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1235611

On Tue, 29 Sep 2015, Ming Lei wrote:
> Yes, I thought of that before, but it has the following cons:
>
> - some drivers/devices may need different IRQ affinity policy, such as virtio
> devices which has its own set affinity handler(see virtqueue_set_affinity()),

That's not a very good example to support your cause; virtio_scsi's use
is a perfect example for one that would benefit from letting blk-mq
handle affinity. virtio_scsi sets affinity only when there is a 1:1
mapping of cpu's to queue's, but this driver doesn't know the mapping
that blk-mq used, creating a potentially less than optimal mapping.

> - block core has to get the irq vector information which has to be
> setup/finalized
> before blk-mq uses that for setting irq affinity, for example, in case
> NVMe's admin
> queue, its vector can be changed after admin queue's initialization.

Why do you want to put a hint on the admin queue's irq?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1235646

On Wed, Sep 30, 2015 at 6:45 AM, Keith Busch <keith.busch@intel.com> wrote:
> On Tue, 29 Sep 2015, Ming Lei wrote:
>>
>> Yes, I thought of that before, but it has the following cons:
>>
>> - some drivers/devices may need different IRQ affinity policy, such as
>> virtio
>> devices which has its own set affinity handler(see
>> virtqueue_set_affinity()),
>
>
> That's not a very good example to support your cause; virtio_scsi's use
> is a perfect example for one that would benefit from letting blk-mq
> handle affinity. virtio_scsi sets affinity only when there is a 1:1
> mapping of cpu's to queue's, but this driver doesn't know the mapping
> that blk-mq used, creating a potentially less than optimal mapping.

The 1:1 mapping is introduced before blk-mq, and that doesn't mean we
have to do that for blk-mq.

Actualy I mean virtio-scsi just lets the 1st CPU of the cpumask handle
the virt-queue's irq, instead of all CPUs mapped to the hw queue(virt-queue).

>
>> - block core has to get the irq vector information which has to be
>> setup/finalized
>> before blk-mq uses that for setting irq affinity, for example, in case
>> NVMe's admin
>> queue, its vector can be changed after admin queue's initialization.
>
>
> Why do you want to put a hint on the admin queue's irq?

No, I don't want, and it is just a example, I mean other drivers/devices
may have this kind of situation too.

-- 
Ming Lei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web