[PATCH net-next-2.6 0/2] net, sfc: Fix number of RX queues

[PATCH net-next-2.6 0/2] net, sfc: Fix number of RX queues

[Ben Hutchings]

This distinguishes the maximum and actual numbers of RX queues, adds a
function to set the actual number, and changes sfc to use it.  Other
multiqueue drivers should presumably be updated similarly.

Ben.

Ben Hutchings (2):
  net: Allow changing number of RX queues after device allocation
  sfc: Use proper functions to set core RX and TX queue counts

 drivers/net/sfc/efx.c     |    3 ++-
 include/linux/netdevice.h |   14 ++++++++++++++
 net/core/dev.c            |   43 +++++++++++++++++++++++++++++++++++++++----
 net/core/net-sysfs.c      |   34 +++++++++++++++++++---------------
 net/core/net-sysfs.h      |    4 ++++
 5 files changed, 78 insertions(+), 20 deletions(-)

-- 
1.7.2.1


-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

[PATCH net-next-2.6 1/2] net: Allow changing number of RX queues after device allocation

[Ben Hutchings]

For RPS, we create a kobject for each RX queue based on the number of
queues passed to alloc_netdev_mq().  However, drivers generally do not
determine the numbers of hardware queues to use until much later, so
this usually represents the maximum number the driver may use and not
the actual number in use.

