Skip to contentSkip to conversation

NET: Multiple queue hardware support

PATCH New PJ Waskiewicz 2 participants 14 messages Jun 23, 2007 – Jun 25, 2007

Subsystems (4)
Networking Network drivers Documentation Traffic control (tc)
lore
Share & tools
lore

PJ Waskiewicz

author
#0 lore raw

PATCH 1/1 NET: Multiple queue hardware support

Please consider these patches for 2.6.23 inclusion.

These patches are built against Patrick McHardy's recently submitted RTNETLINK nested compat attribute patches. They're needed to preserve ABI between sch_{rr|prio} and iproute2.

Updates since the last submission:

1. Added checks for netif_subqueue_stopped() to net/core/netpoll.c, net/core/pktgen.c, and to software device hard_start_xmit in dev_queue_xmit().

2. Removed TCA_PRIO_TEST and added TCA_PRIO_MQ for sch_prio and sch_rr.

3. Fixed dependancy issues in net/sched/Kconfig with NET_SCH_RR.

4. Implemented the new nested compat attribute API for MQ in NET_SCH_PRIO and NET_SCH_RR.

5. Allow sch_rr and sch_prio to turn multiqueue hardware support on and off at loadtime.

This patchset is an updated version of previous multiqueue network device support patches. The general approach of introducing a new API for multiqueue network devices to register with the stack has remained. The changes include adding a round-robin qdisc, heavily based on sch_prio, which will allow queueing to hardware with no OS-enforced queuing policy. sch_prio still has the multiqueue code in it, but has a Kconfig option to compile it out of the qdisc. This allows people with hardware containing scheduling policies to use sch_rr (round-robin), and others without scheduling policies in hardware to continue using sch_prio if they wish to have some notion of scheduling priority.

The patches being sent are split into Documentation, Qdisc changes, and core stack changes. The requested e1000 changes are still being resolved, and will be sent at a later date.

The patches to iproute2 for tc will be sent separately, to support sch_rr.

Signature · 1 line
PJ Waskiewicz <[email protected]>
author
#1 lore raw

PATCH 1/3 NET: [DOC] Multiqueue hardware support documentation

Add a brief howto to Documentation/networking for multiqueue. It explains how to use the multiqueue API in a driver to support multiqueue paths from the stack, as well as the qdiscs to use for feeding a multiqueue device.

Signed-off-by Peter P Waskiewicz Jr <[email protected]>

---

 Documentation/networking/multiqueue.txt |  106 +++++++++++++++++++++++++++++++
 1 files changed, 106 insertions(+), 0 deletions(-)
Documentation/networking/multiqueue.txt new file +106 -0
new file mode 100644
index 0000000..b7ede56
@@ -0,0 +1,106 @@
+
+		HOWTO for multiqueue network device support
+		===========================================
+
+Section 1: Base driver requirements for implementing multiqueue support
+Section 2: Qdisc support for multiqueue devices
+Section 3: Brief howto using PRIO or RR for multiqueue devices
+
+
+Intro: Kernel support for multiqueue devices
+---------------------------------------------------------
+
+Kernel support for multiqueue devices is only an API that is presented to the
+netdevice layer for base drivers to implement.  This feature is part of the
+core networking stack, and all network devices will be running on the
+multiqueue-aware stack.  If a base driver only has one queue, then these
+changes are transparent to that driver.
+
+
+Section 1: Base driver requirements for implementing multiqueue support
+-----------------------------------------------------------------------
+
+Base drivers are required to use the new alloc_etherdev_mq() or
+alloc_netdev_mq() functions to allocate the subqueues for the device.  The
+underlying kernel API will take care of the allocation and deallocation of
+the subqueue memory, as well as netdev configuration of where the queues
+exist in memory.
+
+The base driver will also need to manage the queues as it does the global
+netdev->queue_lock today.  Therefore base drivers should use the
+netif_{start|stop|wake}_subqueue() functions to manage each queue while the
+device is still operational.  netdev->queue_lock is still used when the device
+comes online or when it's completely shut down (unregister_netdev(), etc.).
+
+Finally, the base driver should indicate that it is a multiqueue device.  The
+feature flag NETIF_F_MULTI_QUEUE should be added to the netdev->features
+bitmap on device initialization.  Below is an example from e1000:
+
+#ifdef CONFIG_E1000_MQ
+	if ( (adapter->hw.mac.type == e1000_82571) ||
+	     (adapter->hw.mac.type == e1000_82572) ||
+	     (adapter->hw.mac.type == e1000_80003es2lan))
+		netdev->features |= NETIF_F_MULTI_QUEUE;
+#endif
+
+
+Section 2: Qdisc support for multiqueue devices
+-----------------------------------------------
+
+Currently two qdiscs support multiqueue devices.  A new round-robin qdisc,
+sch_rr, and sch_prio. The qdisc is responsible for classifying the skb's to
+bands and queues, and will store the queue mapping into skb->queue_mapping.
+Use this field in the base driver to determine which queue to send the skb
+to.
+
+sch_rr has been added for hardware that doesn't want scheduling policies from
+software, so it's a straight round-robin qdisc.  It uses the same syntax and
+classification priomap that sch_prio uses, so it should be intuitive to
+configure for people who've used sch_prio.
+
+The PRIO qdisc naturally plugs into a multiqueue device.  If PRIO has been
+built with NET_SCH_PRIO_MQ, then upon load, it will make sure the number of
+bands requested is equal to the number of queues on the hardware.  If they
+are equal, it sets a one-to-one mapping up between the queues and bands.  If
+they're not equal, it will not load the qdisc.  This is the same behavior
+for RR.  Once the association is made, any skb that is classified will have
+skb->queue_mapping set, which will allow the driver to properly queue skb's
+to multiple queues.
+
+
+Section 3: Brief howto using PRIO and RR for multiqueue devices
+---------------------------------------------------------------
+
+The userspace command 'tc,' part of the iproute2 package, is used to configure
+qdiscs.  To add the PRIO qdisc to your network device, assuming the device is
+called eth0, run the following command:
+
+# tc qdisc add dev eth0 root handle 1: prio bands 4 multiqueue
+
+This will create 4 bands, 0 being highest priority, and associate those bands
+to the queues on your NIC.  Assuming eth0 has 4 Tx queues, the band mapping
+would look like:
+
+band 0 => queue 0
+band 1 => queue 1
+band 2 => queue 2
+band 3 => queue 3
+
+Traffic will begin flowing through each queue if your TOS values are assigning
+traffic across the various bands.  For example, ssh traffic will always try to
+go out band 0 based on TOS -> Linux priority conversion (realtime traffic),
+so it will be sent out queue 0.  ICMP traffic (pings) fall into the "normal"
+traffic classification, which is band 1.  Therefore pings will be send out
+queue 1 on the NIC.
+
+Note the use of the multiqueue keyword.  This is only in versions of iproute2
+that support multiqueue networking devices; if this is omitted when loading
+a qdisc onto a multiqueue device, the qdisc will load and operate the same
+if it were loaded onto a single-queue device (i.e. - sends all traffic to
+queue 0).
+
+The behavior of tc filters remains the same, where it will override TOS priority
+classification.
+
+
+Author: Peter P. Waskiewicz Jr. <[email protected]>
author
#2 lore raw

