Re: [PATCH] virtio-blk: support polling I/O

Re: [PATCH] virtio-blk: support polling I/O

[Michael S. Tsirkin]

On Sat, Mar 12, 2022 at 12:28:32AM +0900, Suwan Kim wrote:
> diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h
> index d888f013d9ff..3fcaf937afe1 100644
> --- a/include/uapi/linux/virtio_blk.h
> +++ b/include/uapi/linux/virtio_blk.h
> @@ -119,8 +119,9 @@ struct virtio_blk_config {
>  	 * deallocation of one or more of the sectors.
>  	 */
>  	__u8 write_zeroes_may_unmap;
> +	__u8 unused1;
>  
> -	__u8 unused1[3];
> +	__virtio16 num_poll_queues;
>  } __attribute__((packed));

Same as any virtio UAPI change, this has to go through the virtio TC.
In particular I don't think gating a new config field on
an existing feature flag is a good idea.

>  /*
> -- 
> 2.26.3

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio-blk: support polling I/O

[Michael S. Tsirkin]

On Sat, Mar 12, 2022 at 01:07:23AM +0900, Suwan Kim wrote:
> On Fri, Mar 11, 2022 at 10:38:07AM -0500, Michael S. Tsirkin wrote:
> > On Sat, Mar 12, 2022 at 12:28:32AM +0900, Suwan Kim wrote:
> > > diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h
> > > index d888f013d9ff..3fcaf937afe1 100644
> > > --- a/include/uapi/linux/virtio_blk.h
> > > +++ b/include/uapi/linux/virtio_blk.h
> > > @@ -119,8 +119,9 @@ struct virtio_blk_config {
> > >  	 * deallocation of one or more of the sectors.
> > >  	 */
> > >  	__u8 write_zeroes_may_unmap;
> > > +	__u8 unused1;
> > >  
> > > -	__u8 unused1[3];
> > > +	__virtio16 num_poll_queues;
> > >  } __attribute__((packed));
> > 
> > Same as any virtio UAPI change, this has to go through the virtio TC.


Notice this pls.  Remember to copy one of the TC mailing lists.

> > In particular I don't think gating a new config field on
> > an existing feature flag is a good idea.
> 
> Did you mean that the polling should be based on a new feature like
> "VIRTIO_BLK_F_POLL" and be added at the end of features_legacy[]
> and features[]? If then, I will add the new feture flag and resend it.
> 
> Regards,
> Suwan Kim

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio-blk: support polling I/O

[Michael S. Tsirkin]

On Mon, Mar 14, 2022 at 12:25:08PM +0200, Max Gurtovoy wrote:
> 
> On 3/14/2022 11:43 AM, Suwan Kim wrote:
> > On Sun, Mar 13, 2022 at 12:37:21PM +0200, Max Gurtovoy wrote:
> > > On 3/11/2022 6:07 PM, Suwan Kim wrote:
> > > > On Fri, Mar 11, 2022 at 10:38:07AM -0500, Michael S. Tsirkin wrote:
> > > > > On Sat, Mar 12, 2022 at 12:28:32AM +0900, Suwan Kim wrote:
> > > > > > diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h
> > > > > > index d888f013d9ff..3fcaf937afe1 100644
> > > > > > --- a/include/uapi/linux/virtio_blk.h
> > > > > > +++ b/include/uapi/linux/virtio_blk.h
> > > > > > @@ -119,8 +119,9 @@ struct virtio_blk_config {
> > > > > >    	 * deallocation of one or more of the sectors.
> > > > > >    	 */
> > > > > >    	__u8 write_zeroes_may_unmap;
> > > > > > +	__u8 unused1;
> > > > > > -	__u8 unused1[3];
> > > > > > +	__virtio16 num_poll_queues;
> > > > > >    } __attribute__((packed));
> > > > > Same as any virtio UAPI change, this has to go through the virtio TC.
> > > > > In particular I don't think gating a new config field on
> > > > > an existing feature flag is a good idea.
> > > > Did you mean that the polling should be based on a new feature like
> > > > "VIRTIO_BLK_F_POLL" and be added at the end of features_legacy[]
> > > > and features[]? If then, I will add the new feture flag and resend it.
> > > Isn't there a way in the SPEC today to create a queue without interrupt
> > > vector ?
> > It seems that it is not possible to create a queue without interrupt
> > vector. If it is possible, we can expect more polling improvement.

Yes, it's possible:

Writing a valid MSI-X Table entry number, 0 to 0x7FF, to
\field{config_msix_vector}/\field{queue_msix_vector} maps interrupts triggered
by the configuration change/selected queue events respectively to
the corresponding MSI-X vector. To disable interrupts for an
event type, the driver unmaps this event by writing a special NO_VECTOR
value:

\begin{lstlisting}
/* Vector value used to disable MSI for queue */
#define VIRTIO_MSI_NO_VECTOR            0xffff
\end{lstlisting}



> MST/Jason/Stefan,
> 
> can you confirm that please ?
>
> what does VIRTQ_AVAIL_F_NO_INTERRUPT supposed to do ?

This is a hint to the device not to send interrupts.

> 
> > Regards,
> > Suwan Kim

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio-blk: support polling I/O

[Michael S. Tsirkin]

