diff mbox

[v4] net: Introduce realloc_netdev_mq()

Message ID 20091203202937.GA11436@ami.dom.local
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Jarek Poplawski Dec. 3, 2009, 8:29 p.m. UTC 
On Thu, Dec 03, 2009 at 06:05:39PM +0100, Eric Dumazet wrote:
> Jarek Poplawski a écrit :
> > On Thu, Dec 03, 2009 at 05:36:40PM +0100, Jarek Poplawski wrote:
> >> On Thu, Dec 03, 2009 at 04:17:43PM +0100, Eric Dumazet wrote:
> >>> if (realloc_netdev_mq(dev, real_queues))
> >>> 	dev->real_num_tx_queues = real_queues;
> >>>
> >>> In this case the memory error is not fatal.
> >> Good point! We can consider doing this inside the function too?
> > 
> > Hmm... Of course, not exactly this - I mean using min().
> 
> Sure, allowing to reduce the count in case new allocation failed.
> 
> And report an error if caller wanted to increase number of queues and allocation failed.

Hmm... After re-thinking it looks a bit too complex to me. I think,
there is no reason to not report this error since in most cases it
shouldn't be fatal. That's why I skipped this check in the changelog
example. Unless I miss something?

Thanks,
Jarek P.
---------------> (take 4)

This patch separates allocation of TX subqueues from alloc_netdev_mq()
to realloc_netdev_mq() to allow for resizing like in this example:

some_nic_probe()
{
	...
	dev = alloc_etherdev_mq(sizeof(*bp), 1)
	...
	if (MSI-X_available && device_supports_MSI-X_and_multiqueue)
		realloc_netdev_mq(dev, TX_MAX_RINGS)
	register_netdev(dev)
	...
}

Alternatively, it can be done in reverse order: starting from the
highest queue_count and reallocating with a lower one.

The main difference is to hold in num_tx_queues something that is
really available, instead of max possible value for all configs, in
case of drivers allocating net_device at the beginning of the probe.

The description of alloc_netdev_mq() is fixed btw.

Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
Cc: Eric Dumazet <eric.dumazet@gmail.com>
---

 include/linux/netdevice.h |    1 +
 net/core/dev.c            |   69 ++++++++++++++++++++++++++++++++------------
 2 files changed, 51 insertions(+), 19 deletions(-)

--
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

Comments

Eric Dumazet Dec. 3, 2009, 9:29 p.m. UTC | #1 
Jarek Poplawski a écrit :
> On Thu, Dec 03, 2009 at 06:05:39PM +0100, Eric Dumazet wrote:
>> Jarek Poplawski a écrit :
>>> On Thu, Dec 03, 2009 at 05:36:40PM +0100, Jarek Poplawski wrote:
>>>> On Thu, Dec 03, 2009 at 04:17:43PM +0100, Eric Dumazet wrote:
>>>>> if (realloc_netdev_mq(dev, real_queues))
>>>>> 	dev->real_num_tx_queues = real_queues;
>>>>>
>>>>> In this case the memory error is not fatal.
>>>> Good point! We can consider doing this inside the function too?
>>> Hmm... Of course, not exactly this - I mean using min().
>> Sure, allowing to reduce the count in case new allocation failed.
>>
>> And report an error if caller wanted to increase number of queues and allocation failed.
> 
> Hmm... After re-thinking it looks a bit too complex to me. I think,
> there is no reason to not report this error since in most cases it
> shouldn't be fatal. That's why I skipped this check in the changelog
> example. Unless I miss something?
> 
> Thanks,
> Jarek P.
> ---------------> (take 4)
> 
> This patch separates allocation of TX subqueues from alloc_netdev_mq()
> to realloc_netdev_mq() to allow for resizing like in this example:
> 
> some_nic_probe()
> {
> 	...
> 	dev = alloc_etherdev_mq(sizeof(*bp), 1)
> 	...
> 	if (MSI-X_available && device_supports_MSI-X_and_multiqueue)
> 		realloc_netdev_mq(dev, TX_MAX_RINGS)
> 	register_netdev(dev)
> 	...
> }
> 
> Alternatively, it can be done in reverse order: starting from the
> highest queue_count and reallocating with a lower one.
> 
> The main difference is to hold in num_tx_queues something that is
> really available, instead of max possible value for all configs, in
> case of drivers allocating net_device at the beginning of the probe.
> 
> The description of alloc_netdev_mq() is fixed btw.
> 
> Reported-by: Eric Dumazet <eric.dumazet@gmail.com>
> Signed-off-by: Jarek Poplawski <jarkao2@gmail.com>
> Cc: Eric Dumazet <eric.dumazet@gmail.com>

Nice patch, thanks :)

Acked-by: Eric Dumazet <eric.dumazet@gmail.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
David Miller Dec. 3, 2009, 9:31 p.m. UTC | #2 
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Thu, 03 Dec 2009 22:29:42 +0100

> Nice patch, thanks :)
> 
> Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