PATCH 2/3 NET: [CORE] Stack changes to add multiqueue hardware support API

In reply to PJ Waskiewicz

Updated: Added checks for netif_subqueue_stopped() to netpoll, pktgen, and software device dev_queue_xmit(). This will ensure external events to these subsystems will be handled correctly if a subqueue is shut down.

Add the multiqueue hardware device support API to the core network stack. Allow drivers to allocate multiple queues and manage them at the netdev level if they choose to do so.

Added a new field to sk_buff, namely queue_mapping, for drivers to know which tx_ring to select based on OS classification of the flow.

Signed-off-by Peter P Waskiewicz Jr <[email protected]>

---

 include/linux/etherdevice.h |    3 +-
 include/linux/netdevice.h   |   62 ++++++++++++++++++++++++++++++++++++++++++-
 include/linux/skbuff.h      |    4 ++-
 net/core/dev.c              |   27 +++++++++++++------
 net/core/netpoll.c          |    8 +++---
 net/core/pktgen.c           |   10 +++++--
 net/core/skbuff.c           |    3 ++
 net/ethernet/eth.c          |    9 +++---
 8 files changed, 104 insertions(+), 22 deletions(-)
include/linux/etherdevice.h +2 -1
index f48eb89..b3fbb54 100644
@@ -39,7 +39,8 @@ extern void		eth_header_cache_update(struct hh_cache *hh, struct net_device *dev
 extern int		eth_header_cache(struct neighbour *neigh,
 					 struct hh_cache *hh);
 
-extern struct net_device *alloc_etherdev(int sizeof_priv);
+extern struct net_device *alloc_etherdev_mq(int sizeof_priv, int queue_count);
+#define alloc_etherdev(sizeof_priv) alloc_etherdev_mq(sizeof_priv, 1)
 
 /**
  * is_zero_ether_addr - Determine if give Ethernet address is all zeros.
include/linux/netdevice.h +60 -2

92 lines not on this page · raw message

include/linux/skbuff.h +3 -1
index e7367c7..01b5e25 100644
@@ -197,6 +197,7 @@ typedef unsigned char *sk_buff_data_t;
  *	@tstamp: Time we arrived
  *	@dev: Device we arrived on/are leaving by
  *	@iif: ifindex of device we arrived on
+ *	@queue_mapping: Queue mapping for multiqueue devices
  *	@transport_header: Transport layer header
  *	@network_header: Network layer header
  *	@mac_header: Link layer header

@@ -246,7 +247,8 @@ struct sk_buff {
 	ktime_t			tstamp;
 	struct net_device	*dev;
 	int			iif;
-	/* 4 byte hole on 64 bit*/
+	__u16			queue_mapping;
+	/* 2 byte hole on 64 bit*/
 
 	struct  dst_entry	*dst;
 	struct	sec_path	*sp;
net/core/dev.c +19 -8
index 2609062..9ea8a47 100644
@@ -1429,7 +1429,9 @@ gso:
 			skb->next = nskb;
 			return rc;
 		}
-		if (unlikely(netif_queue_stopped(dev) && skb->next))
+		if (unlikely((netif_queue_stopped(dev) ||
+			     netif_subqueue_stopped(dev, skb->queue_mapping)) &&
+			     skb->next))
 			return NETDEV_TX_BUSY;
 	} while (skb->next);
 