On Mon, Mar 14, 2022 at 10:22:01PM +0900, Suwan Kim wrote:
> On Mon, Mar 14, 2022 at 07:15:18AM -0400, Michael S. Tsirkin wrote:
> > On Mon, Mar 14, 2022 at 12:25:08PM +0200, Max Gurtovoy wrote:
> > > 
> > > On 3/14/2022 11:43 AM, Suwan Kim wrote:
> > > > On Sun, Mar 13, 2022 at 12:37:21PM +0200, Max Gurtovoy wrote:
> > > > > On 3/11/2022 6:07 PM, Suwan Kim wrote:
> > > > > > On Fri, Mar 11, 2022 at 10:38:07AM -0500, Michael S. Tsirkin wrote:
> > > > > > > On Sat, Mar 12, 2022 at 12:28:32AM +0900, Suwan Kim wrote:
> > > > > > > > diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h
> > > > > > > > index d888f013d9ff..3fcaf937afe1 100644
> > > > > > > > --- a/include/uapi/linux/virtio_blk.h
> > > > > > > > +++ b/include/uapi/linux/virtio_blk.h
> > > > > > > > @@ -119,8 +119,9 @@ struct virtio_blk_config {
> > > > > > > >    	 * deallocation of one or more of the sectors.
> > > > > > > >    	 */
> > > > > > > >    	__u8 write_zeroes_may_unmap;
> > > > > > > > +	__u8 unused1;
> > > > > > > > -	__u8 unused1[3];
> > > > > > > > +	__virtio16 num_poll_queues;
> > > > > > > >    } __attribute__((packed));
> > > > > > > Same as any virtio UAPI change, this has to go through the virtio TC.
> > > > > > > In particular I don't think gating a new config field on
> > > > > > > an existing feature flag is a good idea.
> > > > > > Did you mean that the polling should be based on a new feature like
> > > > > > "VIRTIO_BLK_F_POLL" and be added at the end of features_legacy[]
> > > > > > and features[]? If then, I will add the new feture flag and resend it.
> > > > > Isn't there a way in the SPEC today to create a queue without interrupt
> > > > > vector ?
> > > > It seems that it is not possible to create a queue without interrupt
> > > > vector. If it is possible, we can expect more polling improvement.
> > 
> > Yes, it's possible:
> > 
> > Writing a valid MSI-X Table entry number, 0 to 0x7FF, to
> > \field{config_msix_vector}/\field{queue_msix_vector} maps interrupts triggered
> > by the configuration change/selected queue events respectively to
> > the corresponding MSI-X vector. To disable interrupts for an
> > event type, the driver unmaps this event by writing a special NO_VECTOR
> > value:
> > 
> > \begin{lstlisting}
> > /* Vector value used to disable MSI for queue */
> > #define VIRTIO_MSI_NO_VECTOR            0xffff
> > \end{lstlisting}
>  
> Thanks for the information.
> 
> Then, in function vp_find_vqs_msix() at virtio_pci_common.c, it sets
> VIRTIO_MSI_NO_VECTOR if vritqueue->callback is NULL as below code.
> 
> static int vp_find_vqs_msix(struct virtio_device *vdev, unsigned nvqs,
> 		struct virtqueue *vqs[], vq_callback_t *callbacks[],
> ...
> 		if (!callbacks[i])
> 			msix_vec = VIRTIO_MSI_NO_VECTOR;
> ...
> 
> In oder to create poll queue in virtio-blk, I set NULL callback for
> poll virtqueues and it will create queue without irq.
> 
> Regards,
> Suwan Kim

Yes, it will.

-- 
MST

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio-blk: support polling I/O

[Michael S. Tsirkin]

On Mon, Mar 14, 2022 at 03:26:13PM +0200, Max Gurtovoy wrote:
> 
> On 3/14/2022 1:15 PM, Michael S. Tsirkin wrote:
> > On Mon, Mar 14, 2022 at 12:25:08PM +0200, Max Gurtovoy wrote:
> > > On 3/14/2022 11:43 AM, Suwan Kim wrote:
> > > > On Sun, Mar 13, 2022 at 12:37:21PM +0200, Max Gurtovoy wrote:
> > > > > On 3/11/2022 6:07 PM, Suwan Kim wrote:
> > > > > > On Fri, Mar 11, 2022 at 10:38:07AM -0500, Michael S. Tsirkin wrote:
> > > > > > > On Sat, Mar 12, 2022 at 12:28:32AM +0900, Suwan Kim wrote:
> > > > > > > > diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h
> > > > > > > > index d888f013d9ff..3fcaf937afe1 100644
> > > > > > > > --- a/include/uapi/linux/virtio_blk.h
> > > > > > > > +++ b/include/uapi/linux/virtio_blk.h
> > > > > > > > @@ -119,8 +119,9 @@ struct virtio_blk_config {
> > > > > > > >     	 * deallocation of one or more of the sectors.
> > > > > > > >     	 */
> > > > > > > >     	__u8 write_zeroes_may_unmap;
> > > > > > > > +	__u8 unused1;
> > > > > > > > -	__u8 unused1[3];
> > > > > > > > +	__virtio16 num_poll_queues;
> > > > > > > >     } __attribute__((packed));
> > > > > > > Same as any virtio UAPI change, this has to go through the virtio TC.
> > > > > > > In particular I don't think gating a new config field on
> > > > > > > an existing feature flag is a good idea.
> > > > > > Did you mean that the polling should be based on a new feature like
> > > > > > "VIRTIO_BLK_F_POLL" and be added at the end of features_legacy[]
> > > > > > and features[]? If then, I will add the new feture flag and resend it.
> > > > > Isn't there a way in the SPEC today to create a queue without interrupt
> > > > > vector ?
> > > > It seems that it is not possible to create a queue without interrupt
> > > > vector. If it is possible, we can expect more polling improvement.
> > Yes, it's possible:
> > 
> > Writing a valid MSI-X Table entry number, 0 to 0x7FF, to
> > \field{config_msix_vector}/\field{queue_msix_vector} maps interrupts triggered
> > by the configuration change/selected queue events respectively to
> > the corresponding MSI-X vector. To disable interrupts for an
> > event type, the driver unmaps this event by writing a special NO_VECTOR
> > value:
> > 
> > \begin{lstlisting}
> > /* Vector value used to disable MSI for queue */
> > #define VIRTIO_MSI_NO_VECTOR            0xffff
> > \end{lstlisting}
> > 
> > 
> > 
> > > MST/Jason/Stefan,
> > > 
> > > can you confirm that please ?
> > > 
> > > what does VIRTQ_AVAIL_F_NO_INTERRUPT supposed to do ?
> > This is a hint to the device not to send interrupts.
> 
> Why do you need a hint if the driver implicitly wrote 0xffff to disable MSI
> for a virtqueue ?


VIRTIO_MSI_NO_VECTOR is an expensive write into config space, followed
by an even more expensive read. Reliable and appropriate if you turn
events on/off very rarely.

VIRTQ_AVAIL_F_NO_INTERRUPT is an in-memory write so it's much cheaper,
but it's less reliable. Appropriate if you need to turn events on/off a
lot.



> 
> > 
> > > > Regards,
> > > > Suwan Kim

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio-blk: support polling I/O

[Michael S. Tsirkin]

On Mon, Mar 14, 2022 at 06:33:06PM +0200, Max Gurtovoy wrote:
> 
> On 3/14/2022 5:15 PM, Michael S. Tsirkin wrote:
> > On Mon, Mar 14, 2022 at 03:26:13PM +0200, Max Gurtovoy wrote:
> > > On 3/14/2022 1:15 PM, Michael S. Tsirkin wrote:
> > > > On Mon, Mar 14, 2022 at 12:25:08PM +0200, Max Gurtovoy wrote:
> > > > > On 3/14/2022 11:43 AM, Suwan Kim wrote:
> > > > > > On Sun, Mar 13, 2022 at 12:37:21PM +0200, Max Gurtovoy wrote:
> > > > > > > On 3/11/2022 6:07 PM, Suwan Kim wrote:
> > > > > > > > On Fri, Mar 11, 2022 at 10:38:07AM -0500, Michael S. Tsirkin wrote:
> > > > > > > > > On Sat, Mar 12, 2022 at 12:28:32AM +0900, Suwan Kim wrote:
> > > > > > > > > > diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h
> > > > > > > > > > index d888f013d9ff..3fcaf937afe1 100644
> > > > > > > > > > --- a/include/uapi/linux/virtio_blk.h
> > > > > > > > > > +++ b/include/uapi/linux/virtio_blk.h
> > > > > > > > > > @@ -119,8 +119,9 @@ struct virtio_blk_config {
> > > > > > > > > >      	 * deallocation of one or more of the sectors.
> > > > > > > > > >      	 */
> > > > > > > > > >      	__u8 write_zeroes_may_unmap;
> > > > > > > > > > +	__u8 unused1;
> > > > > > > > > > -	__u8 unused1[3];
> > > > > > > > > > +	__virtio16 num_poll_queues;
> > > > > > > > > >      } __attribute__((packed));
> > > > > > > > > Same as any virtio UAPI change, this has to go through the virtio TC.
> > > > > > > > > In particular I don't think gating a new config field on
> > > > > > > > > an existing feature flag is a good idea.
> > > > > > > > Did you mean that the polling should be based on a new feature like
> > > > > > > > "VIRTIO_BLK_F_POLL" and be added at the end of features_legacy[]
> > > > > > > > and features[]? If then, I will add the new feture flag and resend it.
> > > > > > > Isn't there a way in the SPEC today to create a queue without interrupt
> > > > > > > vector ?
> > > > > > It seems that it is not possible to create a queue without interrupt
> > > > > > vector. If it is possible, we can expect more polling improvement.
> > > > Yes, it's possible:
> > > > 
> > > > Writing a valid MSI-X Table entry number, 0 to 0x7FF, to
> > > > \field{config_msix_vector}/\field{queue_msix_vector} maps interrupts triggered
> > > > by the configuration change/selected queue events respectively to
> > > > the corresponding MSI-X vector. To disable interrupts for an
> > > > event type, the driver unmaps this event by writing a special NO_VECTOR
> > > > value:
> > > > 
> > > > \begin{lstlisting}
> > > > /* Vector value used to disable MSI for queue */
> > > > #define VIRTIO_MSI_NO_VECTOR            0xffff
> > > > \end{lstlisting}
> > > > 
> > > > 
> > > > 
> > > > > MST/Jason/Stefan,
> > > > > 
> > > > > can you confirm that please ?
> > > > > 
> > > > > what does VIRTQ_AVAIL_F_NO_INTERRUPT supposed to do ?
> > > > This is a hint to the device not to send interrupts.
> > > Why do you need a hint if the driver implicitly wrote 0xffff to disable MSI
> > > for a virtqueue ?
> > 
> > VIRTIO_MSI_NO_VECTOR is an expensive write into config space, followed
> > by an even more expensive read. Reliable and appropriate if you turn
> > events on/off very rarely.
> > 
> > VIRTQ_AVAIL_F_NO_INTERRUPT is an in-memory write so it's much cheaper,
> > but it's less reliable. Appropriate if you need to turn events on/off a
> > lot.
> 
> An "expensive" operation in the ctrl path during vq creation is fine IMO.

Yes.

> I see that nobody even used VIRTQ_AVAIL_F_NO_INTERRUPT in-memory write in
> Linux.

Because it's called VRING_AVAIL_F_NO_INTERRUPT there.

> > 
> > 
> > > > > > Regards,
> > > > > > Suwan Kim

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio-blk: support polling I/O

[Jason Wang]

在 2022/3/11 下午11:28, Suwan Kim 写道:
> diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h
> index d888f013d9ff..3fcaf937afe1 100644
> --- a/include/uapi/linux/virtio_blk.h
> +++ b/include/uapi/linux/virtio_blk.h
> @@ -119,8 +119,9 @@ struct virtio_blk_config {
>   	 * deallocation of one or more of the sectors.
>   	 */
>   	__u8 write_zeroes_may_unmap;
> +	__u8 unused1;
>   
> -	__u8 unused1[3];
> +	__virtio16 num_poll_queues;
>   } __attribute__((packed));


This looks like a implementation specific (virtio-blk-pci) optimization, 
how about other implementation like vhost-user-blk?

Thanks

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio-blk: support polling I/O

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 1515 bytes --]