For TX queues, drivers can update the actual number using
netif_set_real_num_tx_queues().  Add a corresponding function for RX
queues, netif_set_real_num_rx_queues().

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 include/linux/netdevice.h |   14 ++++++++++++++
 net/core/dev.c            |   43 +++++++++++++++++++++++++++++++++++++++----
 net/core/net-sysfs.c      |   34 +++++++++++++++++++---------------
 net/core/net-sysfs.h      |    4 ++++
 4 files changed, 76 insertions(+), 19 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index f7f1302..c2b3880 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -978,6 +978,9 @@ struct net_device {
 
 	/* Number of RX queues allocated at alloc_netdev_mq() time  */
 	unsigned int		num_rx_queues;
+
+	/* Number of RX queues currently active in device  */
+	unsigned int		real_num_rx_queues;
 #endif
 
 	rx_handler_func_t	*rx_handler;
@@ -1682,6 +1685,17 @@ static inline int netif_is_multiqueue(const struct net_device *dev)
 extern void netif_set_real_num_tx_queues(struct net_device *dev,
 					 unsigned int txq);
 
+#ifdef CONFIG_RPS
+extern int netif_set_real_num_rx_queues(struct net_device *dev,
+					unsigned int rxq);
+#else
+static inline int netif_set_real_num_rx_queues(struct net_device *dev,
+						unsigned int rxq)
+{
+	return 0;
+}
+#endif
+
 /* Use this variant when it is known for sure that it
  * is executing from hardware interrupt context or with hardware interrupts
  * disabled.
diff --git a/net/core/dev.c b/net/core/dev.c
index 2c7934f..1287ce1 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1567,6 +1567,38 @@ void netif_set_real_num_tx_queues(struct net_device *dev, unsigned int txq)
 }
 EXPORT_SYMBOL(netif_set_real_num_tx_queues);
 
+#ifdef CONFIG_RPS
+/**
+ *	netif_set_real_num_rx_queues - set actual number of RX queues used
+ *	@dev: Network device
+ *	@rxq: Actual number of RX queues.  Must not be greater than the
+ *		queue count specified at device allocation time.
+ *
+ * This must be called either with the rtnl_lock held or before registration
+ * of the net device.  Returns 0 on success, or a negative error code.
+ * If called before registration, it always succeeds.
+ */
+int netif_set_real_num_rx_queues(struct net_device *dev, unsigned int rxq)
+{
+	int rc;
+
+	if (rxq > dev->num_rx_queues)
+		return -EINVAL;
+
+	if (dev->reg_state == NETREG_REGISTERED) {
+		ASSERT_RTNL();
+		rc = net_rx_queue_update_kobjects(dev, dev->real_num_rx_queues,
+						  rxq);
+		if (rc)
+			return rc;
+	}
+
+	dev->real_num_rx_queues = rxq;
+	return 0;
+}
+EXPORT_SYMBOL(netif_set_real_num_rx_queues);
+#endif
+
 static inline void __netif_reschedule(struct Qdisc *q)
 {
 	struct softnet_data *sd;
@@ -2352,10 +2384,11 @@ static int get_rps_cpu(struct net_device *dev, struct sk_buff *skb,
 
 	if (skb_rx_queue_recorded(skb)) {
 		u16 index = skb_get_rx_queue(skb);
-		if (unlikely(index >= dev->num_rx_queues)) {
-			WARN_ONCE(dev->num_rx_queues > 1, "%s received packet "
-				"on queue %u, but number of RX queues is %u\n",
-				dev->name, index, dev->num_rx_queues);
+		if (unlikely(index >= dev->real_num_rx_queues)) {
+			WARN_ONCE(dev->real_num_rx_queues > 1,
+				  "%s received packet on queue %u, but number "
+				  "of RX queues is %u\n",
+				  dev->name, index, dev->real_num_rx_queues);
 			goto done;
 		}
 		rxqueue = dev->_rx + index;
@@ -5017,6 +5050,7 @@ int register_netdevice(struct net_device *dev)
 		dev->_rx->first = dev->_rx;
 		atomic_set(&dev->_rx->count, 1);
 		dev->num_rx_queues = 1;
+		dev->real_num_rx_queues = 1;
 	}
 #endif
 	/* Init, if this function is available */
@@ -5479,6 +5513,7 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
 #ifdef CONFIG_RPS
 	dev->_rx = rx;
 	dev->num_rx_queues = queue_count;
+	dev->real_num_rx_queues = queue_count;
 #endif
 
 	dev->gso_max_size = GSO_MAX_SIZE;
diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c
index 76485a3..4791cfc 100644
--- a/net/core/net-sysfs.c
+++ b/net/core/net-sysfs.c
@@ -742,34 +742,38 @@ static int rx_queue_add_kobject(struct net_device *net, int index)
 	return error;
 }
 
-static int rx_queue_register_kobjects(struct net_device *net)
+int
+net_rx_queue_update_kobjects(struct net_device *net, int old_num, int new_num)
 {
 	int i;
 	int error = 0;
-
-	net->queues_kset = kset_create_and_add("queues",
-	    NULL, &net->dev.kobj);
-	if (!net->queues_kset)
-		return -ENOMEM;
-	for (i = 0; i < net->num_rx_queues; i++) {
+	
+	for (i = old_num; i < new_num; i++) {
 		error = rx_queue_add_kobject(net, i);
-		if (error)
+		if (error) {
+			new_num = old_num;
 			break;
+		}
 	}
 
-	if (error)
-		while (--i >= 0)
-			kobject_put(&net->_rx[i].kobj);
+	while (--i >= new_num)
+		kobject_put(&net->_rx[i].kobj);
 
 	return error;
 }
 
-static void rx_queue_remove_kobjects(struct net_device *net)
+static int rx_queue_register_kobjects(struct net_device *net)
 {
-	int i;
+	net->queues_kset = kset_create_and_add("queues",
+	    NULL, &net->dev.kobj);
+	if (!net->queues_kset)
+		return -ENOMEM;
+	return net_rx_queue_update_kobjects(net, 0, net->real_num_rx_queues);
+}
 
-	for (i = 0; i < net->num_rx_queues; i++)
-		kobject_put(&net->_rx[i].kobj);
+static void rx_queue_remove_kobjects(struct net_device *net)
+{
+	net_rx_queue_update_kobjects(net, net->real_num_rx_queues, 0);
 	kset_unregister(net->queues_kset);
 }
 #endif /* CONFIG_RPS */
diff --git a/net/core/net-sysfs.h b/net/core/net-sysfs.h
index 805555e..778e157 100644
--- a/net/core/net-sysfs.h
+++ b/net/core/net-sysfs.h
@@ -4,4 +4,8 @@
 int netdev_kobject_init(void);
 int netdev_register_kobject(struct net_device *);
 void netdev_unregister_kobject(struct net_device *);
+#ifdef CONFIG_RPS
+int net_rx_queue_update_kobjects(struct net_device *, int old_num, int new_num);
+#endif
+
 #endif
-- 
1.7.2.1



-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next-2.6 1/2] net: Allow changing number of RX queues after device allocation

[Eric Dumazet]

Le jeudi 23 septembre 2010 à 21:19 +0100, Ben Hutchings a écrit :
> For RPS, we create a kobject for each RX queue based on the number of
> queues passed to alloc_netdev_mq().  However, drivers generally do not
> determine the numbers of hardware queues to use until much later, so
> this usually represents the maximum number the driver may use and not
> the actual number in use.
> 
> For TX queues, drivers can update the actual number using
> netif_set_real_num_tx_queues().  Add a corresponding function for RX
> queues, netif_set_real_num_rx_queues().
> 
> Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> ---

Seems very good to me, except a minor style issue here :

All functions in this file use a single line.

> -static int rx_queue_register_kobjects(struct net_device *net)
> +int
> +net_rx_queue_update_kobjects(struct net_device *net, int old_num, int new_num)

->

int net_rx_queue_update_kobjects(struct net_device *net, int old, int new)


Also, I dont understand why we need to restrict
netif_set_real_num_rx_queues() to lower the count.
This wastes memory.

Why dont we allocate dev->_rx once we know the real count, not in
alloc_netdev_mq() but in register_netdevice() ?

[PATCH net-next-2.6] rps: allocate rx queues in register_netdevice only

[Eric Dumazet]

> Also, I dont understand why we need to restrict
> netif_set_real_num_rx_queues() to lower the count.
> This wastes memory.
> 
> Why dont we allocate dev->_rx once we know the real count, not in
> alloc_netdev_mq() but in register_netdevice() ?
> 
> 

Here is a patch to make this possible

I guess a similar thing could be done for tx queues.

boot tested, but please double check.

Thanks

[PATCH net-next-2.6] rps: allocate rx queues in register_netdevice()

Instead of having two places were we allocate dev->_rx, introduce
netif_alloc_rx_queues() helper and call it only from
register_netdevice(), not from alloc_netdev_mq()

Goal is to let drivers change dev->num_rx_queues after allocating netdev
and before registering it.

This also removes a lot of ifdefs in net/core/dev.c

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 net/core/dev.c |   76 +++++++++++++++++++----------------------------
 1 file changed, 32 insertions(+), 44 deletions(-)

diff --git a/net/core/dev.c b/net/core/dev.c
index 2c7934f..9110b8d 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -4964,6 +4964,34 @@ void netif_stacked_transfer_operstate(const struct net_device *rootdev,
 }
 EXPORT_SYMBOL(netif_stacked_transfer_operstate);
 
+static int netif_alloc_rx_queues(struct net_device *dev)
+{
+#ifdef CONFIG_RPS
+	unsigned int i, count = dev->num_rx_queues;
+
+	if (count) {
+		struct netdev_rx_queue *rx;
+
+		rx = kcalloc(count, sizeof(struct netdev_rx_queue), GFP_KERNEL);
+		if (!rx) {
+			pr_err("netdev: Unable to allocate %u rx queues.\n",
+			       count);
+			return -ENOMEM;
+		}
+		dev->_rx = rx;
+		atomic_set(&rx->count, count);
+
+		/*
+		 * Set a pointer to first element in the array which holds the
+		 * reference count.
+		 */
+		for (i = 0; i < count; i++)
+			rx[i].first = rx;
+	}
+#endif
+	return 0;
+}
+
 /**
  *	register_netdevice	- register a network device
  *	@dev: device to register
@@ -5001,24 +5029,10 @@ int register_netdevice(struct net_device *dev)
 
 	dev->iflink = -1;
 
-#ifdef CONFIG_RPS
-	if (!dev->num_rx_queues) {
-		/*
-		 * Allocate a single RX queue if driver never called
-		 * alloc_netdev_mq
-		 */
-
-		dev->_rx = kzalloc(sizeof(struct netdev_rx_queue), GFP_KERNEL);
-		if (!dev->_rx) {
-			ret = -ENOMEM;
-			goto out;
-		}
+	ret = netif_alloc_rx_queues(dev);
+	if (ret)
+		goto out;
 
-		dev->_rx->first = dev->_rx;
-		atomic_set(&dev->_rx->count, 1);
-		dev->num_rx_queues = 1;
-	}
-#endif
 	/* Init, if this function is available */
 	if (dev->netdev_ops->ndo_init) {
 		ret = dev->netdev_ops->ndo_init(dev);
@@ -5414,10 +5428,6 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
 	struct net_device *dev;
 	size_t alloc_size;
 	struct net_device *p;
-#ifdef CONFIG_RPS
-	struct netdev_rx_queue *rx;
-	int i;
-#endif
 
 	BUG_ON(strlen(name) >= sizeof(dev->name));
 
@@ -5443,29 +5453,12 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
 		goto free_p;
 	}
 
-#ifdef CONFIG_RPS
-	rx = kcalloc(queue_count, sizeof(struct netdev_rx_queue), GFP_KERNEL);
-	if (!rx) {
-		printk(KERN_ERR "alloc_netdev: Unable to allocate "
-		       "rx queues.\n");
-		goto free_tx;
-	}
-
-	atomic_set(&rx->count, queue_count);
-
-	/*
-	 * Set a pointer to first element in the array which holds the
-	 * reference count.
-	 */
-	for (i = 0; i < queue_count; i++)
-		rx[i].first = rx;
-#endif
 
 	dev = PTR_ALIGN(p, NETDEV_ALIGN);
 	dev->padded = (char *)dev - (char *)p;
 
 	if (dev_addr_init(dev))
-		goto free_rx;
+		goto free_tx;
 
 	dev_mc_init(dev);
 	dev_uc_init(dev);
@@ -5477,7 +5470,6 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
 	dev->real_num_tx_queues = queue_count;
 
 #ifdef CONFIG_RPS
-	dev->_rx = rx;
 	dev->num_rx_queues = queue_count;
 #endif
 
@@ -5495,11 +5487,7 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
 	strcpy(dev->name, name);
 	return dev;
 
-free_rx:
-#ifdef CONFIG_RPS
-	kfree(rx);
 free_tx:
-#endif
 	kfree(tx);
 free_p:
 	kfree(p);

Re: [PATCH net-next-2.6] rps: allocate rx queues in register_netdevice only

[Eric Dumazet]

Le vendredi 24 septembre 2010 à 05:26 +0200, Eric Dumazet a écrit :
> > Also, I dont understand why we need to restrict
> > netif_set_real_num_rx_queues() to lower the count.
> > This wastes memory.
> > 
> > Why dont we allocate dev->_rx once we know the real count, not in
> > alloc_netdev_mq() but in register_netdevice() ?
> > 
> > 
> 
> Here is a patch to make this possible
> 
> I guess a similar thing could be done for tx queues.
> 
> boot tested, but please double check.
> 
> Thanks
> 
> [PATCH net-next-2.6] rps: allocate rx queues in register_netdevice()
> 
> Instead of having two places were we allocate dev->_rx, introduce
> netif_alloc_rx_queues() helper and call it only from
> register_netdevice(), not from alloc_netdev_mq()
> 
> Goal is to let drivers change dev->num_rx_queues after allocating netdev
> and before registering it.
> 
> This also removes a lot of ifdefs in net/core/dev.c
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
>  net/core/dev.c |   76 +++++++++++++++++++----------------------------
>  1 file changed, 32 insertions(+), 44 deletions(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 2c7934f..9110b8d 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4964,6 +4964,34 @@ void netif_stacked_transfer_operstate(const struct net_device *rootdev,
>  }
>  EXPORT_SYMBOL(netif_stacked_transfer_operstate);
>  
> +static int netif_alloc_rx_queues(struct net_device *dev)
> +{
> +#ifdef CONFIG_RPS
> +	unsigned int i, count = dev->num_rx_queues;
> +

Tom

I tried to find a caller with dev->num_rx_queues = 0, but failed

If it's valid, we might add a

	if (!dev->num_rx_queues)
		dev->num_rx_queues = 1;

I wonder if you remember why you added the section :
	/*
	 * Allocate a single RX queue if driver never called
	 * alloc_netdev_mq
	 */
> 
> 
> +	if (count) {
> +		struct netdev_rx_queue *rx;
> +
> +		rx = kcalloc(count, sizeof(struct netdev_rx_queue), GFP_KERNEL);
> +		if (!rx) {
> +			pr_err("netdev: Unable to allocate %u rx queues.\n",
> +			       count);
> +			return -ENOMEM;
> +		}
> +		dev->_rx = rx;
> +		atomic_set(&rx->count, count);
> +
> +		/*
> +		 * Set a pointer to first element in the array which holds the
> +		 * reference count.
> +		 */
> +		for (i = 0; i < count; i++)
> +			rx[i].first = rx;
> +	}
> +#endif
> +	return 0;
> +}
> +
>  /**
>   *	register_netdevice	- register a network device
>   *	@dev: device to register
> @@ -5001,24 +5029,10 @@ int register_netdevice(struct net_device *dev)
>  
>  	dev->iflink = -1;
>  
> -#ifdef CONFIG_RPS
> -	if (!dev->num_rx_queues) {
> -		/*
> -		 * Allocate a single RX queue if driver never called
> -		 * alloc_netdev_mq
> -		 */
> -
> -		dev->_rx = kzalloc(sizeof(struct netdev_rx_queue), GFP_KERNEL);
> -		if (!dev->_rx) {
> -			ret = -ENOMEM;
> -			goto out;
> -		}
> +	ret = netif_alloc_rx_queues(dev);
> +	if (ret)
> +		goto out;
>  
> -		dev->_rx->first = dev->_rx;
> -		atomic_set(&dev->_rx->count, 1);
> -		dev->num_rx_queues = 1;
> -	}
> -#endif
>  	/* Init, if this function is available */
>  	if (dev->netdev_ops->ndo_init) {
>  		ret = dev->netdev_ops->ndo_init(dev);
> @@ -5414,10 +5428,6 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
>  	struct net_device *dev;
>  	size_t alloc_size;
>  	struct net_device *p;
> -#ifdef CONFIG_RPS
> -	struct netdev_rx_queue *rx;
> -	int i;
> -#endif
>  
>  	BUG_ON(strlen(name) >= sizeof(dev->name));
>  
> @@ -5443,29 +5453,12 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
>  		goto free_p;
>  	}
>  
> -#ifdef CONFIG_RPS
> -	rx = kcalloc(queue_count, sizeof(struct netdev_rx_queue), GFP_KERNEL);
> -	if (!rx) {
> -		printk(KERN_ERR "alloc_netdev: Unable to allocate "
> -		       "rx queues.\n");
> -		goto free_tx;
> -	}
> -
> -	atomic_set(&rx->count, queue_count);
> -
> -	/*
> -	 * Set a pointer to first element in the array which holds the
> -	 * reference count.
> -	 */
> -	for (i = 0; i < queue_count; i++)
> -		rx[i].first = rx;
> -#endif
>  
>  	dev = PTR_ALIGN(p, NETDEV_ALIGN);
>  	dev->padded = (char *)dev - (char *)p;
>  
>  	if (dev_addr_init(dev))
> -		goto free_rx;
> +		goto free_tx;
>  
>  	dev_mc_init(dev);
>  	dev_uc_init(dev);
> @@ -5477,7 +5470,6 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
>  	dev->real_num_tx_queues = queue_count;
>  
>  #ifdef CONFIG_RPS
> -	dev->_rx = rx;
>  	dev->num_rx_queues = queue_count;
>  #endif
>  
> @@ -5495,11 +5487,7 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
>  	strcpy(dev->name, name);
>  	return dev;
>  
> -free_rx:
> -#ifdef CONFIG_RPS
> -	kfree(rx);
>  free_tx:
> -#endif
>  	kfree(tx);
>  free_p:
>  	kfree(p);
> 

Re: [PATCH net-next-2.6] rps: allocate rx queues in register_netdevice only

[Tom Herbert]

On Fri, Sep 24, 2010 at 12:07 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le vendredi 24 septembre 2010 à 05:26 +0200, Eric Dumazet a écrit :
>> > Also, I dont understand why we need to restrict
>> > netif_set_real_num_rx_queues() to lower the count.
>> > This wastes memory.
>> >
>> > Why dont we allocate dev->_rx once we know the real count, not in
>> > alloc_netdev_mq() but in register_netdevice() ?
>> >
>> >
>>
>> Here is a patch to make this possible
>>
>> I guess a similar thing could be done for tx queues.
>>
>> boot tested, but please double check.
>>
>> Thanks
>>
>> [PATCH net-next-2.6] rps: allocate rx queues in register_netdevice()
>>
>> Instead of having two places were we allocate dev->_rx, introduce
>> netif_alloc_rx_queues() helper and call it only from
>> register_netdevice(), not from alloc_netdev_mq()
>>
>> Goal is to let drivers change dev->num_rx_queues after allocating netdev
>> and before registering it.
>>
>> This also removes a lot of ifdefs in net/core/dev.c
>>
>> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
>> ---
>>  net/core/dev.c |   76 +++++++++++++++++++----------------------------
>>  1 file changed, 32 insertions(+), 44 deletions(-)
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index 2c7934f..9110b8d 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -4964,6 +4964,34 @@ void netif_stacked_transfer_operstate(const struct net_device *rootdev,
>>  }
>>  EXPORT_SYMBOL(netif_stacked_transfer_operstate);
>>
>> +static int netif_alloc_rx_queues(struct net_device *dev)
>> +{
>> +#ifdef CONFIG_RPS
>> +     unsigned int i, count = dev->num_rx_queues;
>> +
>
> Tom
>
> I tried to find a caller with dev->num_rx_queues = 0, but failed
>
> If it's valid, we might add a
>
>        if (!dev->num_rx_queues)
>                dev->num_rx_queues = 1;
>
Or maybe just enforce that in register_net_device and make it an error
to call alloc_netdev_mq with zero queue count.  You've uncovered a bug
since get_rps_cpus assumes at least one queue, but alloc_netdev_mq
allows zeros queues to be allocated!

> I wonder if you remember why you added the section :
>        /*
>         * Allocate a single RX queue if driver never called
>         * alloc_netdev_mq
>         */

RPS is configured per queue, so every device will need at least one RX
queue for the structures.

Re: [PATCH net-next-2.6] rps: allocate rx queues in register_netdevice only

[John Fastabend]

On 9/23/2010 8:26 PM, Eric Dumazet wrote:
> 
>> Also, I dont understand why we need to restrict
>> netif_set_real_num_rx_queues() to lower the count.
>> This wastes memory.
>>
>> Why dont we allocate dev->_rx once we know the real count, not in
>> alloc_netdev_mq() but in register_netdevice() ?
>>

Eric,

At least in the TX case we may not "know" until later how many tx_queues we want to use. For example it could change based on enabling/disabling features or available interrupts. So we use num_tx_queues as the max we ever expect to use and then netif_set_real_num_tx_queues() sets the number we want to use.

I presume for rx queues there are similar cases where features and available interrupts may determine how many rx queues are needed.

Moving the allocation later could help drivers make better max number of queue decisions. But, I think we still need the netif_set_num_rx_queues() and netif_set_num_tx_queues(). Although this does end up wasting memory as you pointed out.

Thanks,
John. 

>>
> 
> Here is a patch to make this possible
> 
> I guess a similar thing could be done for tx queues.
> 
> boot tested, but please double check.
> 
> Thanks
> 
> [PATCH net-next-2.6] rps: allocate rx queues in register_netdevice()
> 
> Instead of having two places were we allocate dev->_rx, introduce
> netif_alloc_rx_queues() helper and call it only from
> register_netdevice(), not from alloc_netdev_mq()
> 
> Goal is to let drivers change dev->num_rx_queues after allocating netdev
> and before registering it.
> 
> This also removes a lot of ifdefs in net/core/dev.c
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
> ---
>  net/core/dev.c |   76 +++++++++++++++++++----------------------------
>  1 file changed, 32 insertions(+), 44 deletions(-)
> 
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 2c7934f..9110b8d 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4964,6 +4964,34 @@ void netif_stacked_transfer_operstate(const struct net_device *rootdev,
>  }
>  EXPORT_SYMBOL(netif_stacked_transfer_operstate);
>  
> +static int netif_alloc_rx_queues(struct net_device *dev)
> +{
> +#ifdef CONFIG_RPS
> +	unsigned int i, count = dev->num_rx_queues;
> +
> +	if (count) {
> +		struct netdev_rx_queue *rx;
> +
> +		rx = kcalloc(count, sizeof(struct netdev_rx_queue), GFP_KERNEL);
> +		if (!rx) {
> +			pr_err("netdev: Unable to allocate %u rx queues.\n",
> +			       count);
> +			return -ENOMEM;
> +		}
> +		dev->_rx = rx;
> +		atomic_set(&rx->count, count);
> +
> +		/*
> +		 * Set a pointer to first element in the array which holds the
> +		 * reference count.
> +		 */
> +		for (i = 0; i < count; i++)
> +			rx[i].first = rx;
> +	}
> +#endif
> +	return 0;
> +}
> +
>  /**
>   *	register_netdevice	- register a network device
>   *	@dev: device to register
> @@ -5001,24 +5029,10 @@ int register_netdevice(struct net_device *dev)
>  
>  	dev->iflink = -1;
>  
> -#ifdef CONFIG_RPS
> -	if (!dev->num_rx_queues) {
> -		/*
> -		 * Allocate a single RX queue if driver never called
> -		 * alloc_netdev_mq
> -		 */
> -
> -		dev->_rx = kzalloc(sizeof(struct netdev_rx_queue), GFP_KERNEL);
> -		if (!dev->_rx) {
> -			ret = -ENOMEM;
> -			goto out;
> -		}
> +	ret = netif_alloc_rx_queues(dev);
> +	if (ret)
> +		goto out;
>  
> -		dev->_rx->first = dev->_rx;
> -		atomic_set(&dev->_rx->count, 1);
> -		dev->num_rx_queues = 1;
> -	}
> -#endif
>  	/* Init, if this function is available */
>  	if (dev->netdev_ops->ndo_init) {
>  		ret = dev->netdev_ops->ndo_init(dev);
> @@ -5414,10 +5428,6 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
>  	struct net_device *dev;
>  	size_t alloc_size;
>  	struct net_device *p;
> -#ifdef CONFIG_RPS
> -	struct netdev_rx_queue *rx;
> -	int i;
> -#endif
>  
>  	BUG_ON(strlen(name) >= sizeof(dev->name));
>  
> @@ -5443,29 +5453,12 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
>  		goto free_p;
>  	}
>  
> -#ifdef CONFIG_RPS
> -	rx = kcalloc(queue_count, sizeof(struct netdev_rx_queue), GFP_KERNEL);
> -	if (!rx) {
> -		printk(KERN_ERR "alloc_netdev: Unable to allocate "
> -		       "rx queues.\n");
> -		goto free_tx;
> -	}
> -
> -	atomic_set(&rx->count, queue_count);
> -
> -	/*
> -	 * Set a pointer to first element in the array which holds the
> -	 * reference count.
> -	 */
> -	for (i = 0; i < queue_count; i++)
> -		rx[i].first = rx;
> -#endif
>  
>  	dev = PTR_ALIGN(p, NETDEV_ALIGN);
>  	dev->padded = (char *)dev - (char *)p;
>  
>  	if (dev_addr_init(dev))
> -		goto free_rx;
> +		goto free_tx;
>  
>  	dev_mc_init(dev);
>  	dev_uc_init(dev);
> @@ -5477,7 +5470,6 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
>  	dev->real_num_tx_queues = queue_count;
>  
>  #ifdef CONFIG_RPS
> -	dev->_rx = rx;
>  	dev->num_rx_queues = queue_count;
>  #endif
>  
> @@ -5495,11 +5487,7 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
>  	strcpy(dev->name, name);
>  	return dev;
>  
> -free_rx:
> -#ifdef CONFIG_RPS
> -	kfree(rx);
>  free_tx:
> -#endif
>  	kfree(tx);
>  free_p:
>  	kfree(p);
> 
> 
> --
> 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

Re: [PATCH net-next-2.6] rps: allocate rx queues in register_netdevice only

[Eric Dumazet]

Le vendredi 24 septembre 2010 à 01:15 -0700, John Fastabend a écrit :
> On 9/23/2010 8:26 PM, Eric Dumazet wrote:
> > 
> >> Also, I dont understand why we need to restrict
> >> netif_set_real_num_rx_queues() to lower the count.
> >> This wastes memory.
> >>
> >> Why dont we allocate dev->_rx once we know the real count, not in
> >> alloc_netdev_mq() but in register_netdevice() ?
> >>
> 
> Eric,
> 
> At least in the TX case we may not "know" until later how many
> tx_queues we want to use. For example it could change based on
> enabling/disabling features or available interrupts. So we use
> num_tx_queues as the max we ever expect to use and then
> netif_set_real_num_tx_queues() sets the number we want to use.
> 
> I presume for rx queues there are similar cases where features and
> available interrupts may determine how many rx queues are needed.
> 
> Moving the allocation later could help drivers make better max number
> of queue decisions. But, I think we still need the
> netif_set_num_rx_queues() and netif_set_num_tx_queues(). Although this
> does end up wasting memory as you pointed out.
> 

Note I am not against having netif_set_num_rx_queues() and
netif_set_num_tx_queues(). My patch was a cleanup, not an alternative.


If I take a look at sysfs stuff, on a machine with a bnx2 adapter,
single queue, I get :

/sys/class/net/eth0/queues/rx-0/rps_cpus
/sys/class/net/eth0/queues/rx-0/rps_flow_cnt
/sys/class/net/eth0/queues/rx-1/rps_cpus
/sys/class/net/eth0/queues/rx-1/rps_flow_cnt
/sys/class/net/eth0/queues/rx-2/rps_cpus
/sys/class/net/eth0/queues/rx-2/rps_flow_cnt
/sys/class/net/eth0/queues/rx-3/rps_cpus
/sys/class/net/eth0/queues/rx-3/rps_flow_cnt
/sys/class/net/eth0/queues/rx-4/rps_cpus
/sys/class/net/eth0/queues/rx-4/rps_flow_cnt
/sys/class/net/eth0/queues/rx-5/rps_cpus
/sys/class/net/eth0/queues/rx-5/rps_flow_cnt
/sys/class/net/eth0/queues/rx-6/rps_cpus
/sys/class/net/eth0/queues/rx-6/rps_flow_cnt
/sys/class/net/eth0/queues/rx-7/rps_cpus
/sys/class/net/eth0/queues/rx-7/rps_flow_cnt

Thats a lot of extra memory and administrator confusion.

We all agree :)

Re: [PATCH net-next-2.6] rps: allocate rx queues in register_netdevice only

[John Fastabend]

On 9/24/2010 2:21 AM, Eric Dumazet wrote:
> Le vendredi 24 septembre 2010 à 01:15 -0700, John Fastabend a écrit :
>> On 9/23/2010 8:26 PM, Eric Dumazet wrote:
>>>
>>>> Also, I dont understand why we need to restrict
>>>> netif_set_real_num_rx_queues() to lower the count.
>>>> This wastes memory.
>>>>
>>>> Why dont we allocate dev->_rx once we know the real count, not in
>>>> alloc_netdev_mq() but in register_netdevice() ?
>>>>
>>
>> Eric,
>>
>> At least in the TX case we may not "know" until later how many
>> tx_queues we want to use. For example it could change based on
>> enabling/disabling features or available interrupts. So we use
>> num_tx_queues as the max we ever expect to use and then
>> netif_set_real_num_tx_queues() sets the number we want to use.
>>
>> I presume for rx queues there are similar cases where features and
>> available interrupts may determine how many rx queues are needed.
>>
>> Moving the allocation later could help drivers make better max number
>> of queue decisions. But, I think we still need the
>> netif_set_num_rx_queues() and netif_set_num_tx_queues(). Although this
>> does end up wasting memory as you pointed out.
>>
> 
> Note I am not against having netif_set_num_rx_queues() and
> netif_set_num_tx_queues(). My patch was a cleanup, not an alternative.
> 
> 
> If I take a look at sysfs stuff, on a machine with a bnx2 adapter,
> single queue, I get :
> 
> /sys/class/net/eth0/queues/rx-0/rps_cpus
> /sys/class/net/eth0/queues/rx-0/rps_flow_cnt
> /sys/class/net/eth0/queues/rx-1/rps_cpus
> /sys/class/net/eth0/queues/rx-1/rps_flow_cnt
> /sys/class/net/eth0/queues/rx-2/rps_cpus
> /sys/class/net/eth0/queues/rx-2/rps_flow_cnt
> /sys/class/net/eth0/queues/rx-3/rps_cpus
> /sys/class/net/eth0/queues/rx-3/rps_flow_cnt
> /sys/class/net/eth0/queues/rx-4/rps_cpus
> /sys/class/net/eth0/queues/rx-4/rps_flow_cnt
> /sys/class/net/eth0/queues/rx-5/rps_cpus
> /sys/class/net/eth0/queues/rx-5/rps_flow_cnt
> /sys/class/net/eth0/queues/rx-6/rps_cpus
> /sys/class/net/eth0/queues/rx-6/rps_flow_cnt
> /sys/class/net/eth0/queues/rx-7/rps_cpus
> /sys/class/net/eth0/queues/rx-7/rps_flow_cnt
> 
> Thats a lot of extra memory and administrator confusion.
> 
> We all agree :)
> 
> 

