Patchwork virtio_net: zero is an invald queue_pairs number

login
register
mail settings
Submitter Amos Kong
Date April 18, 2014, 5:45 a.m.
Message ID <1397799941-11371-1-git-send-email-akong@redhat.com>
Download mbox | patch
Permalink /patch/340216/
State Accepted
Delegated to: David Miller
Headers show

Comments

Amos Kong - April 18, 2014, 5:45 a.m.
Execute "ethtool -L eth0 combined 0" in guest, if multiqueue
is enabled, virtnet_send_command() will return -EINVAL error,
there is a validation in QEMU.

But if multiqueue is disabled, virtnet_set_queues() will just
return zero (success). We should return error for this situation.

Signed-off-by: Amos Kong <akong@redhat.com>
---
 drivers/net/virtio_net.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Jason Wang - April 21, 2014, 6:32 a.m.
On 04/18/2014 01:45 PM, Amos Kong wrote:
> Execute "ethtool -L eth0 combined 0" in guest, if multiqueue
> is enabled, virtnet_send_command() will return -EINVAL error,
> there is a validation in QEMU.
>
> But if multiqueue is disabled, virtnet_set_queues() will just
> return zero (success). We should return error for this situation.
>
> Signed-off-by: Amos Kong <akong@redhat.com>
> ---
>  drivers/net/virtio_net.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 7b68746..8a852b5 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1285,7 +1285,7 @@ static int virtnet_set_channels(struct net_device *dev,
>  	if (channels->rx_count || channels->tx_count || channels->other_count)
>  		return -EINVAL;
>  
> -	if (queue_pairs > vi->max_queue_pairs)
> +	if (queue_pairs > vi->max_queue_pairs || queue_pairs == 0)
>  		return -EINVAL;
>  
>  	get_online_cpus();

Acked-by:  Jason Wang <jasowang@redhat.com>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jason Wang - April 21, 2014, 6:45 a.m.
On 04/21/2014 02:32 PM, Jason Wang wrote:
> On 04/18/2014 01:45 PM, Amos Kong wrote:
>> Execute "ethtool -L eth0 combined 0" in guest, if multiqueue
>> is enabled, virtnet_send_command() will return -EINVAL error,
>> there is a validation in QEMU.
>>
>> But if multiqueue is disabled, virtnet_set_queues() will just
>> return zero (success). We should return error for this situation.
>>
>> Signed-off-by: Amos Kong <akong@redhat.com>
>> ---
>>  drivers/net/virtio_net.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 7b68746..8a852b5 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -1285,7 +1285,7 @@ static int virtnet_set_channels(struct net_device *dev,
>>  	if (channels->rx_count || channels->tx_count || channels->other_count)
>>  		return -EINVAL;
>>  
>> -	if (queue_pairs > vi->max_queue_pairs)
>> +	if (queue_pairs > vi->max_queue_pairs || queue_pairs == 0)
>>  		return -EINVAL;
>>  
>>  	get_online_cpus();
> Acked-by:  Jason Wang <jasowang@redhat.com>

Looking at virtnet_set_queues(), it will return success even if no cvq
and no mq support. This is wrong. We need return error in this case.
We'd better fix it and check the its return value in virtnet_restore().

With this fix, there's no need for us to check queue_pairs against zero
like what this patch does.


--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Amos Kong - April 21, 2014, 3:03 p.m.
On Mon, Apr 21, 2014 at 02:45:25PM +0800, Jason Wang wrote:
> On 04/21/2014 02:32 PM, Jason Wang wrote:
> > On 04/18/2014 01:45 PM, Amos Kong wrote:
> >> Execute "ethtool -L eth0 combined 0" in guest, if multiqueue
> >> is enabled, virtnet_send_command() will return -EINVAL error,
> >> there is a validation in QEMU.
> >>
> >> But if multiqueue is disabled, virtnet_set_queues() will just
> >> return zero (success). We should return error for this situation.
> >>
> >> Signed-off-by: Amos Kong <akong@redhat.com>
> >> ---
> >>  drivers/net/virtio_net.c | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >> index 7b68746..8a852b5 100644
> >> --- a/drivers/net/virtio_net.c
> >> +++ b/drivers/net/virtio_net.c
> >> @@ -1285,7 +1285,7 @@ static int virtnet_set_channels(struct net_device *dev,
> >>  	if (channels->rx_count || channels->tx_count || channels->other_count)
> >>  		return -EINVAL;
> >>  
> >> -	if (queue_pairs > vi->max_queue_pairs)
> >> +	if (queue_pairs > vi->max_queue_pairs || queue_pairs == 0)
> >>  		return -EINVAL;
> >>  
> >>  	get_online_cpus();
> > Acked-by:  Jason Wang <jasowang@redhat.com>
> 
> Looking at virtnet_set_queues(), it will return success even if no cvq
> and no mq support. This is wrong. We need return error in this case.
> We'd better fix it and check the its return value in virtnet_restore().
> 
> With this fix, there's no need for us to check queue_pairs against zero
> like what this patch does.
 
As we talked in IRC, the problem I tried to fix is actually caused by
that we don't return error when MQ isn't support. I will post another
patch to fix it.

However, this fix is also useful, it will catch the error early.
David Miller - April 22, 2014, 8:01 p.m.
From: Amos Kong <akong@redhat.com>
Date: Fri, 18 Apr 2014 13:45:41 +0800

> Execute "ethtool -L eth0 combined 0" in guest, if multiqueue
> is enabled, virtnet_send_command() will return -EINVAL error,
> there is a validation in QEMU.
> 
> But if multiqueue is disabled, virtnet_set_queues() will just
> return zero (success). We should return error for this situation.
> 
> Signed-off-by: Amos Kong <akong@redhat.com>

Applied, thanks.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 7b68746..8a852b5 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1285,7 +1285,7 @@  static int virtnet_set_channels(struct net_device *dev,
 	if (channels->rx_count || channels->tx_count || channels->other_count)
 		return -EINVAL;
 
-	if (queue_pairs > vi->max_queue_pairs)
+	if (queue_pairs > vi->max_queue_pairs || queue_pairs == 0)
 		return -EINVAL;
 
 	get_online_cpus();