I like it too, but please resubmit once we have at least
one example user submitted.

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
Eric Dumazet Dec. 3, 2009, 9:32 p.m. UTC | #3 
David Miller a écrit :
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Thu, 03 Dec 2009 22:29:42 +0100
> 
>> Nice patch, thanks :)
>>
>> Acked-by: Eric Dumazet <eric.dumazet@gmail.com>
> 
> I like it too, but please resubmit once we have at least
> one example user submitted.
> 
> Thanks!

Well, I was going to do that :)


--
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
diff mbox

Patch

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index daf13d3..36cbd53 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -1904,6 +1904,7 @@  extern void		ether_setup(struct net_device *dev);
 extern struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
 				       void (*setup)(struct net_device *),
 				       unsigned int queue_count);
+extern int realloc_netdev_mq(struct net_device *dev, unsigned int queue_count);
 #define alloc_netdev(sizeof_priv, name, setup) \
 	alloc_netdev_mq(sizeof_priv, name, setup, 1)
 extern int		register_netdev(struct net_device *dev);
diff --git a/net/core/dev.c b/net/core/dev.c
index e3e18de..1f45bae 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5265,11 +5265,53 @@  static void netdev_init_one_queue(struct net_device *dev,
 	queue->dev = dev;
 }
 
-static void netdev_init_queues(struct net_device *dev)
+/**
+ *	realloc_netdev_mq - (re)allocate network subqueues
+ *	@dev:		device
+ *	@queue_count:	the number of subqueues to (re)allocate
+ *
+ *	(Re)allocates and initializes subqueue structs for each queue.
+ *	It is allowed to use only until register_netdev().
+ *	On error previous structs are intact, but dev->real_num_tx_queue is
+ *	replaced if the queue_count is lower.
+ */
+int realloc_netdev_mq(struct net_device *dev, unsigned int queue_count)
 {
-	netdev_init_one_queue(dev, &dev->rx_queue, NULL);
+	struct netdev_queue *tx;
+
+	tx = kcalloc(queue_count, sizeof(struct netdev_queue), GFP_KERNEL);
+	if (!tx) {
+		printk(KERN_ERR "alloc_netdev: Unable to (re)allocate "
+		       "tx qdiscs.\n");
+		if (dev->real_num_tx_queues > queue_count)
+			dev->real_num_tx_queues = queue_count;
+
+		return -ENOMEM;
+	}
+
+	kfree(dev->_tx);
+
+	dev->_tx = tx;
+	dev->num_tx_queues = queue_count;
+	dev->real_num_tx_queues = queue_count;
+
 	netdev_for_each_tx_queue(dev, netdev_init_one_queue, NULL);
+
+	return 0;
+}
+EXPORT_SYMBOL(realloc_netdev_mq);
+
+static int netdev_init_queues(struct net_device *dev, unsigned int queue_count)
+{
+	int err = realloc_netdev_mq(dev, queue_count);
+
+	if (err)
+		return err;
+
+	netdev_init_one_queue(dev, &dev->rx_queue, NULL);
 	spin_lock_init(&dev->tx_global_lock);
+
+	return 0;
 }
 
 /**
@@ -5280,13 +5322,12 @@  static void netdev_init_queues(struct net_device *dev)
  *	@queue_count:	the number of subqueues to allocate
  *
  *	Allocates a struct net_device with private data area for driver use
- *	and performs basic initialization.  Also allocates subquue structs
- *	for each queue on the device at the end of the netdevice.
+ *	and performs basic initialization.  Also allocates subqueue structs
+ *	for each queue on the device.
  */
 struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
 		void (*setup)(struct net_device *), unsigned int queue_count)
 {
-	struct netdev_queue *tx;
 	struct net_device *dev;
 	size_t alloc_size;
 	struct net_device *p;
@@ -5308,16 +5349,12 @@  struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
 		return NULL;
 	}
 
-	tx = kcalloc(queue_count, sizeof(struct netdev_queue), GFP_KERNEL);
-	if (!tx) {
-		printk(KERN_ERR "alloc_netdev: Unable to allocate "
-		       "tx qdiscs.\n");
-		goto free_p;
-	}
-
 	dev = PTR_ALIGN(p, NETDEV_ALIGN);
 	dev->padded = (char *)dev - (char *)p;
 
+	if (netdev_init_queues(dev, queue_count))
+		goto free_p;
+
 	if (dev_addr_init(dev))
 		goto free_tx;
 
@@ -5325,14 +5362,8 @@  struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
 
 	dev_net_set(dev, &init_net);
 
-	dev->_tx = tx;
-	dev->num_tx_queues = queue_count;
-	dev->real_num_tx_queues = queue_count;
-
 	dev->gso_max_size = GSO_MAX_SIZE;
 
-	netdev_init_queues(dev);
-
 	INIT_LIST_HEAD(&dev->napi_list);
 	INIT_LIST_HEAD(&dev->unreg_list);
 	INIT_LIST_HEAD(&dev->link_watch_list);
@@ -5342,7 +5373,7 @@  struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
 	return dev;
 
 free_tx:
-	kfree(tx);
+	kfree(dev->_tx);
 
 free_p:
 	kfree(p);