NET: [UPDATED] Multiqueue network device support implementation.

From: Peter P Waskiewicz Jr <[email protected]>

Update: Fixed a typecast in free_netdev() for the egress_subqueue list.

Added an API and associated supporting routines for multiqueue network devices. This allows network devices supporting multiple TX queues to configure each queue within the netdevice and manage each queue independantly. Changes to the PRIO Qdisc also allow a user to map multiple flows to individual TX queues, taking advantage of each queue on the device.

This indeed looks a lot better than the first patch. I'm too tired to fully review this now, but could you please post the corresponding e1000 patch? From a quick look I'm guessing that this patch changes the behaviour of the prio qdisc from strict priority to whatever scheduling mechanism e1000 uses for its queues when the multiqueue config option is enabled, which might surprise people.

Thanks Pat for the initial feedback. I can post a set of patches to e1000 using the new API; I'll try to get them out asap (need to apply to this kernel tree).

However, the PRIO qdisc still uses the priority in
the bands for dequeueing priority, and will feed the queues on the NIC. The e1000, and any other multiqueue NIC, will schedule Tx based on how the PRIO qdisc feeds the queues. So the only priority here is the dequeuing priority from the kernel. The e1000 will use the new API for starting/stopping the individual queues based on the descriptors available, much like it does today for the global queue.

Thanks.

Quoted 3 lines

However, the PRIO qdisc still uses the priority in the bands for dequeueing priority, and will feed the queues on the NIC. The e1000, and any other multiqueue NIC, will schedule Tx

based on how

Quoted 3 lines

the PRIO qdisc feeds the queues. So the only priority here is the dequeuing priority from the kernel. The e1000 will use the new API for starting/stopping the individual queues based on the

descriptors

Quoted 1 line

available, much like it does today for the global queue.

Packets will only be dequeued from a band if the associated subqueue is active, which moves the decision from prio to the driver, no?
What policy does e1000 use for scheduling its internal queues?

Quoted 5 lines

Packets will only be dequeued from a band if the associated subqueue is active, which moves the decision from prio to the driver, no?
What policy does e1000 use for scheduling its internal queues?

E1000 is handed the skb's from PRIO to whichever queue the PRIO flows dictate (based on band2queue mapping, tc filters, or TOS to skb->priority filtering). Once the skb hits the e1000, the internal policy is round-robin on the hardware queues to dequeue and transmit on the wire. I agree the NIC does influence the decision of which band an skb will be dequeued from based on available descriptors, etc., but that's one of the goals of multiqueue: don't allow another traffic flow in one queue stop or impact another separate flow.

Once NICs start using hardware-based priority scheduling (wireless?), we can use a round-robin type qdisc in the kernel to dequeue, and let the hardware directly decide how important one flow is over another.

You bring up a good point, it would be good to hear the opinion from one of the wireless people on this since they have their own multiqueue scheduler in the wireless-dev tree.

net/core/dev.c +0 -0

index 219a57f..c11c8fa 100644

net/sched/sch_generic.c +2 -1

@@ -133,7 +133,8 @@ static inline int qdisc_restart(struct net_device *dev)
 			/* And release queue */
 			spin_unlock(&dev->queue_lock);
 
-			if (!netif_queue_stopped(dev)) {
+			if (!netif_queue_stopped(dev) &&
+			    !netif_subqueue_stopped(dev, skb->queue_mapping)) {
 				int ret;
 
 				ret = dev_hard_start_xmit(skb, dev);

net/sched/sch_generic.c +0 -0

 			/* NETDEV_TX_BUSY - we need to requeue */
 			/* Release the driver */
 			if (!nolock) {

net/sched/sch_prio.c +1 -0

index 5cfe60b..7365621 100644
@@ -43,6 +43,7 @@ struct prio_sched_data
 	struct tcf_proto *filter_list;
 	u8  prio2band[TC_PRIO_MAX+1];
 	struct Qdisc *queues[TCQ_PRIO_BANDS];
+	u16 band2queue[TC_PRIO_MAX + 1];
 };
 
 

net/sched/sch_prio.c +14 -0

@@ -242,6 +259,30 @@ static int prio_tune(struct Qdisc *sch, struct rtattr *opt)
 			}
 		}
 	}
