| Message ID | alpine.DEB.1.00.1010281036350.22486@pokey.mtv.corp.google.com |
|---|---|
| State | Rejected, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Tom Herbert <therbert@google.com> Date: Thu, 28 Oct 2010 10:45:45 -0700 (PDT) > Both TX and RX queue allocations in the netdev structure had been moved > to register_device with the idea that the number of queues could be > changed between device allocation and registration. It was determined > that changing the num_queues after the call to alloc_netdev_mq was not > a good idea, so that was abandoned. Also, some drivers call > netif_queue_start without registering device, which causes panic > when dev->_tx queue structure is accessed. This patch moves queue > allocations back to alloc_netdev_mq to fix these issues. > > Signed-off-by: Tom Herbert <therbert@google.com> Changing the TX queue state with a netif_stop_queue() call or similar was always completely pointless and frankly a bug. So that should not influence the motivation behind this change at all. In fact I _like_ that it crashes now so that we are forced to fix these cases. Really, the queue state is absolutely immaterial during device allocation and registration. It's state is %100 "don't care" until ->open() is invoked. So any code that touches the queue state at these earlier points in time is completely extraneous if not broken. -- 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
On Thu, Oct 28, 2010 at 10:49 AM, David Miller <davem@davemloft.net> wrote: > From: Tom Herbert <therbert@google.com> > Date: Thu, 28 Oct 2010 10:45:45 -0700 (PDT) > >> Both TX and RX queue allocations in the netdev structure had been moved >> to register_device with the idea that the number of queues could be >> changed between device allocation and registration. It was determined >> that changing the num_queues after the call to alloc_netdev_mq was not >> a good idea, so that was abandoned. Also, some drivers call >> netif_queue_start without registering device, which causes panic >> when dev->_tx queue structure is accessed. This patch moves queue >> allocations back to alloc_netdev_mq to fix these issues. >> >> Signed-off-by: Tom Herbert <therbert@google.com> > > Changing the TX queue state with a netif_stop_queue() call or > similar was always completely pointless and frankly a bug. > > So that should not influence the motivation behind this change > at all. > > In fact I _like_ that it crashes now so that we are forced > to fix these cases. > > Really, the queue state is absolutely immaterial during > device allocation and registration. It's state is > %100 "don't care" until ->open() is invoked. > > So any code that touches the queue state at these earlier points in > time is completely extraneous if not broken. > -- 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
Did you mean to actually say something other than quoting everything said so far? :-) -- 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
On Thu, Oct 28, 2010 at 11:10 AM, David Miller <davem@davemloft.net> wrote: > > Did you mean to actually say something other than quoting everything > said so far? :-) > You're response was so nicely worded that I had to repeat it :-) Anyway, thanks for the clarification! > -- 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 --git a/net/core/dev.c b/net/core/dev.c index b2269ac..1f729d6 100644 --- a/net/core/dev.c +++ b/net/core/dev.c @@ -5009,10 +5009,10 @@ 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) +static int netif_alloc_rx_queues(struct net_device *dev, int count) { #ifdef CONFIG_RPS - unsigned int i, count = dev->num_rx_queues; + int i; struct netdev_rx_queue *rx; BUG_ON(count < 1); @@ -5030,13 +5030,15 @@ static int netif_alloc_rx_queues(struct net_device *dev) */ for (i = 0; i < count; i++) rx[i].first = rx; + + dev->num_rx_queues = count; + dev->real_num_rx_queues = count; #endif return 0; } -static int netif_alloc_netdev_queues(struct net_device *dev) +static int netif_alloc_netdev_queues(struct net_device *dev, int count) { - unsigned int count = dev->num_tx_queues; struct netdev_queue *tx; BUG_ON(count < 1); @@ -5048,6 +5050,10 @@ static int netif_alloc_netdev_queues(struct net_device *dev) return -ENOMEM; } dev->_tx = tx; + + dev->num_tx_queues = count; + dev->real_num_tx_queues = count; + return 0; } @@ -5105,14 +5111,6 @@ int register_netdevice(struct net_device *dev) dev->iflink = -1; - ret = netif_alloc_rx_queues(dev); - if (ret) - goto out; - - ret = netif_alloc_netdev_queues(dev); - if (ret) - goto out; - netdev_init_queues(dev); /* Init, if this function is available */ @@ -5558,6 +5556,12 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name, dev = PTR_ALIGN(p, NETDEV_ALIGN); dev->padded = (char *)dev - (char *)p; + if (netif_alloc_netdev_queues(dev, queue_count)) + goto free_p; + + if (netif_alloc_rx_queues(dev, queue_count)) + goto free_p; + dev->pcpu_refcnt = alloc_percpu(int); if (!dev->pcpu_refcnt) goto free_p; @@ -5570,14 +5574,6 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name, dev_net_set(dev, &init_net); - dev->num_tx_queues = queue_count; - dev->real_num_tx_queues = queue_count; - -#ifdef CONFIG_RPS - dev->num_rx_queues = queue_count; - dev->real_num_rx_queues = queue_count; -#endif - dev->gso_max_size = GSO_MAX_SIZE; INIT_LIST_HEAD(&dev->ethtool_ntuple_list.list); @@ -5593,6 +5589,8 @@ struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name, free_pcpu: free_percpu(dev->pcpu_refcnt); free_p: + kfree(dev->_tx); + kfree(dev->_rx); kfree(p); return NULL; }
Both TX and RX queue allocations in the netdev structure had been moved to register_device with the idea that the number of queues could be changed between device allocation and registration. It was determined that changing the num_queues after the call to alloc_netdev_mq was not a good idea, so that was abandoned. Also, some drivers call netif_queue_start without registering device, which causes panic when dev->_tx queue structure is accessed. This patch moves queue allocations back to alloc_netdev_mq to fix these issues. Signed-off-by: Tom Herbert <therbert@google.com> --- net/core/dev.c | 38 ++++++++++++++++++-------------------- 1 files changed, 18 insertions(+), 20 deletions(-)