| Message ID | alpine.DEB.2.02.1408241312500.30395@tomh.mtv.corp.google.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Tom Herbert <therbert@google.com> Date: Sun, 24 Aug 2014 13:19:47 -0700 (PDT) > +/** > + * netdev_avail_queue - report how much space is availble for xmit > + * @dev: network device > + * > + * Report the amount of space available in the TX queue in terms of > + * number of bytes. This returns the number of bytes avaiable per > + * DQL. This function may be called without taking the txlock on > + * the device, however in that case the result should be taken as > + * a (strong) hint. > + */ > +static inline int netdev_avail_queue(struct net_device *dev_queue) > +{ > + return netdev_tx_avail_queue(netdev_get_tx_queue(dev_queue, 0)); > +} > + This doesn't make any sense, you're only providing queue zero's information. You're passing in a net_device, calling it a "dev_queue" in the variable name, the exlicitly using queue zero of that device in the netdev_get_tx_queue() call. Pretty confusing if you ask me :) -- 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 Sun, Aug 24, 2014 at 10:35 PM, David Miller <davem@davemloft.net> wrote: > From: Tom Herbert <therbert@google.com> > Date: Sun, 24 Aug 2014 13:19:47 -0700 (PDT) > >> +/** >> + * netdev_avail_queue - report how much space is availble for xmit >> + * @dev: network device >> + * >> + * Report the amount of space available in the TX queue in terms of >> + * number of bytes. This returns the number of bytes avaiable per >> + * DQL. This function may be called without taking the txlock on >> + * the device, however in that case the result should be taken as >> + * a (strong) hint. >> + */ >> +static inline int netdev_avail_queue(struct net_device *dev_queue) >> +{ >> + return netdev_tx_avail_queue(netdev_get_tx_queue(dev_queue, 0)); >> +} >> + > > This doesn't make any sense, you're only providing queue zero's > information. > It's consistent with the other BQL functions. netdev_*_queue is used for single queue devices, netdev_tx_*_queue is used for specific queues in MQ. > You're passing in a net_device, calling it a "dev_queue" in the > variable name, the exlicitly using queue zero of that device in the > netdev_get_tx_queue() call. > Cut and paste prototype from netdev_reset_queue which was doing that... I'll fix it. > Pretty confusing if you ask me :) -- 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 Mon, Aug 25, 2014 at 8:18 AM, Tom Herbert <therbert@google.com> wrote: > On Sun, Aug 24, 2014 at 10:35 PM, David Miller <davem@davemloft.net> wrote: >> From: Tom Herbert <therbert@google.com> >> Date: Sun, 24 Aug 2014 13:19:47 -0700 (PDT) >> >>> +/** >>> + * netdev_avail_queue - report how much space is availble for xmit >>> + * @dev: network device >>> + * >>> + * Report the amount of space available in the TX queue in terms of >>> + * number of bytes. This returns the number of bytes avaiable per >>> + * DQL. This function may be called without taking the txlock on >>> + * the device, however in that case the result should be taken as >>> + * a (strong) hint. >>> + */ >>> +static inline int netdev_avail_queue(struct net_device *dev_queue) >>> +{ >>> + return netdev_tx_avail_queue(netdev_get_tx_queue(dev_queue, 0)); >>> +} >>> + >> >> This doesn't make any sense, you're only providing queue zero's >> information. >> > It's consistent with the other BQL functions. netdev_*_queue is used > for single queue devices, netdev_tx_*_queue is used for specific > queues in MQ. > I was also confused by these names when I read the code, it doesn't make sense to me either. We need some cleanup no matter of your patch. I will try to do it after your patch is in. -- 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 Mon, Aug 25, 2014 at 12:24 PM, Cong Wang <cwang@twopensource.com> wrote: > On Mon, Aug 25, 2014 at 8:18 AM, Tom Herbert <therbert@google.com> wrote: >> On Sun, Aug 24, 2014 at 10:35 PM, David Miller <davem@davemloft.net> wrote: >>> From: Tom Herbert <therbert@google.com> >>> Date: Sun, 24 Aug 2014 13:19:47 -0700 (PDT) >>> >>>> +/** >>>> + * netdev_avail_queue - report how much space is availble for xmit >>>> + * @dev: network device >>>> + * >>>> + * Report the amount of space available in the TX queue in terms of >>>> + * number of bytes. This returns the number of bytes avaiable per >>>> + * DQL. This function may be called without taking the txlock on >>>> + * the device, however in that case the result should be taken as >>>> + * a (strong) hint. >>>> + */ >>>> +static inline int netdev_avail_queue(struct net_device *dev_queue) >>>> +{ >>>> + return netdev_tx_avail_queue(netdev_get_tx_queue(dev_queue, 0)); >>>> +} >>>> + >>> >>> This doesn't make any sense, you're only providing queue zero's >>> information. >>> >> It's consistent with the other BQL functions. netdev_*_queue is used >> for single queue devices, netdev_tx_*_queue is used for specific >> queues in MQ. >> > > I was also confused by these names when I read the code, it doesn't > make sense to me either. We need some cleanup no matter of your > patch. I will try to do it after your patch is in. That would be great if you can clean this up in a sane way, but beware that some of these functions following the naming convention are already in widespread use. For instance, both netif_stop_queue and netif_tx_stop_queue are called a lot from drivers. -- 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/include/linux/netdevice.h b/include/linux/netdevice.h index 0fac884..13883c1 100644 --- a/include/linux/netdevice.h +++ b/include/linux/netdevice.h @@ -2544,6 +2544,30 @@ static inline void netdev_completed_queue(struct net_device *dev, netdev_tx_completed_queue(netdev_get_tx_queue(dev, 0), pkts, bytes); } +static inline int netdev_tx_avail_queue(struct netdev_queue *dev_queue) +{ +#ifdef CONFIG_BQL + return dql_avail(&dev_queue->dql); +#else + return DQL_MAX_LIMIT; +#endif +} + +/** + * netdev_avail_queue - report how much space is availble for xmit + * @dev: network device + * + * Report the amount of space available in the TX queue in terms of + * number of bytes. This returns the number of bytes avaiable per + * DQL. This function may be called without taking the txlock on + * the device, however in that case the result should be taken as + * a (strong) hint. + */ +static inline int netdev_avail_queue(struct net_device *dev_queue) +{ + return netdev_tx_avail_queue(netdev_get_tx_queue(dev_queue, 0)); +} + static inline void netdev_tx_reset_queue(struct netdev_queue *q) { #ifdef CONFIG_BQL
This patch adds netdev_tx_avail_queue and netdev_avail_queue which are used to report number of bytes available in transmit queues per BQL. The functions call dql_avail which returns BQL limit minus number of inflight bytes. These functions can be called without txlock, for instance to ascertain how much data should be dequeued from a qdisc in a batch. When called without the tx_lock, the result is technically a hint, subsequently when the tx_lock is done for a transmit it is possible the availability has changed (for example a transmit completion may have freed up more space in the queue or changed the limit). Signed-off-by: Tom Herbert <therbert@google.com> --- include/linux/netdevice.h | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+)