+	/* setup queue to band mapping */
+	if (q->bands < sch->dev->egress_subqueue_count) {
+		qmapoffset = 1;
+		mod = 0;
+	} else {
+		mod = q->bands % sch->dev->egress_subqueue_count;
+		qmapoffset = q->bands / sch->dev->egress_subqueue_count +
+				((mod) ? 1 : 0);
+	}
+
+	queue = 0;
+	offset = 0;
+	for (i = 0; i < q->bands; i++) {
+		q->band2queue[i] = queue;

Review

Quoted 2 lines · Peter P Waskiewicz Jr

(quoted diff) +2 -0

+	/* To retrieve statistics per subqueue - FOR FUTURE USE */
+	struct net_device_stats* (*get_subqueue_stats)(struct 

net_device *dev,

Review

Quoted 1 line

(quoted diff) +1 -0

+							int 

queue_index);

Please no future use stuff, just add it when you need it.

net/core/dev.c struct net_device *alloc_netdev(int

Quoted 6 lines

net/core/dev.c index 219a57f..c11c8fa renamed from net/core/dev.c +0 -0

100644

net/core/dev.c +0 -0

@@ -3326,12 +3328,23 @@ struct net_device *alloc_netdev(int 

sizeof_priv, const char *name,

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

Quoted 8 lines

net/core/dev.c +5 -0

 	if (sizeof_priv)
 		dev->priv = netdev_priv(dev);
 
+ 	alloc_size = (sizeof(struct net_device_subqueue) * queue_count);
+ 
+ 	p = kzalloc(alloc_size, GFP_KERNEL);
+ 	if (!p) {
+ 		printk(KERN_ERR "alloc_netdev: Unable to 

allocate queues.\n");

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

Quoted 1 line

net/core/dev.c +1 -0

+ 		return NULL;

This leaks the device. You treat every single-queue device as having a single subqueue. If it doesn't get too ugly it would be nice to avoid this and only allocate the subqueue states for real multiqueue devices.

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

Quoted 4 lines

net/sched/sch_generic.c +0 -0

@@ -133,7 +133,8 @@ static inline int qdisc_restart(struct 

net_device *dev)

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

Quoted 6 lines

net/sched/sch_generic.c +2 -1

 			/* And release queue */
 			spin_unlock(&dev->queue_lock);
 
-			if (!netif_queue_stopped(dev)) {
+			if (!netif_queue_stopped(dev) &&
+			    !netif_subqueue_stopped(dev, 

skb->queue_mapping)) {

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

Quoted 3 lines

net/sched/sch_generic.c +0 -0

 				int ret;
 
 				ret = dev_hard_start_xmit(skb, 

dev); @@ -149,7 +150,6 @@ static

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

Quoted 5 lines

inline int qdisc_restart(struct net_device *dev)

net/sched/sch_generic.c +0 -1

 					goto collision;
 				}
 			}
-

Unrelated whitespace change.

Quoted 16 lines

net/sched/sch_generic.c +0 -0

 			/* NETDEV_TX_BUSY - we need to requeue */
 			/* Release the driver */
 			if (!nolock) {

net/sched/sch_prio.c index renamed from net/sched/sch_prio.c +0 -0

5cfe60b..7365621 100644

net/sched/sch_prio.c +1 -0

@@ -43,6 +43,7 @@ struct prio_sched_data
 	struct tcf_proto *filter_list;
 	u8  prio2band[TC_PRIO_MAX+1];
 	struct Qdisc *queues[TCQ_PRIO_BANDS];
+	u16 band2queue[TC_PRIO_MAX + 1];
 };
 
 

net/sched/sch_prio.c prio_classify(struct sk_buff *skb,

Quoted 1 line

net/sched/sch_prio.c +0 -0

@@ -63,20 +64,26 @@ prio_classify(struct sk_buff *skb, 

struct Qdisc *sch, int *qerr)

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

Quoted 4 lines

net/sched/sch_prio.c +0 -1

 		case TC_ACT_SHOT:
 			return NULL;
 		};
-

Same here

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

Quoted 4 lines

net/sched/sch_prio.c +0 -0

 		if (!q->filter_list ) {
 #else
 		if (!q->filter_list || tc_classify(skb, 

q->filter_list, &res)) {

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

Quoted 5 lines

net/sched/sch_prio.c +2 -0

 #endif
 			if (TC_H_MAJ(band))
 				band = 0;
+			skb->queue_mapping =
+				  

q->prio2band[q->band2queue[band&TC_PRIO_MAX]];

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

Quoted 1 line

net/sched/sch_prio.c +1 -0

+

Does this needs to be cleared at some point again? TC actions might redirect or mirror packets to other (multiqueue) devices.

net/sched/sch_prio.c static int prio_tune(struct Qdisc

Quoted 2 lines

net/sched/sch_prio.c +0 -0

@@ -242,6 +259,30 @@ static int prio_tune(struct Qdisc 

*sch, struct rtattr *opt)

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

Quoted 10 lines

net/sched/sch_prio.c +7 -0

 			}
 		}
 	}
+	/* setup queue to band mapping */
+	if (q->bands < sch->dev->egress_subqueue_count) {
+		qmapoffset = 1;
+		mod = 0;
+	} else {
+		mod = q->bands % sch->dev->egress_subqueue_count;
+		qmapoffset = q->bands / 

sch->dev->egress_subqueue_count +

Quoted 13 lines

net/sched/sch_prio.c +13 -0

+				((mod) ? 1 : 0);
+	}
+
+	queue = 0;
+	offset = 0;
+	for (i = 0; i < q->bands; i++) {
+		q->band2queue[i] = queue;
+		if ( ((i + 1) - offset) == qmapoffset) {
+			queue++;
+			offset += qmapoffset;
+			if (mod)
+				mod--;
+			qmapoffset = q->bands /

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

Quoted 4 lines

net/sched/sch_prio.c +4 -0

+				sch->dev->egress_subqueue_count +
+				((mod) ? 1 : 0);
+		}
+	}

Besides being quite ugly, I don't think this does what you want. For bands < queues we get band2queue[0] = 0, all others map to 1.

Quoted 4 lines

This leaks the device. You treat every single-queue device as having a single subqueue. If it doesn't get too ugly it would be nice to avoid this and only allocate the subqueue states for real multiqueue devices.

We went back and forth on this. The reason we allocate a queue in every case, even on single-queue devices, was to make the stack not have branching for multiqueue and non-multiqueue devices. If we don't have at least one queue on a device, then we can't have netif_subqueue_stopped() in the hotpath unless we check if a device is multiqueue before. The original patches I released had this branching, and I was asked to not do that.

Quoted 6 lines

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

Quoted 2 lines

net/sched/sch_prio.c +2 -0

+			skb->queue_mapping =
+				 q->prio2band[q->band2queue[band&TC_PRIO_MAX]];

Does this needs to be cleared at some point again? TC actions might redirect or mirror packets to other (multiqueue) devices.

If an skb is redirected to another device, the skb should be filtered through that device's qdisc, yes?

Quoted 2 lines

Quoted 2 lines

Review

Quoted 2 lines

(quoted diff) +2 -0

+			skb->queue_mapping =
+				 

q->prio2band[q->band2queue[band&TC_PRIO_MAX]];

Quoted 7 lines

Quoted 4 lines

Does this needs to be cleared at some point again? TC actions might redirect or mirror packets to other (multiqueue) devices.

If an skb is redirected to another device, the skb should

be filtered

Quoted 1 line

through that device's qdisc, yes?

Yes, but the device might not have a queue or use something different than prio, so the value would stay the same. I think you need to clear it before enqueueing a packet or alternatively when redirecting in the mirred action.