Skip to contentSkip to conversation

net: add a tx_queue attribute rate_queue_limits in Mbps

RFC PATCH In review John Fastabend 3 participants 5 messages Jun 24, 2013 – Jun 25, 2013

Subsystems (3)
Networking Network drivers Intel Ethernet
lore
Share & tools
lore

John Fastabend

author
#0 lore raw

RFC PATCH 1/1 net: add a tx_queue attribute rate_queue_limits in Mbps

This adds a rate_queue_limit attribute to the tx_queue sysfs entry to allow rate limiting in units of Mpbs. Along with mqprio and BQL this provides another knob to tune queue performance. By default it is disabled with a setting of '0'.

By adding this as a queue attribute and _not_ a qdisc option allows using rate limits with qdisc schemes that may not align with tx rings and also allows using QOS schemes along with rate limits.

A sample implementation is provided for ixgbe. Any improvements or suggestions welcome I would also be interested to know if this works with other hardware and if Mbps is a good default unit.

I tested this briefly with iperf/netperf,

# echo 4000 > /sys/class/net/p3p2/queues/tx-0/tx_rate_limit
# cat /sys/class/net/p3p2/queues/tx-0/tx_rate_limit
4000
Signed-off-by John Fastabend <[email protected]>
---
 drivers/net/ethernet/intel/ixgbe/ixgbe_main.c  |    1 
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c |   47 ++++++++++++++--
 drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h |    2 +
 include/linux/netdevice.h                      |   12 ++++
 net/core/net-sysfs.c                           |   69 +++++++++++++++++++-----
 5 files changed, 110 insertions(+), 21 deletions(-)
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +1 -0
index 047ebaa..8d168a0 100644
@@ -7244,6 +7244,7 @@ static const struct net_device_ops ixgbe_netdev_ops = {
 	.ndo_fdb_add		= ixgbe_ndo_fdb_add,
 	.ndo_bridge_setlink	= ixgbe_ndo_bridge_setlink,
 	.ndo_bridge_getlink	= ixgbe_ndo_bridge_getlink,
+	.ndo_set_ratelimit	= ixgbe_set_rate_limit,
 };
 
 /**
drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c +41 -6

70 lines not on this page · raw message

drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.h +2 -0
index 4713f9f..d8b4bbe 100644
@@ -56,5 +56,7 @@ static inline void ixgbe_set_vmvir(struct ixgbe_adapter *adapter,
 	IXGBE_WRITE_REG(hw, IXGBE_VMVIR(vf), vmvir);
 }
 
+int ixgbe_set_rate_limit(struct net_device *dev, int index, u32 *tx_rate);
+
 #endif /* _IXGBE_SRIOV_H_ */
include/linux/netdevice.h +12 -0
index 09b4188..d84d69a 100644
@@ -574,6 +574,7 @@ struct netdev_queue {
 #ifdef CONFIG_BQL
 	struct dql		dql;
 #endif
+	unsigned long		rate_limit;
 } ____cacheline_aligned_in_smp;
 
 static inline int netdev_queue_numa_node_read(const struct netdev_queue *q)