Thanks for the clarification Eric.

Re: [PATCH net-next-2.6] rps: allocate rx queues in register_netdevice only

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 24 Sep 2010 05:26:35 +0200

> [PATCH net-next-2.6] rps: allocate rx queues in register_netdevice()
> 
> Instead of having two places were we allocate dev->_rx, introduce
> netif_alloc_rx_queues() helper and call it only from
> register_netdevice(), not from alloc_netdev_mq()
> 
> Goal is to let drivers change dev->num_rx_queues after allocating netdev
> and before registering it.
> 
> This also removes a lot of ifdefs in net/core/dev.c
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied, thanks Eric.

Please cook up a patch that adds a check to make sure that
alloc_netdev_mq() is never called with number of queues < 1

Thanks again!

Re: [PATCH net-next-2.6 1/2] net: Allow changing number of RX queues after device allocation

[Ben Hutchings]

On Fri, 2010-09-24 at 04:48 +0200, Eric Dumazet wrote:
> Le jeudi 23 septembre 2010 à 21:19 +0100, Ben Hutchings a écrit :
> > For RPS, we create a kobject for each RX queue based on the number of
> > queues passed to alloc_netdev_mq().  However, drivers generally do not
> > determine the numbers of hardware queues to use until much later, so
> > this usually represents the maximum number the driver may use and not
> > the actual number in use.
> > 
> > For TX queues, drivers can update the actual number using
> > netif_set_real_num_tx_queues().  Add a corresponding function for RX
> > queues, netif_set_real_num_rx_queues().
> > 
> > Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
> > ---
> 
> Seems very good to me, except a minor style issue here :
> 
> All functions in this file use a single line.
> 
> > -static int rx_queue_register_kobjects(struct net_device *net)
> > +int
> > +net_rx_queue_update_kobjects(struct net_device *net, int old_num, int new_num)
> 
> ->
> 
> int net_rx_queue_update_kobjects(struct net_device *net, int old, int new)