On Mon, Mar 14, 2022 at 09:33:08PM +0900, Suwan Kim wrote:
> On Mon, Mar 14, 2022 at 02:14:53PM +0800, Jason Wang wrote:
> > 
> > 在 2022/3/11 下午11:28, Suwan Kim 写道:
> > > diff --git a/include/uapi/linux/virtio_blk.h b/include/uapi/linux/virtio_blk.h
> > > index d888f013d9ff..3fcaf937afe1 100644
> > > --- a/include/uapi/linux/virtio_blk.h
> > > +++ b/include/uapi/linux/virtio_blk.h
> > > @@ -119,8 +119,9 @@ struct virtio_blk_config {
> > >   	 * deallocation of one or more of the sectors.
> > >   	 */
> > >   	__u8 write_zeroes_may_unmap;
> > > +	__u8 unused1;
> > > -	__u8 unused1[3];
> > > +	__virtio16 num_poll_queues;
> > >   } __attribute__((packed));
> > 
> > 
> > This looks like a implementation specific (virtio-blk-pci) optimization, how
> > about other implementation like vhost-user-blk?
> 
> I didn’t consider vhost-user-blk yet. But does vhost-user-blk also
> use vritio_blk_config as kernel-qemu interface?
> 
> Does vhost-user-blk need additional modification to support polling
> in kernel side?