@@ -932,6 +933,14 @@ struct netdev_fcoe_hbainfo {
  *	that determine carrier state from physical hardware properties (eg
  *	network cables) or protocol-dependent mechanisms (eg
  *	USB_CDC_NOTIFY_NETWORK_CONNECTION) should NOT implement this function.
+ *
+ * int (*ndo_set_ratelimit)(struct net_device *dev,
+ *			    int queue_index, u32 *maxrate)
+ *	Called to set the rate limit in Mpbs specified by maxrate of the
+ *	specified queue_index. It is expected that hardware way may quantize
+ *	the rate limits. In these cases the driver should guarentee the
+ *	specfied maxrate is not exceeded and return the set value in maxrate.
+ *	Zero should be returned on sucess otherwise use appropriate error code.
  */
 struct net_device_ops {
 	int			(*ndo_init)(struct net_device *dev);

@@ -1060,6 +1069,9 @@ struct net_device_ops {
 						      struct nlmsghdr *nlh);
 	int			(*ndo_change_carrier)(struct net_device *dev,
 						      bool new_carrier);
+	int			(*ndo_set_ratelimit)(struct net_device *dev,
+						     int queue_index,
+						     u32 *max_rate);
 };
 
 /*
net/core/net-sysfs.c +54 -15

87 lines not on this page · raw message

#1 lore raw
Quoted 13 lines · On Sun, 23 Jun 2013 20:24:11 -0700 John Fastabend <[email protected]>…

This adds a rate_queue_limit attribute to the tx_queue sysfs entry to allow rate limiting in units of Mpbs. Along with mqprio and BQL this provides another knob to tune queue performance. By default it is disabled with a setting of '0'.

By adding this as a queue attribute and _not_ a qdisc option allows using rate limits with qdisc schemes that may not align with tx rings and also allows using QOS schemes along with rate limits.

A sample implementation is provided for ixgbe. Any improvements or suggestions welcome I would also be interested to know if this works with other hardware and if Mbps is a good default unit.

Quoted 5 lines

I tested this briefly with iperf/netperf,

# echo 4000 > /sys/class/net/p3p2/queues/tx-0/tx_rate_limit
# cat /sys/class/net/p3p2/queues/tx-0/tx_rate_limit
4000

I like the facility and there is a real need for it, but we need to think about what best API for it is.

There are several possible API's for this. My preference in order is:
1. Netlink:
PRO: most well structured and can notify control applications
CON: doesn't propagate well down to device
2. Ethtool:
PRO: fits current model of speed/duplex
CON: still and ioctl based model, non-extensible structures
3. Sysfs
PRO: easy to add
CON: not a general mechanism, ends up being per-device
4. Module parameter
PRO: easy to code
CON: hard to manage from application, hard to associate with multiple devices

author
#2 lore raw
Quoted 22 lines · Stephen Hemminger
Quoted 13 lines · On Sun, 23 Jun 2013 20:24:11 -0700 John Fastabend <[email protected]>…

This adds a rate_queue_limit attribute to the tx_queue sysfs entry to allow rate limiting in units of Mpbs. Along with mqprio and BQL this provides another knob to tune queue performance. By default it is disabled with a setting of '0'.

By adding this as a queue attribute and _not_ a qdisc option allows using rate limits with qdisc schemes that may not align with tx rings and also allows using QOS schemes along with rate limits.

A sample implementation is provided for ixgbe. Any improvements or suggestions welcome I would also be interested to know if this works with other hardware and if Mbps is a good default unit.

Quoted 5 lines

I tested this briefly with iperf/netperf,

# echo 4000 > /sys/class/net/p3p2/queues/tx-0/tx_rate_limit
# cat /sys/class/net/p3p2/queues/tx-0/tx_rate_limit
4000
Quoted 7 lines

I like the facility and there is a real need for it, but we need to think about what best API for it is.

There are several possible API's for this. My preference in order is:
1. Netlink:
PRO: most well structured and can notify control applications
CON: doesn't propagate well down to device

Not sure I understand the con here. We push a lot of netlink messages down to the device already via ndo_ops/rtnetlink.

Having a mechanism in place to notify control applications is nice.

Quoted 3 lines

2. Ethtool:
PRO: fits current model of speed/duplex
CON: still and ioctl based model, non-extensible structures

I prefer netlink or sysfs but this is likely my own bias. Being able to extend the interface easily if/when we get more queue attributes will help in the future.

Quoted 3 lines

3. Sysfs
PRO: easy to add
CON: not a general mechanism, ends up being per-device

There are already entries for other queue attributes here namely byte_queue_limits, tx_timeout, and xps_cpus. And net-sysfs.c keeps all of this reasonable sane IMO and not really device specific. My thought for using this was to keep all the queue attributes in one location.

Quoted 6 lines

4. Module parameter
PRO: easy to code
CON: hard to manage from application, hard to associate with multiple devices

I would prefer Netlink (for notifications) or Sysfs (to keep attributes in one place). I don't really have a strong preference either way.

.John

Signature · 1 line
John Fastabend         Intel Corporation
#3 lore raw
In reply to John Fastabend

On Sun, 2013-06-23 at 20:24 -0700, John Fastabend wrote:
[...]

Quoted 2 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c +0 -0

[...]

drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c static void ixgbe_set_vf_rate_limit(struct ixgbe_adapter *adapter, int vf) · line 1163

Quoted 11 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c +11 -0
+int ixgbe_set_rate_limit(struct net_device *dev, int index, u32 *tx_rate)
+{
+	struct ixgbe_adapter *a = netdev_priv(dev);
+	struct ixgbe_hw *hw = &a->hw;
+	int linkspeed = ixgbe_link_mbps(a);
+	u8 reg_idx = a->tx_ring[index]->reg_idx;
+	u32 bcnrc = ixgbe_bcnrc_from_rate(a, *tx_rate, linkspeed);
+
+	/* rate limit cannot be less than 10Mbs or greater than link speed */
+	if (*tx_rate && ((*tx_rate <= 10) || (*tx_rate > linkspeed)))
+		return -EINVAL;

What if the link is not up?

What if the link speed is currently higher than tx_rate, but is later reduced below tx_rate?

[...]

include/linux/netdevice.h struct netdev_queue { · line 577

Quoted 7 lines
include/linux/netdevice.h +1 -0
@@ -574,6 +574,7 @@ struct netdev_queue {
 #ifdef CONFIG_BQL
 	struct dql		dql;
 #endif
+	unsigned long		rate_limit;

Should be u32, consistent with variables in ndo_set_ratelimit and set_tx_rate_limit.

Quoted 5 lines
include/linux/netdevice.h +0 -0
 } ____cacheline_aligned_in_smp;
 
 static inline int netdev_queue_numa_node_read(const struct netdev_queue *q)

@@ -932,6 +933,14 @@ struct netdev_fcoe_hbainfo {
  *	that determine carrier state from physical hardware properties (eg

include/linux/netdevice.h struct netdev_fcoe_hbainfo { · line 943

Quoted 10 lines
include/linux/netdevice.h +8 -0
  *	network cables) or protocol-dependent mechanisms (eg
  *	USB_CDC_NOTIFY_NETWORK_CONNECTION) should NOT implement this function.
+ *
+ * int (*ndo_set_ratelimit)(struct net_device *dev,
+ *			    int queue_index, u32 *maxrate)
+ *	Called to set the rate limit in Mpbs specified by maxrate of the
+ *	specified queue_index. It is expected that hardware way may quantize
+ *	the rate limits. In these cases the driver should guarentee the
+ *	specfied maxrate is not exceeded and return the set value in maxrate.
+ *	Zero should be returned on sucess otherwise use appropriate error code.

[...]

In general, how does maxrate interact with link speed?

There are also several spelling and grammar errors in this text.

Ben.

Signature · 3 lines
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
author
#4 lore raw
Quoted 5 lines · Ben Hutchings

On Sun, 2013-06-23 at 20:24 -0700, John Fastabend wrote:
[...]

Quoted 2 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c +0 -0

[...]

Quoted 14 lines

drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c static void ixgbe_set_vf_rate_limit(struct ixgbe_adapter *adapter, int vf) · line 1163

Quoted 11 lines
drivers/net/ethernet/intel/ixgbe/ixgbe_sriov.c +11 -0
+int ixgbe_set_rate_limit(struct net_device *dev, int index, u32 *tx_rate)
+{
+	struct ixgbe_adapter *a = netdev_priv(dev);
+	struct ixgbe_hw *hw = &a->hw;
+	int linkspeed = ixgbe_link_mbps(a);
+	u8 reg_idx = a->tx_ring[index]->reg_idx;
+	u32 bcnrc = ixgbe_bcnrc_from_rate(a, *tx_rate, linkspeed);
+
+	/* rate limit cannot be less than 10Mbs or greater than link speed */
+	if (*tx_rate && ((*tx_rate <= 10) || (*tx_rate > linkspeed)))
+		return -EINVAL;

What if the link is not up?

As its written this would always be an error because link_speed is 0. Not likely what we want.

Quoted 2 lines

What if the link speed is currently higher than tx_rate, but is later reduced below tx_rate?

As its written you just end up with a tx_rate limit that is broken and because the hw calculation depends on link speed the value won't be correct any more.

Both of these cases are probably broken as is.

Quoted 30 lines

[...]

include/linux/netdevice.h struct netdev_queue { · line 577

Quoted 7 lines
include/linux/netdevice.h +1 -0
@@ -574,6 +574,7 @@ struct netdev_queue {
  #ifdef CONFIG_BQL
  	struct dql		dql;
  #endif
+	unsigned long		rate_limit;

Should be u32, consistent with variables in ndo_set_ratelimit and set_tx_rate_limit.

Quoted 15 lines
include/linux/netdevice.h +8 -0
  } ____cacheline_aligned_in_smp;
 
  static inline int netdev_queue_numa_node_read(const struct netdev_queue *q)

@@ -932,6 +933,14 @@ struct netdev_fcoe_hbainfo {
   *	that determine carrier state from physical hardware properties (eg
   *	network cables) or protocol-dependent mechanisms (eg
   *	USB_CDC_NOTIFY_NETWORK_CONNECTION) should NOT implement this function.
+ *
+ * int (*ndo_set_ratelimit)(struct net_device *dev,
+ *			    int queue_index, u32 *maxrate)
+ *	Called to set the rate limit in Mpbs specified by maxrate of the
+ *	specified queue_index. It is expected that hardware way may quantize
+ *	the rate limits. In these cases the driver should guarentee the
+ *	specfied maxrate is not exceeded and return the set value in maxrate.
+ *	Zero should be returned on sucess otherwise use appropriate error code.

[...]

Quoted 1 line

In general, how does maxrate interact with link speed?

I think the best approach is to allow a max_rate that is greater than the link speed. This seems more straight forward than arbitrarily resetting the value or doing something else.

ixgbe and maybe other drivers will need to reconfigure their internal hardware limiters if they are derived from link speed.

Quoted 3 lines

There are also several spelling and grammar errors in this text.

Yeah, must have been late, not enough coffee. Thanks for looking at it Ben.

Also I was thinking about pushing it over to netlink as Stephen preferred.

Signature · 1 line
John Fastabend         Intel Corporation