Why should I rename variables to fit a function signature on one line?

> Also, I dont understand why we need to restrict
> netif_set_real_num_rx_queues() to lower the count.
> This wastes memory.
> 
> Why dont we allocate dev->_rx once we know the real count, not in
> alloc_netdev_mq() but in register_netdevice() ?

We probably could do.  I was just being consistent with
netif_set_real_num_tx_queues().

Note that both functions allow changing the number of queues on a
registered device, although I don't think any driver uses this.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

Re: [PATCH net-next-2.6 1/2] net: Allow changing number of RX queues after device allocation

[Tom Herbert]

Looks good to me.

>> All functions in this file use a single line.
>>
>> > -static int rx_queue_register_kobjects(struct net_device *net)
>> > +int
>> > +net_rx_queue_update_kobjects(struct net_device *net, int old_num, int new_num)
>>
>> ->
>>
>> int net_rx_queue_update_kobjects(struct net_device *net, int old, int new)
>
> Why should I rename variables to fit a function signature on one line?
>
Maybe split the line within the parameter list, this would be more
consistent with other function declarations.

[PATCH net-next-2.6 2/2] sfc: Use proper functions to set core RX and TX queue counts

[Ben Hutchings]

Call netif_real_num_tx_queues() instead of setting
net_device::real_num_tx_queues directly.

Also call the new netif_real_num_rx_queues() function to set the
core RX queue count correctly for RPS.

Signed-off-by: Ben Hutchings <bhutchings@solarflare.com>
---
 drivers/net/sfc/efx.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/net/sfc/efx.c b/drivers/net/sfc/efx.c
index 5be71f4..fa6e020 100644
--- a/drivers/net/sfc/efx.c
+++ b/drivers/net/sfc/efx.c
@@ -1315,7 +1315,8 @@ static int efx_probe_nic(struct efx_nic *efx)
 		efx->rx_indir_table[i] = i % efx->n_rx_channels;
 
 	efx_set_channels(efx);
-	efx->net_dev->real_num_tx_queues = efx->n_tx_channels;
+	netif_set_real_num_tx_queues(efx->net_dev, efx->n_tx_channels);
+	netif_set_real_num_rx_queues(efx->net_dev, efx->n_rx_channels);
 
 	/* Initialise the interrupt moderation settings */
 	efx_init_irq_moderation(efx, tx_irq_mod_usec, rx_irq_mod_usec, true);
-- 
1.7.2.1


-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.