I think QEMU's --device vhost-user-blk-pci will work with this patch
series because QEMU passes the struct virtio_blk_config from the
vhost-user-blk server to the guest. If a new vhost-user-blk server
supports num_poll_queues then the guest will see the config field.

However, the new feature bit that was discussed in another sub-thread
needs to be added to hw/block/vhost-user-blk.c:user_feature_bits[] and
QEMU needs to be recompiled.

Stefan

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio-blk: support polling I/O

[Jason Wang]

[-- Attachment #1.1: Type: text/plain, Size: 1539 bytes --]

On Mon, Mar 14, 2022 at 8:33 PM Suwan Kim <suwan.kim027@gmail.com> wrote:

> On Mon, Mar 14, 2022 at 02:14:53PM +0800, Jason Wang wrote:
> >
> > 在 2022/3/11 下午11:28, Suwan Kim 写道:
> > > diff --git a/include/uapi/linux/virtio_blk.h
> b/include/uapi/linux/virtio_blk.h
> > > index d888f013d9ff..3fcaf937afe1 100644
> > > --- a/include/uapi/linux/virtio_blk.h
> > > +++ b/include/uapi/linux/virtio_blk.h
> > > @@ -119,8 +119,9 @@ struct virtio_blk_config {
> > >      * deallocation of one or more of the sectors.
> > >      */
> > >     __u8 write_zeroes_may_unmap;
> > > +   __u8 unused1;
> > > -   __u8 unused1[3];
> > > +   __virtio16 num_poll_queues;
> > >   } __attribute__((packed));
> >
> >
> > This looks like a implementation specific (virtio-blk-pci) optimization,
> how
> > about other implementation like vhost-user-blk?
>
> I didn’t consider vhost-user-blk yet. But does vhost-user-blk also
> use vritio_blk_config as kernel-qemu interface?
>

Yes, but see below.


>
> Does vhost-user-blk need additional modification to support polling
> in kernel side?
>


No, but the issue is, things like polling looks not a good candidate for
the attributes belonging to the device but the driver. So I have more
questions:

1) what does it really mean for hardware virtio block devices?
2) Does driver polling help for the qemu implementation without polling?
3) Using blk_config means we can only get the benefit from the new device

Thanks



> Regards,
> Suwan Kim
>
>