@@ -1545,6 +1547,8 @@ gso:
 		spin_lock(&dev->queue_lock);
 		q = dev->qdisc;
 		if (q->enqueue) {
+			/* reset queue_mapping to zero */
+			skb->queue_mapping = 0;
 			rc = q->enqueue(skb, q);
 			qdisc_run(dev);
 			spin_unlock(&dev->queue_lock);

@@ -1574,7 +1578,8 @@ gso:
 
 			HARD_TX_LOCK(dev, cpu);
 
-			if (!netif_queue_stopped(dev)) {
+			if (!netif_queue_stopped(dev) &&
+			    !netif_subqueue_stopped(dev, skb->queue_mapping)) {
 				rc = 0;
 				if (!dev_hard_start_xmit(skb, dev)) {
 					HARD_TX_UNLOCK(dev);

@@ -3343,16 +3348,18 @@ static struct net_device_stats *internal_stats(struct net_device *dev)
 }
 
 /**
- *	alloc_netdev - allocate network device
+ *	alloc_netdev_mq - allocate network device
  *	@sizeof_priv:	size of private data to allocate space for
  *	@name:		device name format string
  *	@setup:		callback to initialize device
+ *	@queue_count:	the number of subqueues to allocate
  *
  *	Allocates a struct net_device with private data area for driver use
- *	and performs basic initialization.
+ *	and performs basic initialization.  Also allocates subqueue structs
+ *	for each queue on the device at the end of the netdevice.
  */
-struct net_device *alloc_netdev(int sizeof_priv, const char *name,
-		void (*setup)(struct net_device *))
+struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
+		void (*setup)(struct net_device *), int queue_count)
 {
 	void *p;
 	struct net_device *dev;

@@ -3361,7 +3368,9 @@ struct net_device *alloc_netdev(int sizeof_priv, const char *name,
 	BUG_ON(strlen(name) >= sizeof(dev->name));
 
 	/* ensure 32-byte alignment of both the device and private area */
-	alloc_size = (sizeof(*dev) + NETDEV_ALIGN_CONST) & ~NETDEV_ALIGN_CONST;
+	alloc_size = (sizeof(*dev) + NETDEV_ALIGN_CONST +
+		     (sizeof(struct net_device_subqueue) * (queue_count - 1))) &
+		     ~NETDEV_ALIGN_CONST;
 	alloc_size += sizeof_priv + NETDEV_ALIGN_CONST;
 
 	p = kzalloc(alloc_size, GFP_KERNEL);

@@ -3377,12 +3386,14 @@ struct net_device *alloc_netdev(int sizeof_priv, const char *name,
 	if (sizeof_priv)
 		dev->priv = netdev_priv(dev);
 
+  	dev->egress_subqueue_count = queue_count;
+
 	dev->get_stats = internal_stats;
 	setup(dev);
 	strcpy(dev->name, name);
 	return dev;
 }
-EXPORT_SYMBOL(alloc_netdev);
+EXPORT_SYMBOL(alloc_netdev_mq);
 
 /**
  *	free_netdev - free network device
net/core/netpoll.c +5 -3
index 758dafe..aac8acf 100644
@@ -66,8 +66,9 @@ static void queue_process(struct work_struct *work)
 
 		local_irq_save(flags);
 		netif_tx_lock(dev);
-		if (netif_queue_stopped(dev) ||
-		    dev->hard_start_xmit(skb, dev) != NETDEV_TX_OK) {
+		if ((netif_queue_stopped(dev) || 
+		     netif_subqueue_stopped(dev, skb->queue_mapping)) ||
+		     dev->hard_start_xmit(skb, dev) != NETDEV_TX_OK) {
 			skb_queue_head(&npinfo->txq, skb);
 			netif_tx_unlock(dev);
 			local_irq_restore(flags);

@@ -254,7 +255,8 @@ static void netpoll_send_skb(struct netpoll *np, struct sk_buff *skb)
 			/* try until next clock tick */
 			for (tries = jiffies_to_usecs(1)/USEC_PER_POLL;
 					tries > 0; --tries) {
-				if (!netif_queue_stopped(dev))
+				if (!netif_queue_stopped(dev) &&
+				    !netif_subqueue_stopped(dev, skb->queue_mapping))
 					status = dev->hard_start_xmit(skb, dev);
 
 				if (status == NETDEV_TX_OK)
net/core/pktgen.c +7 -3
index 9cd3a1c..dffe067 100644
@@ -3139,7 +3139,9 @@ static __inline__ void pktgen_xmit(struct pktgen_dev *pkt_dev)
 		}
 	}
 
-	if (netif_queue_stopped(odev) || need_resched()) {
+	if ((netif_queue_stopped(odev) ||
+	     netif_subqueue_stopped(odev, pkt_dev->skb->queue_mapping)) ||
+	     need_resched()) {
 		idle_start = getCurUs();
 
 		if (!netif_running(odev)) {

@@ -3154,7 +3156,8 @@ static __inline__ void pktgen_xmit(struct pktgen_dev *pkt_dev)
 
 		pkt_dev->idle_acc += getCurUs() - idle_start;
 
-		if (netif_queue_stopped(odev)) {
+		if (netif_queue_stopped(odev) ||
+		    netif_subqueue_stopped(odev, pkt_dev->skb->queue_mapping)) {
 			pkt_dev->next_tx_us = getCurUs();	/* TODO */
 			pkt_dev->next_tx_ns = 0;
 			goto out;	/* Try the next interface */

@@ -3181,7 +3184,8 @@ static __inline__ void pktgen_xmit(struct pktgen_dev *pkt_dev)
 	}
 
 	netif_tx_lock_bh(odev);
-	if (!netif_queue_stopped(odev)) {
+	if (!netif_queue_stopped(odev) &&
+	    !netif_subqueue_stopped(odev, pkt_dev->skb->queue_mapping)) {
 
 		atomic_inc(&(pkt_dev->skb->users));
 	      retry_now:
net/core/skbuff.c +3 -0
index 7c6a34e..7bbed45 100644
@@ -418,6 +418,7 @@ struct sk_buff *skb_clone(struct sk_buff *skb, gfp_t gfp_mask)
 	n->nohdr = 0;
 	C(pkt_type);
 	C(ip_summed);
+	C(queue_mapping);
 	C(priority);
 #if defined(CONFIG_IP_VS) || defined(CONFIG_IP_VS_MODULE)
 	C(ipvs_property);

@@ -459,6 +460,7 @@ static void copy_skb_header(struct sk_buff *new, const struct sk_buff *old)
 #endif
 	new->sk		= NULL;
 	new->dev	= old->dev;
+	new->queue_mapping = old->queue_mapping;
 	new->priority	= old->priority;
 	new->protocol	= old->protocol;
 	new->dst	= dst_clone(old->dst);

@@ -1925,6 +1927,7 @@ struct sk_buff *skb_segment(struct sk_buff *skb, int features)
 		tail = nskb;
 
 		nskb->dev = skb->dev;
+		nskb->queue_mapping = skb->queue_mapping;
 		nskb->priority = skb->priority;
 		nskb->protocol = skb->protocol;
 		nskb->dst = dst_clone(skb->dst);
net/ethernet/eth.c +5 -4
index 0ac2524..87a509c 100644
@@ -316,9 +316,10 @@ void ether_setup(struct net_device *dev)
 EXPORT_SYMBOL(ether_setup);
 
 /**
- * alloc_etherdev - Allocates and sets up an Ethernet device
+ * alloc_etherdev_mq - Allocates and sets up an Ethernet device
  * @sizeof_priv: Size of additional driver-private structure to be allocated
  *	for this Ethernet device
+ * @queue_count: The number of queues this device has.
  *
  * Fill in the fields of the device structure with Ethernet-generic
  * values. Basically does everything except registering the device.

@@ -328,8 +329,8 @@ EXPORT_SYMBOL(ether_setup);
  * this private data area.
  */
 
-struct net_device *alloc_etherdev(int sizeof_priv)
+struct net_device *alloc_etherdev_mq(int sizeof_priv, int queue_count)
 {
-	return alloc_netdev(sizeof_priv, "eth%d", ether_setup);
+	return alloc_netdev_mq(sizeof_priv, "eth%d", ether_setup, queue_count);
 }
-EXPORT_SYMBOL(alloc_etherdev);
+EXPORT_SYMBOL(alloc_etherdev_mq);
#3 lore raw

Re: [PATCH 2/3] NET: [CORE] Stack changes to add multiqueue hardware support API

net/core/dev.c struct net_device *alloc_netdev(int sizeof_priv, const char *name, · line 3372

Quoted 12 lines · PJ Waskiewicz
net/core/dev.c +4 -1
+struct net_device *alloc_netdev_mq(int sizeof_priv, const char *name,
+		void (*setup)(struct net_device *), int queue_count)
 {
 	void *p;
 	struct net_device *dev;

@@ -3361,7 +3368,9 @@ struct net_device *alloc_netdev(int sizeof_priv, const char *name,
 	BUG_ON(strlen(name) >= sizeof(dev->name));
 
 	/* ensure 32-byte alignment of both the device and private area */
-	alloc_size = (sizeof(*dev) + NETDEV_ALIGN_CONST) & ~NETDEV_ALIGN_CONST;
+	alloc_size = (sizeof(*dev) + NETDEV_ALIGN_CONST +
+		     (sizeof(struct net_device_subqueue) * (queue_count - 1))) &

Why queue_count - 1 ? It should be queue_count I think.

Otherwise ACK for this patch except that it should also contain the sch_generic changes.

author
#4 lore raw

RE: [PATCH 2/3] NET: [CORE] Stack changes to add multiqueue hardware support API

Quoted 10 lines
Quoted 1 line

/* ensure 32-byte alignment of both the device and

private area */

Review

Quoted 1 line
(quoted diff) +0 -1
-	alloc_size = (sizeof(*dev) + NETDEV_ALIGN_CONST) &

~NETDEV_ALIGN_CONST;

Review

Quoted 2 lines
(quoted diff) +2 -0
+	alloc_size = (sizeof(*dev) + NETDEV_ALIGN_CONST +
+		     (sizeof(struct net_device_subqueue) *

(queue_count - 1))) &

Why queue_count - 1 ? It should be queue_count I think.

I'm not sure what went through my head, but I'll fix this.

Quoted 2 lines

Otherwise ACK for this patch except that it should also contain the sch_generic changes.

I misread your previous mail; I'll get the sch_generic.c changes into this patch.

Thanks Patrick,
-PJ

author
#5 lore raw

PATCH 3/3 NET: [SCHED] Qdisc changes and sch_rr added for multiqueue

In reply to PJ Waskiewicz

Updated: This patch applies on top of Patrick McHardy's RTNETLINK nested compat attribute patches. These are required to preserve ABI for iproute2 when working with the multiqueue qdiscs.

Add the new sch_rr qdisc for multiqueue network device support. Allow sch_prio and sch_rr to be compiled with or without multiqueue hardware support.

sch_rr is part of sch_prio, and is referenced from MODULE_ALIAS. This was done since sch_prio and sch_rr only differ in their dequeue routine.

Signed-off-by Peter P Waskiewicz Jr <[email protected]>

---

 include/linux/pkt_sched.h |    4 +-
 net/sched/Kconfig         |   30 +++++++++++++
 net/sched/sch_generic.c   |    3 +
 net/sched/sch_prio.c      |  106 ++++++++++++++++++++++++++++++++++++++++-----
 4 files changed, 129 insertions(+), 14 deletions(-)
include/linux/pkt_sched.h +2 -2
index 09808b7..ec3a9a5 100644
@@ -103,8 +103,8 @@ struct tc_prio_qopt
 
 enum
 {
-	TCA_PRIO_UNPSEC,
-	TCA_PRIO_TEST,
+	TCA_PRIO_UNSPEC,
+	TCA_PRIO_MQ,
 	__TCA_PRIO_MAX
 };
net/sched/Kconfig +30 -0
index 475df84..7f14fa6 100644
@@ -102,8 +102,16 @@ config NET_SCH_ATM
 	  To compile this code as a module, choose M here: the
 	  module will be called sch_atm.
 
+config NET_SCH_BANDS
+        bool "Multi Band Queueing (PRIO and RR)"
+        ---help---
+          Say Y here if you want to use n-band multiqueue packet
+          schedulers.  These include a priority-based scheduler and
+	   a round-robin scheduler.
+
 config NET_SCH_PRIO
 	tristate "Multi Band Priority Queueing (PRIO)"
+	depends on NET_SCH_BANDS
 	---help---
 	  Say Y here if you want to use an n-band priority queue packet
 	  scheduler.

@@ -111,6 +119,28 @@ config NET_SCH_PRIO
 	  To compile this code as a module, choose M here: the
 	  module will be called sch_prio.
 
+config NET_SCH_RR
+	tristate "Multi Band Round Robin Queuing (RR)"
+	depends on NET_SCH_BANDS
+	select NET_SCH_PRIO
+	---help---
+	  Say Y here if you want to use an n-band round robin packet
+	  scheduler.
+
+	  The module uses sch_prio for its framework and is aliased as
+	  sch_rr, so it will load sch_prio, although it is referred
+	  to using sch_rr.
+
+config NET_SCH_BANDS_MQ
+	bool "Multiple hardware queue support"
+	depends on NET_SCH_BANDS
+	---help---
+	  Say Y here if you want to allow the PRIO and RR qdiscs to assign
+	  flows to multiple hardware queues on an ethernet device.  This
+	  will still work on devices with 1 queue.
+
+	  Most people will say N here.
+
 config NET_SCH_RED
 	tristate "Random Early Detection (RED)"
 	---help---
net/sched/sch_generic.c +2 -1
index 9461e8a..203d5c4 100644
@@ -168,7 +168,8 @@ static inline int qdisc_restart(struct net_device *dev)
 	spin_unlock(&dev->queue_lock);
 
 	ret = NETDEV_TX_BUSY;
-	if (!netif_queue_stopped(dev))
+	if (!netif_queue_stopped(dev) &&
+	    !netif_subqueue_stopped(dev, skb->queue_mapping))
 		/* churn baby churn .. */
 		ret = dev_hard_start_xmit(skb, dev);
net/sched/sch_prio.c +95 -11

174 lines not on this page · raw message

#6 lore raw

Re: [PATCH 3/3] NET: [SCHED] Qdisc changes and sch_rr added for multiqueue

include/linux/pkt_sched.h struct tc_prio_qopt · line 107

Quoted 10 lines · PJ Waskiewicz
include/linux/pkt_sched.h +0 -2
index 09808b7..ec3a9a5 100644
@@ -103,8 +103,8 @@ struct tc_prio_qopt
 
 enum
 {
-	TCA_PRIO_UNPSEC,
-	TCA_PRIO_TEST,

You misunderstood me. You can work on top of my compat attribute patches, but the example code should not have to go in to apply your patch.

net/sched/Kconfig config NET_SCH_ATM · line 106

Quoted 10 lines
net/sched/Kconfig +2 -0
index 475df84..7f14fa6 100644
@@ -102,8 +102,16 @@ config NET_SCH_ATM
 	  To compile this code as a module, choose M here: the
 	  module will be called sch_atm.
 
+config NET_SCH_BANDS
+        bool "Multi Band Queueing (PRIO and RR)"

This options seems useless. Its not used *anywhere* except for dependencies.

net/sched/Kconfig config NET_SCH_ATM · line 114

Quoted 8 lines
net/sched/Kconfig +6 -0
+        ---help---
+          Say Y here if you want to use n-band multiqueue packet
+          schedulers.  These include a priority-based scheduler and
+	   a round-robin scheduler.
+
 config NET_SCH_PRIO
 	tristate "Multi Band Priority Queueing (PRIO)"
+	depends on NET_SCH_BANDS

And this dependency as well.

net/sched/Kconfig config NET_SCH_PRIO · line 124

Quoted 10 lines
net/sched/Kconfig +3 -0
 	---help---
 	  Say Y here if you want to use an n-band priority queue packet
 	  scheduler.

@@ -111,6 +119,28 @@ config NET_SCH_PRIO
 	  To compile this code as a module, choose M here: the
 	  module will be called sch_prio.
 
+config NET_SCH_RR
+	tristate "Multi Band Round Robin Queuing (RR)"
+	depends on NET_SCH_BANDS

Same here. RR

net/sched/Kconfig config NET_SCH_PRIO · line 136

Quoted 12 lines
net/sched/Kconfig +12 -0
+	select NET_SCH_PRIO
+	---help---
+	  Say Y here if you want to use an n-band round robin packet
+	  scheduler.
+
+	  The module uses sch_prio for its framework and is aliased as
+	  sch_rr, so it will load sch_prio, although it is referred
+	  to using sch_rr.
+
+config NET_SCH_BANDS_MQ
+	bool "Multiple hardware queue support"
+	depends on NET_SCH_BANDS

OK, again:

Introduce NET_SCH_RR. NET_SCH_RR selects NET_SCH_PRIO. Nothing at all changes for NET_SCH_PRIO itself. Additionally introduce a boolean NET_SCH_MULTIQUEUE. No dependencies at all. Use NET_SCH_MULTIQUEUE to guard the multiqueue code in sch_prio.c. Your current code doesn't even have any ifdefs anymore though, so this might not be needed at all.

Additionally you could later introduce E1000_MULTIQUEUE and have that select NET_SCH_MULTIQUEUE.

net/sched/sch_generic.c static inline int qdisc_restart(struct net_device *dev) · line 174

Quoted 13 lines
net/sched/sch_generic.c +2 -1
index 9461e8a..203d5c4 100644
@@ -168,7 +168,8 @@ static inline int qdisc_restart(struct net_device *dev)
 	spin_unlock(&dev->queue_lock);
 
 	ret = NETDEV_TX_BUSY;
-	if (!netif_queue_stopped(dev))
+	if (!netif_queue_stopped(dev) &&
+	    !netif_subqueue_stopped(dev, skb->queue_mapping))
 		/* churn baby churn .. */
 		ret = dev_hard_start_xmit(skb, dev);

I'll try again - please move this to patch 2/3.

Quoted 17 lines
net/sched/sch_prio.c +2 -0
index 40a13e8..8a716f0 100644
@@ -40,9 +40,11 @@
 struct prio_sched_data
 {
 	int bands;
+	int curband; /* for round-robin */
 	struct tcf_proto *filter_list;
 	u8  prio2band[TC_PRIO_MAX+1];
 	struct Qdisc *queues[TCQ_PRIO_BANDS];
+	unsigned char mq;
 };

net/sched/sch_prio.c prio_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr) · line 79

Quoted 8 lines
net/sched/sch_prio.c +5 -0
@@ -70,14 +72,28 @@ prio_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr)
 #endif
 			if (TC_H_MAJ(band))
 				band = 0;
+			if (q->mq)
+				skb->queue_mapping = 
+						q->prio2band[band&TC_PRIO_MAX];
+			else
+				skb->queue_mapping = 0;

Might look cleaner if you have one central point where queue_mapping is set and the band is returned.

net/sched/sch_prio.c static int prio_tune(struct Qdisc *sch, struct rtattr *opt) · line 276

Quoted 5 lines
net/sched/sch_prio.c +5 -0
+	/* If we're multiqueue, make sure the number of incoming bands
+	 * matches the number of queues on the device we're associating with.
+	 */
+	if (tb[TCA_PRIO_MQ - 1])
+		q->mq = *(unsigned char *)RTA_DATA(tb[TCA_PRIO_MQ - 1]);

If you're using it as a flag, please use RTA_GET_FLAG(), otherwise RTA_GET_U8.

net/sched/sch_prio.c static int prio_dump(struct Qdisc *sch, struct sk_buff *skb) · line 345

Quoted 11 lines
net/sched/sch_prio.c +3 -1
+	if (q->mq && (qopt->bands != sch->dev->egress_subqueue_count))
+		return -EINVAL;
 
 	sch_tree_lock(sch);
 	q->bands = qopt->bands;

@@ -280,7 +342,7 @@ static int prio_dump(struct Qdisc *sch, struct sk_buff *skb)
 	memcpy(&opt.priomap, q->prio2band, TC_PRIO_MAX+1);
 
 	nest = RTA_NEST_COMPAT(skb, TCA_OPTIONS, sizeof(opt), &opt);
-	RTA_PUT_U32(skb, TCA_PRIO_TEST, 321);
+	RTA_PUT_U8(skb, TCA_PRIO_MQ, q->mq);

And RTA_PUT_FLAG. Now that I think of it, does it even makes sense to have a prio private flag for this instead of a qdisc global one?

net/sched/sch_prio.c static struct Qdisc_ops prio_qdisc_ops = { · line 537

Quoted 5 lines
net/sched/sch_prio.c +2 -1
 static int __init prio_module_init(void)
 {
-	return register_qdisc(&prio_qdisc_ops);
+	register_qdisc(&prio_qdisc_ops);
+	register_qdisc(&rr_qdisc_ops);

Proper error handling please.

author
#7 lore raw

RE: [PATCH 3/3] NET: [SCHED] Qdisc changes and sch_rr added for multiqueue

Quoted 9 lines

include/linux/pkt_sched.h struct tc_prio_qopt · line 107

Quoted 4 lines

enum
{

include/linux/pkt_sched.h +0 -2
-	TCA_PRIO_UNPSEC,
-	TCA_PRIO_TEST,

You misunderstood me. You can work on top of my compat attribute patches, but the example code should not have to go in to apply your patch.

Ok. I'll fix my patches.

Quoted 8 lines
Quoted 8 lines
net/sched/Kconfig index renamed from net/sched/Kconfig +0 -0

475df84..7f14fa6 100644

net/sched/Kconfig +0 -0
@@ -102,8 +102,16 @@ config NET_SCH_ATM
 	  To compile this code as a module, choose M here: the
 	  module will be called sch_atm.
Quoted 5 lines

net/sched/Kconfig config NET_SCH_ATM · line 106

Quoted 2 lines
net/sched/Kconfig +2 -0
+config NET_SCH_BANDS
+        bool "Multi Band Queueing (PRIO and RR)"

This options seems useless. Its not used *anywhere* except for dependencies.

I was trying to group the multiqueue qdiscs together with this. But I can see just having the multiqueue option for scheduling will cover this. I'll remove this.

Quoted 15 lines

net/sched/Kconfig config NET_SCH_PRIO · line 136

Quoted 3 lines
net/sched/Kconfig +3 -0
+config NET_SCH_BANDS_MQ
+	bool "Multiple hardware queue support"
+	depends on NET_SCH_BANDS

OK, again:

Introduce NET_SCH_RR. NET_SCH_RR selects NET_SCH_PRIO. Nothing at all changes for NET_SCH_PRIO itself. Additionally introduce a boolean NET_SCH_MULTIQUEUE. No dependencies at all. Use NET_SCH_MULTIQUEUE to guard the multiqueue code in sch_prio.c.
Your current code doesn't even have any ifdefs anymore though, so this might not be needed at all.

Quoted 2 lines

Additionally you could later introduce E1000_MULTIQUEUE and have that select NET_SCH_MULTIQUEUE.

I'll clean this up. Thanks for the persistance. :)

Quoted 9 lines

Review

Quoted 1 line
(quoted diff) +0 -0

b/net/sched/sch_generic.c index

net/sched/sch_generic.c static inline int qdisc_restart(struct

Quoted 4 lines

9461e8a..203d5c4 100644

net/sched/sch_generic.c +0 -0
@@ -168,7 +168,8 @@ static inline int qdisc_restart(struct

net_device *dev)

Quoted 2 lines
net/sched/sch_generic.c +0 -0
 	spin_unlock(&dev->queue_lock);
Quoted 8 lines

net/sched/sch_generic.c static inline int qdisc_restart(struct net_device *dev) · line 174

Quoted 6 lines
net/sched/sch_generic.c +2 -1
 	ret = NETDEV_TX_BUSY;
-	if (!netif_queue_stopped(dev))
+	if (!netif_queue_stopped(dev) &&
+	    !netif_subqueue_stopped(dev, skb->queue_mapping))
 		/* churn baby churn .. */
 		ret = dev_hard_start_xmit(skb, dev);

I'll try again - please move this to patch 2/3.

I'm sorry; I misread your original comment about this. I'll move the change (although this disappears with Jamal's and KK's qdisc_restart() cleanup).

Quoted 24 lines
Quoted 17 lines
net/sched/sch_prio.c index renamed from net/sched/sch_prio.c +0 -0

40a13e8..8a716f0 100644

net/sched/sch_prio.c +2 -0
@@ -40,9 +40,11 @@
 struct prio_sched_data
 {
 	int bands;
+	int curband; /* for round-robin */
 	struct tcf_proto *filter_list;
 	u8  prio2band[TC_PRIO_MAX+1];
 	struct Qdisc *queues[TCQ_PRIO_BANDS];
+	unsigned char mq;
 };

net/sched/sch_prio.c prio_classify(struct sk_buff *skb, struct Qdisc · line 77

Quoted 6 lines
net/sched/sch_prio.c +3 -0
@@ -70,14 +72,28 @@ prio_classify(struct sk_buff *skb, struct Qdisc 
 *sch, int *qerr)  #endif
 			if (TC_H_MAJ(band))
 				band = 0;
+			if (q->mq)
+				skb->queue_mapping = 
+

q->prio2band[band&TC_PRIO_MAX];

Quoted 6 lines

net/sched/sch_prio.c prio_classify(struct sk_buff *skb, struct Qdisc · line 79

Quoted 2 lines
net/sched/sch_prio.c +2 -0
+			else
+				skb->queue_mapping = 0;

Might look cleaner if you have one central point where queue_mapping is set and the band is returned.

I'll see how easy it'll be to condense this; because the queue being selected in the qdisc can be different based on a few different things, I'm not sure how easy it'll be to assign this in one spot. I'll play around with it and see what I can come up with.

Quoted 10 lines

net/sched/sch_prio.c prio_classify(struct sk_buff *skb, struct Qdisc · line 81

Quoted 2 lines
net/sched/sch_prio.c +2 -0
+	/* If we're multiqueue, make sure the number of incoming bands
+	 * matches the number of queues on the device we're

associating with.

net/sched/sch_prio.c static int prio_tune(struct Qdisc *sch, struct rtattr *opt) · line 276

Quoted 3 lines
net/sched/sch_prio.c +3 -0
+	 */
+	if (tb[TCA_PRIO_MQ - 1])
+		q->mq = *(unsigned char *)RTA_DATA(tb[TCA_PRIO_MQ - 1]);

If you're using it as a flag, please use RTA_GET_FLAG(), otherwise RTA_GET_U8.

Will do. Thanks.

Quoted 9 lines

net/sched/sch_prio.c static int prio_dump(struct Qdisc *sch,

Quoted 6 lines
net/sched/sch_prio.c +2 -0
+	if (q->mq && (qopt->bands != sch->dev->egress_subqueue_count))
+		return -EINVAL;
 
 	sch_tree_lock(sch);
 	q->bands = qopt->bands;

@@ -280,7 +342,7 @@ static int prio_dump(struct Qdisc *sch,

struct sk_buff *skb)

Quoted 2 lines
net/sched/sch_prio.c +0 -0
 	memcpy(&opt.priomap, q->prio2band, TC_PRIO_MAX+1);
Quoted 8 lines

net/sched/sch_prio.c static int prio_dump(struct Qdisc *sch, struct sk_buff *skb) · line 345

Quoted 3 lines
net/sched/sch_prio.c +1 -1
 	nest = RTA_NEST_COMPAT(skb, TCA_OPTIONS, sizeof(opt), &opt);
-	RTA_PUT_U32(skb, TCA_PRIO_TEST, 321);
+	RTA_PUT_U8(skb, TCA_PRIO_MQ, q->mq);

And RTA_PUT_FLAG. Now that I think of it, does it even makes sense to have a prio private flag for this instead of a qdisc global one?

There currently aren't any other qdiscs that are natural fits for multiqueue that I can see. I can see the benefit though of having this as a global flag in the qdisc API; let me check it out, and if it makes sense, I can move it.

Quoted 6 lines

net/sched/sch_prio.c static struct Qdisc_ops prio_qdisc_ops = { · line 537

Quoted 4 lines
net/sched/sch_prio.c +2 -1
 static int __init prio_module_init(void)  {
-	return register_qdisc(&prio_qdisc_ops);
+	register_qdisc(&prio_qdisc_ops);
+	register_qdisc(&rr_qdisc_ops);

Proper error handling please.

Will do.

Thanks,
-PJ Waskiewicz

#8 lore raw

Re: [PATCH 3/3] NET: [SCHED] Qdisc changes and sch_rr added for multiqueue

Quoted 9 lines · Peter P
Quoted 3 lines

And RTA_PUT_FLAG. Now that I think of it, does it even makes sense to have a prio private flag for this instead of a qdisc global one?

There currently aren't any other qdiscs that are natural fits for multiqueue that I can see. I can see the benefit though of having this as a global flag in the qdisc API; let me check it out, and if it makes sense, I can move it.

Yes, that thought occured to me as well. Keeping it private seems better.

author
#9 lore raw

RE: [PATCH 3/3] NET: [SCHED] Qdisc changes and sch_rr added for multiqueue

In reply to Patrick McHardy
Quoted 8 lines

net/sched/sch_prio.c prio_classify(struct sk_buff *skb, struct Qdisc · line 77

Quoted 7 lines
net/sched/sch_prio.c +3 -0
@@ -70,14 +72,28 @@ prio_classify(struct sk_buff *skb, struct Qdisc 
 *sch, int *qerr)  #endif
 			if (TC_H_MAJ(band))
 				band = 0;
+			if (q->mq)
+				skb->queue_mapping = 
+

q->prio2band[band&TC_PRIO_MAX];

Quoted 6 lines

Review prio_classify(struct sk_buff *skb, struct Qdisc · line 79

Quoted 2 lines
(quoted diff) +2 -0
+			else
+				skb->queue_mapping = 0;

Might look cleaner if you have one central point where queue_mapping is set and the band is returned.

I've taken a stab at this. I can have one return point, but I'll still have multiple assignments of skb->queue_mapping due to the different branches for which queue to select in the qdisc. I suppose we can do a rewrite of prio_classify(), but to me that seems beyond the scope of the multiqueue patches themselves. What do you think?

Thanks,
-PJ

#10 lore raw

Re: [PATCH 3/3] NET: [SCHED] Qdisc changes and sch_rr added for multiqueue

Quoted 16 lines · Peter P
Quoted 10 lines

net/sched/sch_prio.c prio_classify(struct sk_buff *skb, struct Qdisc · line 78

Quoted 8 lines
net/sched/sch_prio.c +3 -0
@@ -70,14 +72,28 @@ prio_classify(struct sk_buff *skb, struct Qdisc 
 *sch, int *qerr)  #endif
 			if (TC_H_MAJ(band))
 				band = 0;
+			if (q->mq)
+				skb->queue_mapping = 
+

q->prio2band[band&TC_PRIO_MAX];

Quoted 5 lines

net/sched/sch_prio.c prio_classify(struct sk_buff *skb, struct Qdisc *sch, int *qerr) · line 92

Quoted 3 lines
net/sched/sch_prio.c +2 -0
+			else
+				skb->queue_mapping = 0;

Might look cleaner if you have one central point where queue_mapping is set and the band is returned.

Quoted 6 lines

I've taken a stab at this. I can have one return point, but I'll still have multiple assignments of skb->queue_mapping due to the different branches for which queue to select in the qdisc. I suppose we can do a rewrite of prio_classify(), but to me that seems beyond the scope of the multiqueue patches themselves. What do you think?

Thats not necessary. I just though you could add one exit point:

...
out:
skb->queue_mapping = q->mq ? band : 0;
return q->queues[band];
}

But if that doesn't work don't bother ..

author
#11 lore raw

RE: [PATCH 3/3] NET: [SCHED] Qdisc changes and sch_rr added for multiqueue

Quoted 10 lines

Thats not necessary. I just though you could add one exit point:

...
out:

    skb->queue_mapping = q->mq ? band : 0;
    return q->queues[band];
}

But if that doesn't work don't bother ..

Unfortunately it won't, given how band might be used like this to select the queue:

return q->queues[q->prio2band[band&TC_PRIO_MAX]];

I'll keep this in mind though, and if it can be done cleanly, I'll submit a patch.

Thanks Patrick,
-PJ

#12 lore raw

Re: [PATCH 3/3] NET: [SCHED] Qdisc changes and sch_rr added for multiqueue

In reply to PJ Waskiewicz

net/sched/sch_prio.c static int prio_tune(struct Qdisc *sch, struct rtattr *opt) · line 279

Quoted 8 lines · PJ Waskiewicz
net/sched/sch_prio.c +8 -0
+	/* If we're multiqueue, make sure the number of incoming bands
+	 * matches the number of queues on the device we're associating with.
+	 */
+	if (tb[TCA_PRIO_MQ - 1])
+		q->mq = *(unsigned char *)RTA_DATA(tb[TCA_PRIO_MQ - 1]);
+
+	if (q->mq && (qopt->bands != sch->dev->egress_subqueue_count))
+		return -EINVAL;

A nice thing you could do for the user here is use egress_subqueue_count as default when qopt->bands == 0 (and change tc prio to accept 0 in case it doesn't).

author
#13 lore raw

RE: [PATCH 3/3] NET: [SCHED] Qdisc changes and sch_rr added for multiqueue

Quoted 4 lines

Review

Quoted 2 lines · PJ Waskiewicz
(quoted diff) +2 -0
+	/* If we're multiqueue, make sure the number of incoming bands
+	 * matches the number of queues on the device we're

associating with.

Quoted 11 lines

net/sched/sch_prio.c static int prio_tune(struct Qdisc *sch, struct rtattr *opt) · line 279

Quoted 6 lines
net/sched/sch_prio.c +6 -0
+	 */
+	if (tb[TCA_PRIO_MQ - 1])
+		q->mq = *(unsigned char *)RTA_DATA(tb[TCA_PRIO_MQ - 1]);
+
+	if (q->mq && (qopt->bands != sch->dev->egress_subqueue_count))
+		return -EINVAL;

A nice thing you could do for the user here is use egress_subqueue_count as default when qopt->bands == 0 (and change tc prio to accept 0 in case it doesn't).

prio only allows a minimum of 2 bands right now. I see what you're suggesting though; let me think about this. I do like this suggestion.

Thanks,
-PJ Waskiewicz