Patchwork [rfc,net-next,v6,3/3] virtio-net: change the number of queues through ethtool

login
register
mail settings
Submitter Jason Wang
Date Oct. 30, 2012, 10:03 a.m.
Message ID <1351591403-23065-4-git-send-email-jasowang@redhat.com>
Download mbox | patch
Permalink /patch/195387/
State Not Applicable
Delegated to: David Miller
Headers show

Comments

Jason Wang - Oct. 30, 2012, 10:03 a.m.
This patch implement the {set|get}_channels method of ethool to allow user to
change the number of queues dymaically when the device is running. This would
let the user to tune the device for specific applications.

Signed-off-by: Jason Wang <jasowang@redhat.com>
---
 drivers/net/virtio_net.c |   43 +++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 43 insertions(+), 0 deletions(-)
Rusty Russell - Nov. 4, 2012, 11:46 p.m.
Jason Wang <jasowang@redhat.com> writes:
> This patch implement the {set|get}_channels method of ethool to allow user to
> change the number of queues dymaically when the device is running. This would
> let the user to tune the device for specific applications.
...
> +	/* Only two modes were support currently */
> +	if (queue_pairs == 0)
> +		return -EINVAL;
> +	if (queue_pairs != vi->total_queue_pairs - 1 && queue_pairs != 1)
> +		return -EINVAL;

OK, so you let them do all or nothing, but this three-way test is
pretty unclear.

In fact, the whole total_queue_pairs/num_queue_pairs thing is weird (and
uncommented).  I think for "total" you mean "max"; the maximum possible
queue pair number.

Let me go back and review the previous patch again...

Cheers,
Rusty.
--
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
Ben Hutchings - Nov. 8, 2012, 9:13 p.m.
On Tue, 2012-10-30 at 18:03 +0800, Jason Wang wrote:
> This patch implement the {set|get}_channels method of ethool to allow user to
> change the number of queues dymaically when the device is running. This would
> let the user to tune the device for specific applications.
> 
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
>  drivers/net/virtio_net.c |   43 +++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 43 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 8cc43e5..66fc129 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -1616,10 +1616,53 @@ static struct virtio_driver virtio_net_driver = {
>  #endif
>  };
>  
> +/* TODO: Eliminate OOO packets during switching */
> +static int virtnet_set_channels(struct net_device *dev,
> +				struct ethtool_channels *channels)
> +{
> +	struct virtnet_info *vi = netdev_priv(dev);
> +	u16 queue_pairs = channels->combined_count;
> +
> +	/* Only two modes were support currently */
> +	if (queue_pairs == 0)
> +		return -EINVAL;
> +	if (queue_pairs != vi->total_queue_pairs - 1 && queue_pairs != 1)
> +		return -EINVAL;

You should also check that all of the other counts are == 0.

> +	vi->num_queue_pairs = queue_pairs > 1 ? queue_pairs + 1 : 1;

This doesn't make sense.  You're setting vi->num_queue_pairs to be
combined_count + 1...

> +	BUG_ON(virtnet_set_queues(vi));
> +
> +	netif_set_real_num_tx_queues(dev, vi->num_queue_pairs);
> +	netif_set_real_num_rx_queues(dev, vi->num_queue_pairs);
> +
> +	return 0;
> +}
> +
> +static void virtnet_get_channels(struct net_device *dev,
> +				 struct ethtool_channels *channels)
> +{
> +	struct virtnet_info *vi = netdev_priv(dev);
> +
> +	if (vi->total_queue_pairs != 1) {
> +		channels->combined_count = vi->num_queue_pairs;

but here you report combined_count as being vi->num_queue_pairs.  Do you
need to subtract 1 here?

Ben.

> +		channels->max_combined = vi->total_queue_pairs - 1;
> +	} else {
> +		channels->combined_count = 1;
> +		channels->max_combined = 1;
> +	}
> +
> +	channels->max_other = 0;
> +	channels->rx_count = 0;
> +	channels->tx_count = 0;
> +	channels->other_count = 0;
> +}
> +
>  static const struct ethtool_ops virtnet_ethtool_ops = {
>  	.get_drvinfo = virtnet_get_drvinfo,
>  	.get_link = ethtool_op_get_link,
>  	.get_ringparam = virtnet_get_ringparam,
> +	.set_channels = virtnet_set_channels,
> +	.get_channels = virtnet_get_channels,
>  };
>  
>  static int __init init(void)
Jason Wang - Nov. 19, 2012, 6:22 a.m.
On 11/05/2012 07:46 AM, Rusty Russell wrote:
> Jason Wang <jasowang@redhat.com> writes:
>> This patch implement the {set|get}_channels method of ethool to allow user to
>> change the number of queues dymaically when the device is running. This would
>> let the user to tune the device for specific applications.
> ...
>> +	/* Only two modes were support currently */
>> +	if (queue_pairs == 0)
>> +		return -EINVAL;
>> +	if (queue_pairs != vi->total_queue_pairs - 1 && queue_pairs != 1)
>> +		return -EINVAL;
> OK, so you let them do all or nothing, but this three-way test is
> pretty unclear.

True, looks like the first check could be removed.
>
> In fact, the whole total_queue_pairs/num_queue_pairs thing is weird (and
> uncommented).  I think for "total" you mean "max"; the maximum possible
> queue pair number.

Yes, "total" means "max", will add a comment or change the name to 
max_queue_pairs/current_queue_pairs.
>
> Let me go back and review the previous patch again...
>
> Cheers,
> Rusty.
> --
> 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/

--
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 8cc43e5..66fc129 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1616,10 +1616,53 @@  static struct virtio_driver virtio_net_driver = {
 #endif
 };
 
+/* TODO: Eliminate OOO packets during switching */
+static int virtnet_set_channels(struct net_device *dev,
+				struct ethtool_channels *channels)
+{
+	struct virtnet_info *vi = netdev_priv(dev);
+	u16 queue_pairs = channels->combined_count;
+
+	/* Only two modes were support currently */
+	if (queue_pairs == 0)
+		return -EINVAL;
+	if (queue_pairs != vi->total_queue_pairs - 1 && queue_pairs != 1)
+		return -EINVAL;
+
+	vi->num_queue_pairs = queue_pairs > 1 ? queue_pairs + 1 : 1;
+	BUG_ON(virtnet_set_queues(vi));
+
+	netif_set_real_num_tx_queues(dev, vi->num_queue_pairs);
+	netif_set_real_num_rx_queues(dev, vi->num_queue_pairs);
+
+	return 0;
+}
+
+static void virtnet_get_channels(struct net_device *dev,
+				 struct ethtool_channels *channels)
+{
+	struct virtnet_info *vi = netdev_priv(dev);
+
+	if (vi->total_queue_pairs != 1) {
+		channels->combined_count = vi->num_queue_pairs;
+		channels->max_combined = vi->total_queue_pairs - 1;
+	} else {
+		channels->combined_count = 1;
+		channels->max_combined = 1;
+	}
+
+	channels->max_other = 0;
+	channels->rx_count = 0;
+	channels->tx_count = 0;
+	channels->other_count = 0;
+}
+
 static const struct ethtool_ops virtnet_ethtool_ops = {
 	.get_drvinfo = virtnet_get_drvinfo,
 	.get_link = ethtool_op_get_link,
 	.get_ringparam = virtnet_get_ringparam,
+	.set_channels = virtnet_set_channels,
+	.get_channels = virtnet_get_channels,
 };
 
 static int __init init(void)