[-- Attachment #1.2: Type: text/html, Size: 2415 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio-blk: support polling I/O

[Michael S. Tsirkin]

On Tue, Mar 15, 2022 at 11:43:18PM +0900, Suwan Kim wrote:
> On Tue, Mar 15, 2022 at 04:59:23PM +0800, Jason Wang wrote:
> > On Mon, Mar 14, 2022 at 8:33 PM Suwan Kim <suwan.kim027@gmail.com> wrote:
> > 
> > > On Mon, Mar 14, 2022 at 02:14:53PM +0800, Jason Wang wrote:
> > > >
> > > > 在 2022/3/11 下午11:28, Suwan Kim 写道:
> > > > > diff --git a/include/uapi/linux/virtio_blk.h
> > > b/include/uapi/linux/virtio_blk.h
> > > > > index d888f013d9ff..3fcaf937afe1 100644
> > > > > --- a/include/uapi/linux/virtio_blk.h
> > > > > +++ b/include/uapi/linux/virtio_blk.h
> > > > > @@ -119,8 +119,9 @@ struct virtio_blk_config {
> > > > >      * deallocation of one or more of the sectors.
> > > > >      */
> > > > >     __u8 write_zeroes_may_unmap;
> > > > > +   __u8 unused1;
> > > > > -   __u8 unused1[3];
> > > > > +   __virtio16 num_poll_queues;
> > > > >   } __attribute__((packed));
> > > >
> > > >
> > > > This looks like a implementation specific (virtio-blk-pci) optimization,
> > > how
> > > > about other implementation like vhost-user-blk?
> > >
> > > I didn’t consider vhost-user-blk yet. But does vhost-user-blk also
> > > use vritio_blk_config as kernel-qemu interface?
> > >
> > 
> > Yes, but see below.
> > 
> > 
> > >
> > > Does vhost-user-blk need additional modification to support polling
> > > in kernel side?
> > >
> > 
> > 
> > No, but the issue is, things like polling looks not a good candidate for
> > the attributes belonging to the device but the driver. So I have more
> > questions:
> > 
> > 1) what does it really mean for hardware virtio block devices?
> > 2) Does driver polling help for the qemu implementation without polling?
> > 3) Using blk_config means we can only get the benefit from the new device
> 
> 1) what does it really mean for hardware virtio block devices?
> 3) Using blk_config means we can only get the benefit from the new device
> 
> This patch adds dedicated HW queue for polling purpose to virtio
> block device.
> 
> So I think it can be a new hw feature. And it can be a new device
> that supports hw poll queue.
> 
> BTW, I have other idea about it.
> 
> How about adding “num-poll-queues" property as a driver parameter
> like NVMe driver, not to QEMU virtio-blk-pci property?
> 
> If then, we don’t need to modify virtio_blk_config.
> And we can apply the polling feature only to virtio-blk-pci.
> But can QEMU pass “num-poll-queues" to virtio-blk driver param?

Same as any other driver parameter, pass it on kernel command line.

> 
> 
> 2) Does driver polling help for the qemu implementation without polling?
> 
> Sorry, I didn't understand your question. Could you please explain more about?
> 
> Regards,
> Suwan Kim

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio-blk: support polling I/O

[Jason Wang]

On Tue, Mar 15, 2022 at 10:43 PM Suwan Kim <suwan.kim027@gmail.com> wrote:
>
> On Tue, Mar 15, 2022 at 04:59:23PM +0800, Jason Wang wrote:
> > On Mon, Mar 14, 2022 at 8:33 PM Suwan Kim <suwan.kim027@gmail.com> wrote:
> >
> > > On Mon, Mar 14, 2022 at 02:14:53PM +0800, Jason Wang wrote:
> > > >
> > > > 在 2022/3/11 下午11:28, Suwan Kim 写道:
> > > > > diff --git a/include/uapi/linux/virtio_blk.h
> > > b/include/uapi/linux/virtio_blk.h
> > > > > index d888f013d9ff..3fcaf937afe1 100644
> > > > > --- a/include/uapi/linux/virtio_blk.h
> > > > > +++ b/include/uapi/linux/virtio_blk.h
> > > > > @@ -119,8 +119,9 @@ struct virtio_blk_config {
> > > > >      * deallocation of one or more of the sectors.
> > > > >      */
> > > > >     __u8 write_zeroes_may_unmap;
> > > > > +   __u8 unused1;
> > > > > -   __u8 unused1[3];
> > > > > +   __virtio16 num_poll_queues;
> > > > >   } __attribute__((packed));
> > > >
> > > >
> > > > This looks like a implementation specific (virtio-blk-pci) optimization,
> > > how
> > > > about other implementation like vhost-user-blk?
> > >
> > > I didn’t consider vhost-user-blk yet. But does vhost-user-blk also
> > > use vritio_blk_config as kernel-qemu interface?
> > >
> >
> > Yes, but see below.
> >
> >
> > >
> > > Does vhost-user-blk need additional modification to support polling
> > > in kernel side?
> > >
> >
> >
> > No, but the issue is, things like polling looks not a good candidate for
> > the attributes belonging to the device but the driver. So I have more
> > questions:
> >
> > 1) what does it really mean for hardware virtio block devices?
> > 2) Does driver polling help for the qemu implementation without polling?
> > 3) Using blk_config means we can only get the benefit from the new device
>
> 1) what does it really mean for hardware virtio block devices?
> 3) Using blk_config means we can only get the benefit from the new device
>
> This patch adds dedicated HW queue for polling purpose to virtio
> block device.
>
> So I think it can be a new hw feature. And it can be a new device
> that supports hw poll queue.

One possible issue is that the "poll" looks more like a
software/driver concept other than the device/hardware.

>
> BTW, I have other idea about it.
>
> How about adding “num-poll-queues" property as a driver parameter
> like NVMe driver, not to QEMU virtio-blk-pci property?

It should be fine, but we need to listen to others.

>
> If then, we don’t need to modify virtio_blk_config.
> And we can apply the polling feature only to virtio-blk-pci.
> But can QEMU pass “num-poll-queues" to virtio-blk driver param?

As Michael said we can leave this to guest kernel / administrator.

>
>
>
> 2) Does driver polling help for the qemu implementation without polling?
>
> Sorry, I didn't understand your question. Could you please explain more about?

I mean does the polling work for the ordinary qemu block device
without busy polling?

