Re: [PATCH] Do not name control queue for virtio-net

Re: [PATCH] Do not name control queue for virtio-net

[Michael S. Tsirkin]

On Sat, Sep 17, 2022 at 09:28:57AM +0000, junbo4242@gmail.com wrote:
> From: Junbo <junbo4242@gmail.com>
> 
> In virtio drivers, the control queue always named <virtioX>-config.
> 
> Signed-off-by: Junbo <junbo4242@gmail.com>

I don't think that's right. config is the config interrupt.



> ---
>  drivers/net/virtio_net.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 9cce7dec7366..0b3e74cfe201 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -3469,7 +3469,8 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>  	/* Parameters for control virtqueue, if any */
>  	if (vi->has_cvq) {
>  		callbacks[total_vqs - 1] = NULL;
> -		names[total_vqs - 1] = "control";
> +		/* control virtqueue always named <virtioX>-config */
> +		names[total_vqs - 1] = "";
>  	}
>  
>  	/* Allocate/initialize parameters for send/receive virtqueues */
> -- 
> 2.31.1

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

Re: [PATCH] Do not name control queue for virtio-net

[Michael S. Tsirkin]

On Sun, Sep 18, 2022 at 05:00:20PM +0800, Junbo wrote:
> hi Michael
> 
> in virtio-net.c
>     /* Parameters for control virtqueue, if any */
>     if (vi->has_cvq) {
>         callbacks[total_vqs - 1] = NULL;
>         names[total_vqs - 1] = "control";
>     }
> 
> I think the Author who write the code

wait, that was not you?

> maybe want to name the control queue to
> 'virtioX-control', but it never worked, we can see the name still be
> 'virtioX-config' in /proc/interrupts, for example 
>  43:          0          0          0          0          0          0        
>  0          0   PCI-MSI-edge      virtio0-config
>  44:         64          0          0          0          0          0      
> 1845          0   PCI-MSI-edge      virtio0-input.0
>  45:          1          0          0          0          0          0        
>  0          0   PCI-MSI-edge      virtio0-output.0
> 
> Because in function vp_request_msix_vectors, it just allocate 'xxxx-config' to
> every virtio devices, even the virtio device do not need it. in /proc/
> interrupts, we can see that each virtio device's first interrupt always named
> 'virtioX-config'.
> 
> So I think it's better to not explicitly give the "control" here, it's
> useless...  
> 
> 
> Michael S. Tsirkin <mst@redhat.com> 于2022年9月18日周日 14:56写道：
> 
>     On Sat, Sep 17, 2022 at 09:28:57AM +0000, junbo4242@gmail.com wrote:
>     > From: Junbo <junbo4242@gmail.com>
>     >
>     > In virtio drivers, the control queue always named <virtioX>-config.
>     >
>     > Signed-off-by: Junbo <junbo4242@gmail.com>
> 
>     I don't think that's right. config is the config interrupt.
> 
> 
> 
>     > ---
>     >  drivers/net/virtio_net.c | 3 ++-
>     >  1 file changed, 2 insertions(+), 1 deletion(-)
>     >
>     > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>     > index 9cce7dec7366..0b3e74cfe201 100644
>     > --- a/drivers/net/virtio_net.c
>     > +++ b/drivers/net/virtio_net.c
>     > @@ -3469,7 +3469,8 @@ static int virtnet_find_vqs(struct virtnet_info
>     *vi)
>     >       /* Parameters for control virtqueue, if any */
>     >       if (vi->has_cvq) {
>     >               callbacks[total_vqs - 1] = NULL;
>     > -             names[total_vqs - 1] = "control";
>     > +             /* control virtqueue always named <virtioX>-config */
>     > +             names[total_vqs - 1] = "";
>     >       }
>     > 
>     >       /* Allocate/initialize parameters for send/receive virtqueues */
>     > --
>     > 2.31.1
> 
> 

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

Re: [PATCH] Do not name control queue for virtio-net

[Paolo Abeni]

On Sun, 2022-09-18 at 08:17 -0400, Michael S. Tsirkin wrote:
> On Sun, Sep 18, 2022 at 05:00:20PM +0800, Junbo wrote:
> > hi Michael
> > 
> > in virtio-net.c
> >     /* Parameters for control virtqueue, if any */
> >     if (vi->has_cvq) {
> >         callbacks[total_vqs - 1] = NULL;
> >         names[total_vqs - 1] = "control";
> >     }
> > 
> > I think the Author who write the code
> 
> wait, that was not you?

I believe 'the Author' refers to the author of the current code, not to
the author of the patch.

@Junbo: the control queue is created only if the VIRTIO_NET_F_CTRL_VQ
feature is set, please check that in your setup.

Thanks

Paolo


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

Re: [PATCH] Do not name control queue for virtio-net

[Michael S. Tsirkin]

On Thu, Sep 22, 2022 at 11:10:37AM +0200, Paolo Abeni wrote:
> On Sun, 2022-09-18 at 08:17 -0400, Michael S. Tsirkin wrote:
> > On Sun, Sep 18, 2022 at 05:00:20PM +0800, Junbo wrote:
> > > hi Michael
> > > 
> > > in virtio-net.c
> > >     /* Parameters for control virtqueue, if any */
> > >     if (vi->has_cvq) {
> > >         callbacks[total_vqs - 1] = NULL;
> > >         names[total_vqs - 1] = "control";
> > >     }
> > > 
> > > I think the Author who write the code
> > 
> > wait, that was not you?
> 
> I believe 'the Author' refers to the author of the current code, not to
> the author of the patch.

Oh I see. Responded.

> @Junbo: the control queue is created only if the VIRTIO_NET_F_CTRL_VQ
> feature is set, please check that in your setup.
> 
> Thanks
> 
> Paolo


-- 
MST

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

Re: [PATCH] Do not name control queue for virtio-net

[Michael S. Tsirkin]

On Sun, Sep 18, 2022 at 05:00:20PM +0800, Junbo wrote:
> hi Michael
> 
> in virtio-net.c
>     /* Parameters for control virtqueue, if any */
>     if (vi->has_cvq) {
>         callbacks[total_vqs - 1] = NULL;
>         names[total_vqs - 1] = "control";
>     }
> 
> I think the Author who write the code maybe want to name the control queue to
> 'virtioX-control',

That would be me I suspect ;)

> but it never worked, we can see the name still be
> 'virtioX-config' in /proc/interrupts,

Nope, what you see in /proc/interrupts are the interrupts, not the queue
name.

> for example 
>  43:          0          0          0          0          0          0        
>  0          0   PCI-MSI-edge      virtio0-config
>  44:         64          0          0          0          0          0      
> 1845          0   PCI-MSI-edge      virtio0-input.0
>  45:          1          0          0          0          0          0        
>  0          0   PCI-MSI-edge      virtio0-output.0
> 
> Because in function vp_request_msix_vectors, it just allocate 'xxxx-config' to
> every virtio devices, even the virtio device do not need it.

Oh yes, we can fix that. The result will be this line disappearing for
devices without a config interrupt. Not for net though, that
generally uses a config interrupt for things like link
state detection.


> in /proc/
> interrupts, we can see that each virtio device's first interrupt always named
> 'virtioX-config'.
> 
> So I think it's better to not explicitly give the "control" here, it's
> useless...  


it's used for debugging.


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