* Re: [PATCH V2 5/6] virtio: add one field into virtio_device for recording if device uses managed irq
2021-07-02 15:05 ` [PATCH V2 5/6] virtio: add one field into virtio_device for recording if device uses managed irq Ming Lei
@ 2021-07-02 15:55 ` Michael S. Tsirkin
2021-07-05 2:48 ` Ming Lei
2021-07-05 3:59 ` Jason Wang
2021-07-06 5:42 ` Christoph Hellwig
2 siblings, 1 reply; 10+ messages in thread
From: Michael S. Tsirkin @ 2021-07-02 15:55 UTC (permalink / raw)
To: Ming Lei
Cc: Jens Axboe, Damien Le Moal, John Garry, linux-scsi,
Martin K . Petersen, Daniel Wagner, linux-nvme, virtualization,
linux-block, Keith Busch, Wen Xiong, Christoph Hellwig,
Sagi Grimberg
On Fri, Jul 02, 2021 at 11:05:54PM +0800, Ming Lei wrote:
> blk-mq needs to know if the device uses managed irq, so add one field
> to virtio_device for recording if device uses managed irq.
>
> If the driver use managed irq, this flag has to be set so it can be
> passed to blk-mq.
>
> Cc: "Michael S. Tsirkin" <mst@redhat.com>
> Cc: Jason Wang <jasowang@redhat.com>
> Cc: virtualization@lists.linux-foundation.org
> Signed-off-by: Ming Lei <ming.lei@redhat.com>
The API seems somewhat confusing. virtio does not request
a managed irq as such, does it? I think it's a decision taken
by the irq core.
Any way to query the irq to find out if it's managed?
> ---
> drivers/block/virtio_blk.c | 2 ++
> drivers/scsi/virtio_scsi.c | 1 +
> drivers/virtio/virtio_pci_common.c | 1 +
> include/linux/virtio.h | 1 +
> 4 files changed, 5 insertions(+)
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index e4bd3b1fc3c2..33b9c80ac475 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -764,6 +764,8 @@ static int virtblk_probe(struct virtio_device *vdev)
> vblk->tag_set.queue_depth = queue_depth;
> vblk->tag_set.numa_node = NUMA_NO_NODE;
> vblk->tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
> + if (vdev->use_managed_irq)
> + vblk->tag_set.flags |= BLK_MQ_F_MANAGED_IRQ;
> vblk->tag_set.cmd_size =
> sizeof(struct virtblk_req) +
> sizeof(struct scatterlist) * sg_elems;
> diff --git a/drivers/scsi/virtio_scsi.c b/drivers/scsi/virtio_scsi.c
> index b9c86a7e3b97..f301917abc84 100644
> --- a/drivers/scsi/virtio_scsi.c
> +++ b/drivers/scsi/virtio_scsi.c
> @@ -891,6 +891,7 @@ static int virtscsi_probe(struct virtio_device *vdev)
> shost->max_channel = 0;
> shost->max_cmd_len = VIRTIO_SCSI_CDB_SIZE;
> shost->nr_hw_queues = num_queues;
> + shost->use_managed_irq = vdev->use_managed_irq;
>
> #ifdef CONFIG_BLK_DEV_INTEGRITY
> if (virtio_has_feature(vdev, VIRTIO_SCSI_F_T10_PI)) {
> diff --git a/drivers/virtio/virtio_pci_common.c b/drivers/virtio/virtio_pci_common.c
> index 222d630c41fc..f2ac48fb477b 100644
> --- a/drivers/virtio/virtio_pci_common.c
> +++ b/drivers/virtio/virtio_pci_common.c
> @@ -128,6 +128,7 @@ static int vp_request_msix_vectors(struct virtio_device *vdev, int nvectors,
> if (desc) {
> flags |= PCI_IRQ_AFFINITY;
> desc->pre_vectors++; /* virtio config vector */
> + vdev->use_managed_irq = true;
> }
>
> err = pci_alloc_irq_vectors_affinity(vp_dev->pci_dev, nvectors,
> diff --git a/include/linux/virtio.h b/include/linux/virtio.h
> index b1894e0323fa..85cc773b2dc7 100644
> --- a/include/linux/virtio.h
> +++ b/include/linux/virtio.h
> @@ -109,6 +109,7 @@ struct virtio_device {
> bool failed;
> bool config_enabled;
> bool config_change_pending;
> + bool use_managed_irq;
> spinlock_t config_lock;
> struct device dev;
> struct virtio_device_id id;
> --
> 2.31.1
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread* Re: [PATCH V2 5/6] virtio: add one field into virtio_device for recording if device uses managed irq
2021-07-02 15:55 ` Michael S. Tsirkin
@ 2021-07-05 2:48 ` Ming Lei
0 siblings, 0 replies; 10+ messages in thread
From: Ming Lei @ 2021-07-05 2:48 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Jens Axboe, Damien Le Moal, John Garry, linux-scsi,
Martin K . Petersen, Daniel Wagner, linux-nvme, virtualization,
linux-block, Keith Busch, Wen Xiong, Christoph Hellwig,
Sagi Grimberg
On Fri, Jul 02, 2021 at 11:55:40AM -0400, Michael S. Tsirkin wrote:
> On Fri, Jul 02, 2021 at 11:05:54PM +0800, Ming Lei wrote:
> > blk-mq needs to know if the device uses managed irq, so add one field
> > to virtio_device for recording if device uses managed irq.
> >
> > If the driver use managed irq, this flag has to be set so it can be
> > passed to blk-mq.
> >
> > Cc: "Michael S. Tsirkin" <mst@redhat.com>
> > Cc: Jason Wang <jasowang@redhat.com>
> > Cc: virtualization@lists.linux-foundation.org
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
>
>
> The API seems somewhat confusing. virtio does not request
> a managed irq as such, does it? I think it's a decision taken
> by the irq core.
vp_request_msix_vectors():
if (desc) {
flags |= PCI_IRQ_AFFINITY;
desc->pre_vectors++; /* virtio config vector */
vdev->use_managed_irq = true;
}
err = pci_alloc_irq_vectors_affinity(vp_dev->pci_dev, nvectors,
nvectors, flags, desc);
Managed irq is used once PCI_IRQ_AFFINITY is passed to
pci_alloc_irq_vectors_affinity().
>
> Any way to query the irq to find out if it's managed?
So far the managed info isn't exported via /proc/irq, but if one irq is
managed, its smp_affinity & smp_affinity_list attributes can't be
changed successfully.
If irq's debugfs is enabled, this info can be retrieved.
Thanks,
Ming
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2 5/6] virtio: add one field into virtio_device for recording if device uses managed irq
2021-07-02 15:05 ` [PATCH V2 5/6] virtio: add one field into virtio_device for recording if device uses managed irq Ming Lei
2021-07-02 15:55 ` Michael S. Tsirkin
@ 2021-07-05 3:59 ` Jason Wang
2021-07-06 5:42 ` Christoph Hellwig
2 siblings, 0 replies; 10+ messages in thread
From: Jason Wang @ 2021-07-05 3:59 UTC (permalink / raw)
To: Ming Lei, Jens Axboe, Christoph Hellwig, Martin K . Petersen,
linux-block, linux-nvme, linux-scsi
Cc: Damien Le Moal, Sagi Grimberg, Daniel Wagner, Michael S. Tsirkin,
John Garry, virtualization, Keith Busch, Wen Xiong
在 2021/7/2 下午11:05, Ming Lei 写道:
> blk-mq needs to know if the device uses managed irq, so add one field
> to virtio_device for recording if device uses managed irq.
>
> If the driver use managed irq, this flag has to be set so it can be
> passed to blk-mq.
>
> Cc: "Michael S. Tsirkin"<mst@redhat.com>
> Cc: Jason Wang<jasowang@redhat.com>
> Cc:virtualization@lists.linux-foundation.org
> Signed-off-by: Ming Lei<ming.lei@redhat.com>
> ---
> drivers/block/virtio_blk.c | 2 ++
> drivers/scsi/virtio_scsi.c | 1 +
> drivers/virtio/virtio_pci_common.c | 1 +
> include/linux/virtio.h | 1 +
> 4 files changed, 5 insertions(+)
>
> diff --git a/drivers/block/virtio_blk.c b/drivers/block/virtio_blk.c
> index e4bd3b1fc3c2..33b9c80ac475 100644
> --- a/drivers/block/virtio_blk.c
> +++ b/drivers/block/virtio_blk.c
> @@ -764,6 +764,8 @@ static int virtblk_probe(struct virtio_device *vdev)
> vblk->tag_set.queue_depth = queue_depth;
> vblk->tag_set.numa_node = NUMA_NO_NODE;
> vblk->tag_set.flags = BLK_MQ_F_SHOULD_MERGE;
> + if (vdev->use_managed_irq)
> + vblk->tag_set.flags |= BLK_MQ_F_MANAGED_IRQ;
I'm not familiar with blk mq.
But the name is kind of confusing, I guess
"BLK_MQ_F_AFFINITY_MANAGED_IRQ" is better? (Consider we had
"IRQD_AFFINITY_MANAGED")
This helps me to differ this from the devres (device managed IRQ) at least.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2 5/6] virtio: add one field into virtio_device for recording if device uses managed irq
2021-07-02 15:05 ` [PATCH V2 5/6] virtio: add one field into virtio_device for recording if device uses managed irq Ming Lei
2021-07-02 15:55 ` Michael S. Tsirkin
2021-07-05 3:59 ` Jason Wang
@ 2021-07-06 5:42 ` Christoph Hellwig
2021-07-06 7:53 ` Ming Lei
2021-07-07 9:06 ` Thomas Gleixner
2 siblings, 2 replies; 10+ messages in thread
From: Christoph Hellwig @ 2021-07-06 5:42 UTC (permalink / raw)
To: Ming Lei
Cc: Jens Axboe, Damien Le Moal, John Garry, linux-scsi,
Martin K . Petersen, Michael S. Tsirkin, Daniel Wagner,
linux-nvme, virtualization, linux-block, Keith Busch,
Thomas Gleixner, Wen Xiong, Christoph Hellwig, Sagi Grimberg
On Fri, Jul 02, 2021 at 11:05:54PM +0800, Ming Lei wrote:
> blk-mq needs to know if the device uses managed irq, so add one field
> to virtio_device for recording if device uses managed irq.
>
> If the driver use managed irq, this flag has to be set so it can be
> passed to blk-mq.
I don't think all this boilerplate code make a whole lot of sense.
I think we need to record this information deep down in the irq code by
setting a flag in struct device only if pci_alloc_irq_vectors_affinity
atually managed to allocate multiple vectors and the PCI_IRQ_AFFINITY
flag was set. Then blk-mq can look at that flag, and also check that
more than one queue is in used and work based on that.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2 5/6] virtio: add one field into virtio_device for recording if device uses managed irq
2021-07-06 5:42 ` Christoph Hellwig
@ 2021-07-06 7:53 ` Ming Lei
2021-07-07 9:06 ` Thomas Gleixner
1 sibling, 0 replies; 10+ messages in thread
From: Ming Lei @ 2021-07-06 7:53 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, Damien Le Moal, Sagi Grimberg, Martin K . Petersen,
Michael S. Tsirkin, John Garry, linux-nvme, virtualization,
linux-block, linux-scsi, Keith Busch, Thomas Gleixner,
Daniel Wagner, Wen Xiong
On Tue, Jul 06, 2021 at 07:42:03AM +0200, Christoph Hellwig wrote:
> On Fri, Jul 02, 2021 at 11:05:54PM +0800, Ming Lei wrote:
> > blk-mq needs to know if the device uses managed irq, so add one field
> > to virtio_device for recording if device uses managed irq.
> >
> > If the driver use managed irq, this flag has to be set so it can be
> > passed to blk-mq.
>
> I don't think all this boilerplate code make a whole lot of sense.
> I think we need to record this information deep down in the irq code by
> setting a flag in struct device only if pci_alloc_irq_vectors_affinity
> atually managed to allocate multiple vectors and the PCI_IRQ_AFFINITY
> flag was set. Then blk-mq can look at that flag, and also check that
> more than one queue is in used and work based on that.
How can blk-mq look at that flag? Usually blk-mq doesn't play with
physical device(HBA).
So far almost all physically properties(segment, max_hw_sectors, ...)
are not provided to blk-mq directly, instead by queue limits abstract.
Thanks,
Ming
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2 5/6] virtio: add one field into virtio_device for recording if device uses managed irq
2021-07-06 5:42 ` Christoph Hellwig
2021-07-06 7:53 ` Ming Lei
@ 2021-07-07 9:06 ` Thomas Gleixner
2021-07-07 9:42 ` Ming Lei
1 sibling, 1 reply; 10+ messages in thread
From: Thomas Gleixner @ 2021-07-07 9:06 UTC (permalink / raw)
To: Christoph Hellwig, Ming Lei
Cc: Jens Axboe, Damien Le Moal, John Garry, linux-scsi,
Martin K . Petersen, Michael S. Tsirkin, Daniel Wagner,
linux-nvme, virtualization, linux-block, Keith Busch, Wen Xiong,
Christoph Hellwig, Sagi Grimberg
On Tue, Jul 06 2021 at 07:42, Christoph Hellwig wrote:
> On Fri, Jul 02, 2021 at 11:05:54PM +0800, Ming Lei wrote:
>> blk-mq needs to know if the device uses managed irq, so add one field
>> to virtio_device for recording if device uses managed irq.
>>
>> If the driver use managed irq, this flag has to be set so it can be
>> passed to blk-mq.
>
> I don't think all this boilerplate code make a whole lot of sense.
> I think we need to record this information deep down in the irq code by
> setting a flag in struct device only if pci_alloc_irq_vectors_affinity
> atually managed to allocate multiple vectors and the PCI_IRQ_AFFINITY
> flag was set. Then blk-mq can look at that flag, and also check that
> more than one queue is in used and work based on that.
Ack.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2 5/6] virtio: add one field into virtio_device for recording if device uses managed irq
2021-07-07 9:06 ` Thomas Gleixner
@ 2021-07-07 9:42 ` Ming Lei
2021-07-07 14:05 ` Christoph Hellwig
0 siblings, 1 reply; 10+ messages in thread
From: Ming Lei @ 2021-07-07 9:42 UTC (permalink / raw)
To: Thomas Gleixner
Cc: Jens Axboe, Damien Le Moal, John Garry, linux-scsi,
Martin K . Petersen, Michael S. Tsirkin, Daniel Wagner,
linux-nvme, virtualization, linux-block, Keith Busch, Wen Xiong,
Christoph Hellwig, Sagi Grimberg
On Wed, Jul 07, 2021 at 11:06:25AM +0200, Thomas Gleixner wrote:
> On Tue, Jul 06 2021 at 07:42, Christoph Hellwig wrote:
> > On Fri, Jul 02, 2021 at 11:05:54PM +0800, Ming Lei wrote:
> >> blk-mq needs to know if the device uses managed irq, so add one field
> >> to virtio_device for recording if device uses managed irq.
> >>
> >> If the driver use managed irq, this flag has to be set so it can be
> >> passed to blk-mq.
> >
> > I don't think all this boilerplate code make a whole lot of sense.
> > I think we need to record this information deep down in the irq code by
> > setting a flag in struct device only if pci_alloc_irq_vectors_affinity
> > atually managed to allocate multiple vectors and the PCI_IRQ_AFFINITY
> > flag was set. Then blk-mq can look at that flag, and also check that
> > more than one queue is in used and work based on that.
>
> Ack.
The problem is that how blk-mq looks at that flag, since the device
representing the controller which allocates irq vectors isn't visible
to blk-mq.
Usually blk-mq(block layer) provides queue limits abstract for drivers
to tell any physical property(dma segment, max sectors, ...) to block
layer, that is why this patch takes this very similar usage to check if
HBA uses managed irq or not.
Thanks,
Ming
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2 5/6] virtio: add one field into virtio_device for recording if device uses managed irq
2021-07-07 9:42 ` Ming Lei
@ 2021-07-07 14:05 ` Christoph Hellwig
2021-07-08 6:34 ` Ming Lei
0 siblings, 1 reply; 10+ messages in thread
From: Christoph Hellwig @ 2021-07-07 14:05 UTC (permalink / raw)
To: Ming Lei
Cc: Jens Axboe, Damien Le Moal, John Garry, linux-scsi,
Martin K . Petersen, Michael S. Tsirkin, Daniel Wagner,
linux-nvme, virtualization, linux-block, Keith Busch,
Thomas Gleixner, Wen Xiong, Christoph Hellwig, Sagi Grimberg
On Wed, Jul 07, 2021 at 05:42:54PM +0800, Ming Lei wrote:
> The problem is that how blk-mq looks at that flag, since the device
> representing the controller which allocates irq vectors isn't visible
> to blk-mq.
In blk_mq_pci_map_queues and similar helpers.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH V2 5/6] virtio: add one field into virtio_device for recording if device uses managed irq
2021-07-07 14:05 ` Christoph Hellwig
@ 2021-07-08 6:34 ` Ming Lei
0 siblings, 0 replies; 10+ messages in thread
From: Ming Lei @ 2021-07-08 6:34 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Jens Axboe, Damien Le Moal, linux-scsi, Martin K . Petersen,
Michael S. Tsirkin, John Garry, linux-nvme, virtualization,
linux-block, Keith Busch, Thomas Gleixner, Wen Xiong,
Daniel Wagner, Sagi Grimberg
On Wed, Jul 07, 2021 at 04:05:29PM +0200, Christoph Hellwig wrote:
> On Wed, Jul 07, 2021 at 05:42:54PM +0800, Ming Lei wrote:
> > The problem is that how blk-mq looks at that flag, since the device
> > representing the controller which allocates irq vectors isn't visible
> > to blk-mq.
>
> In blk_mq_pci_map_queues and similar helpers.
Firstly it depends if drivers call into these helpers, so this way
is fragile.
Secondly, I think it isn't good to expose specific physical devices
into blk-mq which shouldn't deal with physical device directly, also
all the three helpers just duplicates same logic except for retrieving
each vector's affinity from specific physical device.
I will think about how to cleanup them.
Thanks,
Ming
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply [flat|nested] 10+ messages in thread