Thanks

>
> Regards,
> Suwan Kim
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio-blk: support polling I/O

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 3727 bytes --]

On Wed, Mar 16, 2022 at 05:36:20PM +0200, Max Gurtovoy wrote:
> 
> On 3/16/2022 5:32 PM, Suwan Kim wrote:
> > On Wed, Mar 16, 2022 at 10:02:13AM +0800, Jason Wang wrote:
> > > On Tue, Mar 15, 2022 at 10:43 PM Suwan Kim <suwan.kim027@gmail.com> wrote:
> > > > On Tue, Mar 15, 2022 at 04:59:23PM +0800, Jason Wang wrote:
> > > > > On Mon, Mar 14, 2022 at 8:33 PM Suwan Kim <suwan.kim027@gmail.com> wrote:
> > > > > 
> > > > > > On Mon, Mar 14, 2022 at 02:14:53PM +0800, Jason Wang wrote:
> > > > > > > 在 2022/3/11 下午11:28, Suwan Kim 写道:
> > > > > > > > diff --git a/include/uapi/linux/virtio_blk.h
> > > > > > b/include/uapi/linux/virtio_blk.h
> > > > > > > > index d888f013d9ff..3fcaf937afe1 100644
> > > > > > > > --- a/include/uapi/linux/virtio_blk.h
> > > > > > > > +++ b/include/uapi/linux/virtio_blk.h
> > > > > > > > @@ -119,8 +119,9 @@ struct virtio_blk_config {
> > > > > > > >       * deallocation of one or more of the sectors.
> > > > > > > >       */
> > > > > > > >      __u8 write_zeroes_may_unmap;
> > > > > > > > +   __u8 unused1;
> > > > > > > > -   __u8 unused1[3];
> > > > > > > > +   __virtio16 num_poll_queues;
> > > > > > > >    } __attribute__((packed));
> > > > > > > 
> > > > > > > This looks like a implementation specific (virtio-blk-pci) optimization,
> > > > > > how
> > > > > > > about other implementation like vhost-user-blk?
> > > > > > I didn’t consider vhost-user-blk yet. But does vhost-user-blk also
> > > > > > use vritio_blk_config as kernel-qemu interface?
> > > > > > 
> > > > > Yes, but see below.
> > > > > 
> > > > > 
> > > > > > Does vhost-user-blk need additional modification to support polling
> > > > > > in kernel side?
> > > > > > 
> > > > > 
> > > > > No, but the issue is, things like polling looks not a good candidate for
> > > > > the attributes belonging to the device but the driver. So I have more
> > > > > questions:
> > > > > 
> > > > > 1) what does it really mean for hardware virtio block devices?
> > > > > 2) Does driver polling help for the qemu implementation without polling?
> > > > > 3) Using blk_config means we can only get the benefit from the new device
> > > > 1) what does it really mean for hardware virtio block devices?
> > > > 3) Using blk_config means we can only get the benefit from the new device
> > > > 
> > > > This patch adds dedicated HW queue for polling purpose to virtio
> > > > block device.
> > > > 
> > > > So I think it can be a new hw feature. And it can be a new device
> > > > that supports hw poll queue.
> > > One possible issue is that the "poll" looks more like a
> > > software/driver concept other than the device/hardware.
> > > 
> > > > BTW, I have other idea about it.
> > > > 
> > > > How about adding “num-poll-queues" property as a driver parameter
> > > > like NVMe driver, not to QEMU virtio-blk-pci property?
> > > It should be fine, but we need to listen to others.
> > To Michael, Stefan, Max
> > 
> > How about using driver parameter instead of virio_blk_config?
> 
> Yes. I mentioned that virtio_blk_config shouldn't change.

There are pros and cons to module parameters and configuration space
fields. Both are valid but feel free to drop the configuration space
field.

On note about module parameters: if the guest has a virtio-blk device
with a root file system and another one for benchmarking then it's
unfortunate that the number of poll queues module parameter affects both
devices.

Is there a way to set a module parameter for a specific device instance?
I guess you'd need something else like a sysfs attribute instead but the
implementation is more complex.

I'm fine with a module parameter.

Stefan

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio-blk: support polling I/O

[Jason Wang]

