| Message ID | 1285298795.2380.54.camel@edumazet-laptop |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
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); > -- 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 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 -- 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
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 :) -- 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 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. -- 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 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. -- 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
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! -- 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 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);