On Wed, Mar 16, 2022 at 11:33 PM Suwan Kim <suwan.kim027@gmail.com> wrote:
>
> On Wed, Mar 16, 2022 at 10:02:13AM +0800, Jason Wang wrote:
> > On Tue, Mar 15, 2022 at 10:43 PM Suwan Kim <suwan.kim027@gmail.com> wrote:
> > >
> > > On Tue, Mar 15, 2022 at 04:59:23PM +0800, Jason Wang wrote:
> > > > On Mon, Mar 14, 2022 at 8:33 PM Suwan Kim <suwan.kim027@gmail.com> wrote:
> > > >
> > > > > On Mon, Mar 14, 2022 at 02:14:53PM +0800, Jason Wang wrote:
> > > > > >
> > > > > > 在 2022/3/11 下午11:28, Suwan Kim 写道:
> > > > > > > diff --git a/include/uapi/linux/virtio_blk.h
> > > > > b/include/uapi/linux/virtio_blk.h
> > > > > > > index d888f013d9ff..3fcaf937afe1 100644
> > > > > > > --- a/include/uapi/linux/virtio_blk.h
> > > > > > > +++ b/include/uapi/linux/virtio_blk.h
> > > > > > > @@ -119,8 +119,9 @@ struct virtio_blk_config {
> > > > > > >      * deallocation of one or more of the sectors.
> > > > > > >      */
> > > > > > >     __u8 write_zeroes_may_unmap;
> > > > > > > +   __u8 unused1;
> > > > > > > -   __u8 unused1[3];
> > > > > > > +   __virtio16 num_poll_queues;
> > > > > > >   } __attribute__((packed));
> > > > > >
> > > > > >
> > > > > > This looks like a implementation specific (virtio-blk-pci) optimization,
> > > > > how
> > > > > > about other implementation like vhost-user-blk?
> > > > >
> > > > > I didn’t consider vhost-user-blk yet. But does vhost-user-blk also
> > > > > use vritio_blk_config as kernel-qemu interface?
> > > > >
> > > >
> > > > Yes, but see below.
> > > >
> > > >
> > > > >
> > > > > Does vhost-user-blk need additional modification to support polling
> > > > > in kernel side?
> > > > >
> > > >
> > > >
> > > > No, but the issue is, things like polling looks not a good candidate for
> > > > the attributes belonging to the device but the driver. So I have more
> > > > questions:
> > > >
> > > > 1) what does it really mean for hardware virtio block devices?
> > > > 2) Does driver polling help for the qemu implementation without polling?
> > > > 3) Using blk_config means we can only get the benefit from the new device
> > >
> > > 1) what does it really mean for hardware virtio block devices?
> > > 3) Using blk_config means we can only get the benefit from the new device
> > >
> > > This patch adds dedicated HW queue for polling purpose to virtio
> > > block device.
> > >
> > > So I think it can be a new hw feature. And it can be a new device
> > > that supports hw poll queue.
> >
> > One possible issue is that the "poll" looks more like a
> > software/driver concept other than the device/hardware.
> >
> > >
> > > BTW, I have other idea about it.
> > >
> > > How about adding “num-poll-queues" property as a driver parameter
> > > like NVMe driver, not to QEMU virtio-blk-pci property?
> >
> > It should be fine, but we need to listen to others.
>
> To Michael, Stefan, Max
>
> How about using driver parameter instead of virio_blk_config?

I agree.

Thanks

>
> Regards,
> Suwan Kim
>

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio-blk: support polling I/O

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 4166 bytes --]

On Sat, Mar 12, 2022 at 12:28:32AM +0900, Suwan Kim wrote:
> This patch supports polling I/O via virtio-blk driver. Polling
> feature is enabled based on "VIRTIO_BLK_F_MQ" feature and the number
> of polling queues can be set by QEMU virtio-blk-pci property
> "num-poll-queues=N". This patch improves the polling I/O throughput
> and latency.
> 
> The virtio-blk driver doesn't not have a poll function and a poll
> queue and it has been operating in interrupt driven method even if
> the polling function is called in the upper layer.
> 
> virtio-blk polling is implemented upon 'batched completion' of block
> layer. virtblk_poll() queues completed request to io_comp_batch->req_list
> and later, virtblk_complete_batch() calls unmap function and ends
> the requests in batch.
> 
> virtio-blk reads the number of queues and poll queues from QEMU
> virtio-blk-pci properties ("num-queues=N", "num-poll-queues=M").
> It allocates N virtqueues to virtio_blk->vqs[N] and it uses [0..(N-M-1)]
> as default queues and [(N-M)..(N-1)] as poll queues. Unlike the default
> queues, the poll queues have no callback function.
> 
> Regarding HW-SW queue mapping, the default queue mapping uses the
> existing method that condsiders MSI irq vector. But the poll queue
> doesn't have an irq, so it uses the regular blk-mq cpu mapping.
> 
> To enable poll queues, "num-poll-queues=N" property of virtio-blk-pci
> needs to be added to QEMU command line. For that, I temporarily
> implemented the property on QEMU. Please refer to the git repository below.
> 
> 	git : https://github.com/asfaca/qemu.git #on master branch commit
> 
> For verifying the improvement, I did Fio polling I/O performance test
> with io_uring engine with the options below.
> (io_uring, hipri, randread, direct=1, bs=512, iodepth=64 numjobs=N)
> I set 4 vcpu and 4 virtio-blk queues - 2 default queues and 2 poll
> queues for VM.
> (-device virtio-blk-pci,num-queues=4,num-poll-queues=2)
> As a result, IOPS and average latency improved about 10%.
> 
> Test result:
> 
> - Fio io_uring poll without virtio-blk poll support
> 	-- numjobs=1 : IOPS = 297K, avg latency = 214.59us
> 	-- numjobs=2 : IOPS = 360K, avg latency = 363.88us
> 	-- numjobs=4 : IOPS = 289K, avg latency = 885.42us
> 
> - Fio io_uring poll with virtio-blk poll support
> 	-- numjobs=1 : IOPS = 332K, avg latency = 192.61us
> 	-- numjobs=2 : IOPS = 371K, avg latency = 348.31us
> 	-- numjobs=4 : IOPS = 321K, avg latency = 795.93us

Last year there was a patch series that switched regular queues into
polling queues when HIPRI requests were in flight:
https://lore.kernel.org/linux-block/20210520141305.355961-1-stefanha@redhat.com/T/

The advantage is that polling is possible without prior device
configuration, making it easier for users.

However, the dynamic approach is a bit more complex and bugs can result
in lost irqs (hung I/O). Christoph Hellwig asked for dedicated polling
queues, which your patch series now delivers.

I think your patch series is worth merging once the comments others have
already made have been addressed. I'll keep an eye out for the VIRTIO
spec change to extend the virtio-blk configuration space, which needs to
be accepted before the Linux can be merged.

> @@ -728,16 +749,82 @@ static const struct attribute_group *virtblk_attr_groups[] = {
>  static int virtblk_map_queues(struct blk_mq_tag_set *set)
>  {
>  	struct virtio_blk *vblk = set->driver_data;
> +	int i, qoff;
> +
> +	for (i = 0, qoff = 0; i < set->nr_maps; i++) {
> +		struct blk_mq_queue_map *map = &set->map[i];
> +
> +		map->nr_queues = vblk->io_queues[i];
> +		map->queue_offset = qoff;
> +		qoff += map->nr_queues;
> +
> +		if (map->nr_queues == 0)
> +			continue;
> +
> +		if (i == HCTX_TYPE_DEFAULT)
> +			blk_mq_virtio_map_queues(&set->map[i], vblk->vdev, 0);
> +		else
> +			blk_mq_map_queues(&set->map[i]);

A comment would be nice here to explain that regular queues have
interrupts and hence CPU affinity is defined by the core virtio code,
but polling queues have no interrupts so we let the block layer assign
CPU affinity.

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization

Re: [PATCH] virtio-blk: support polling I/O

[Stefan Hajnoczi]

[-- Attachment #1.1: Type: text/plain, Size: 4060 bytes --]

On Tue, Mar 15, 2022 at 10:55:04PM +0900, Suwan Kim wrote:
> On Mon, Mar 14, 2022 at 03:19:01PM +0000, Stefan Hajnoczi wrote:
> > On Sat, Mar 12, 2022 at 12:28:32AM +0900, Suwan Kim wrote:
> > > This patch supports polling I/O via virtio-blk driver. Polling
> > > feature is enabled based on "VIRTIO_BLK_F_MQ" feature and the number
> > > of polling queues can be set by QEMU virtio-blk-pci property
> > > "num-poll-queues=N". This patch improves the polling I/O throughput
> > > and latency.
> > > 
> > > The virtio-blk driver doesn't not have a poll function and a poll
> > > queue and it has been operating in interrupt driven method even if
> > > the polling function is called in the upper layer.
> > > 
> > > virtio-blk polling is implemented upon 'batched completion' of block
> > > layer. virtblk_poll() queues completed request to io_comp_batch->req_list
> > > and later, virtblk_complete_batch() calls unmap function and ends
> > > the requests in batch.
> > > 
> > > virtio-blk reads the number of queues and poll queues from QEMU
> > > virtio-blk-pci properties ("num-queues=N", "num-poll-queues=M").
> > > It allocates N virtqueues to virtio_blk->vqs[N] and it uses [0..(N-M-1)]
> > > as default queues and [(N-M)..(N-1)] as poll queues. Unlike the default
> > > queues, the poll queues have no callback function.
> > > 
> > > Regarding HW-SW queue mapping, the default queue mapping uses the
> > > existing method that condsiders MSI irq vector. But the poll queue
> > > doesn't have an irq, so it uses the regular blk-mq cpu mapping.
> > > 
> > > To enable poll queues, "num-poll-queues=N" property of virtio-blk-pci
> > > needs to be added to QEMU command line. For that, I temporarily
> > > implemented the property on QEMU. Please refer to the git repository below.
> > > 
> > > 	git : https://github.com/asfaca/qemu.git #on master branch commit
> > > 
> > > For verifying the improvement, I did Fio polling I/O performance test
> > > with io_uring engine with the options below.
> > > (io_uring, hipri, randread, direct=1, bs=512, iodepth=64 numjobs=N)
> > > I set 4 vcpu and 4 virtio-blk queues - 2 default queues and 2 poll
> > > queues for VM.
> > > (-device virtio-blk-pci,num-queues=4,num-poll-queues=2)
> > > As a result, IOPS and average latency improved about 10%.
> > > 
> > > Test result:
> > > 
> > > - Fio io_uring poll without virtio-blk poll support
> > > 	-- numjobs=1 : IOPS = 297K, avg latency = 214.59us
> > > 	-- numjobs=2 : IOPS = 360K, avg latency = 363.88us
> > > 	-- numjobs=4 : IOPS = 289K, avg latency = 885.42us
> > > 
> > > - Fio io_uring poll with virtio-blk poll support
> > > 	-- numjobs=1 : IOPS = 332K, avg latency = 192.61us
> > > 	-- numjobs=2 : IOPS = 371K, avg latency = 348.31us
> > > 	-- numjobs=4 : IOPS = 321K, avg latency = 795.93us
> > 
> > Last year there was a patch series that switched regular queues into
> > polling queues when HIPRI requests were in flight:
> > https://lore.kernel.org/linux-block/20210520141305.355961-1-stefanha@redhat.com/T/
> > 
> > The advantage is that polling is possible without prior device
> > configuration, making it easier for users.
> > 
> > However, the dynamic approach is a bit more complex and bugs can result
> > in lost irqs (hung I/O). Christoph Hellwig asked for dedicated polling
> > queues, which your patch series now delivers.
> > 
> > I think your patch series is worth merging once the comments others have
> > already made have been addressed. I'll keep an eye out for the VIRTIO
> > spec change to extend the virtio-blk configuration space, which needs to
> > be accepted before the Linux can be merged.
> 
> Thanks for the feedback :)
> There's a lot of history.. I will try to improve the patch.
> 
> It might take some time because it need more discussion about qemu
> device property and I do this in my night time.

I see, it's great that you're making this contribution. Don't worry
about the old patch series I linked. I think your approach is fine.

Stefan

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[-- Attachment #2: Type: text/plain, Size: 183 bytes --]

_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization