Skip to contentSkip to conversation

Diet struct sk_buff a bit

PATCH Applied Pavel Emelyanov 2 participants 9 messages Oct 19, 2007 – Oct 22, 2007

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

Pavel Emelyanov

author
#0 lore raw

cover Diet struct sk_buff a bit

The __u16 queue_mapping field only makes sense in the CONFIG_NETDEVICES_MULTIQUEUE=y case only.

Despite this field may be set explicitly to some non-zero value (in net/core/pktgen.c), the exact value affects nothing in case the config option in question is N (mainly it is used in netif_subqueue_stopped(), which will always return 0 in this case).

So cleanup the code a bit and move this field under the config option.

Signed-off-by Pavel Emelyanov <[email protected]>
author
#1 lore raw

PATCH 1/4 Use the skb_set_queue_mapping where appropriate

There's already such a helper to initialize this field. Use it.

Signed-off-by Pavel Emelyanov <[email protected]>

---

net/core/dev.c +1 -1
index 38b03da..8726589 100644
@@ -1661,7 +1661,7 @@ gso:
 		q = dev->qdisc;
 		if (q->enqueue) {
 			/* reset queue_mapping to zero */
-			skb->queue_mapping = 0;
+			skb_set_queue_mapping(skb, 0);
 			rc = q->enqueue(skb, q);
 			qdisc_run(dev);
 			spin_unlock(&dev->queue_lock);
net/core/pktgen.c +2 -4
index 8cae60c..d4d9a36 100644
@@ -2603,8 +2603,7 @@ static struct sk_buff *fill_packet_ipv4(struct net_device *odev,
 	skb->network_header = skb->tail;
 	skb->transport_header = skb->network_header + sizeof(struct iphdr);
 	skb_put(skb, sizeof(struct iphdr) + sizeof(struct udphdr));
-	skb->queue_mapping = pkt_dev->cur_queue_map;
-
+	skb_set_queue_mapping(skb, pkt_dev->cur_queue_map);
 	iph = ip_hdr(skb);
 	udph = udp_hdr(skb);
 

@@ -2941,8 +2940,7 @@ static struct sk_buff *fill_packet_ipv6(struct net_device *odev,
 	skb->network_header = skb->tail;
 	skb->transport_header = skb->network_header + sizeof(struct ipv6hdr);
 	skb_put(skb, sizeof(struct ipv6hdr) + sizeof(struct udphdr));
-	skb->queue_mapping = pkt_dev->cur_queue_map;
-
+	skb_set_queue_mapping(skb, pkt_dev->cur_queue_map);
 	iph = ipv6_hdr(skb);
 	udph = udp_hdr(skb);
maintainer Applied
#2 lore raw

Re: [PATCH 1/4] Use the skb_set_queue_mapping where appropriate

From: Pavel Emelyanov <[email protected]>
Date: Fri, 19 Oct 2007 13:00:52 +0400

Quoted 4 lines

There's already such a helper to initialize this field. Use it.

Signed-off-by Pavel Emelyanov <[email protected]>

Applied.

net/core/dev.c gso: · line 1664

Quoted 3 lines
net/core/dev.c +1 -1
 			/* reset queue_mapping to zero */
-			skb->queue_mapping = 0;
+			skb_set_queue_mapping(skb, 0);

This right here is a great example why I add next to no comments to the code I write. They are %99.999 of the time stating the obvious, and where a comment is needed it's the code's fault and the code in such cases could be rewritten to make what's going on more clear thus making any commentary, again, redundant and stating the obvious.

Here the case is the former, it's of the form:

	/* Add one to 'a'! */
	a += 1;

Gee thanks for letting me know! How insightful! :-))

author
#3 lore raw

PATCH 2/4 Make and use skb_get_queue_mapping

In reply to Pavel Emelyanov

Make the helper for getting the field, symmetrical to the "set" one. Return 0 if CONFIG_NETDEVICES_MULTIQUEUE=n

Signed-off-by Pavel Emelyanov <[email protected]>

---

drivers/net/cpmac.c +1 -1
index ed53aaa..ae41973 100644
@@ -471,7 +471,7 @@ static int cpmac_start_xmit(struct sk_buff *skb, struct net_device *dev)
 	}
 
 	len = max(skb->len, ETH_ZLEN);
-	queue = skb->queue_mapping;
+	queue = skb_get_queue_mapping(skb);
 #ifdef CONFIG_NETDEVICES_MULTIQUEUE
 	netif_stop_subqueue(dev, queue);
 #else
include/linux/skbuff.h +9 -0
index f93f22b..580ed1f 100644
@@ -1770,6 +1771,15 @@ static inline void skb_set_queue_mapping(struct sk_buff *skb, u16 queue_mapping)
 #endif
 }
 
+static inline u16 skb_get_queue_mapping(struct sk_buff *skb)
+{
+#ifdef CONFIG_NETDEVICES_MULTIQUEUE
+	return skb->queue_mapping;
+#else
+	return 0;
+#endif
+}
+
 static inline void skb_copy_queue_mapping(struct sk_buff *to, const struct sk_buff *from)
 {
 #ifdef CONFIG_NETDEVICES_MULTIQUEUE
net/sched/sch_teql.c +1 -1
index be57cf3..421281d 100644
@@ -266,7 +266,7 @@ static int teql_master_xmit(struct sk_buff *skb, struct net_device *dev)
 	int busy;
 	int nores;
 	int len = skb->len;
-	int subq = skb->queue_mapping;
+	int subq = skb_get_queue_mapping(skb);
 	struct sk_buff *skb_res = NULL;
 
 	start = master->slaves;
maintainer Applied
#4 lore raw

Re: [PATCH 2/4] Make and use skb_get_queue_mapping

From: Pavel Emelyanov <[email protected]>
Date: Fri, 19 Oct 2007 13:02:31 +0400

Quoted 4 lines

Make the helper for getting the field, symmetrical to the "set" one. Return 0 if CONFIG_NETDEVICES_MULTIQUEUE=n

Signed-off-by Pavel Emelyanov <[email protected]>

Applied.

author
#5 lore raw

PATCH 3/4 Hide the queue_mapping field inside netif_subqueue_stopped

In reply to Pavel Emelyanov

Many places get the queue_mapping field from skb to pass it to the netif_subqueue_stopped() which will be 0 in any case.

Make the helper that works with sk_buff

Signed-off-by Pavel Emelyanov <[email protected]>

---

include/linux/netdevice.h +6 -1
index 6f85db3..4a3f54e 100644
@@ -996,7 +996,7 @@ static inline void netif_stop_subqueue(struct net_device *dev, u16 queue_index)
  *
  * Check individual transmit queue of a device with multiple transmit queues.
  */
-static inline int netif_subqueue_stopped(const struct net_device *dev,
+static inline int __netif_subqueue_stopped(const struct net_device *dev,
 					 u16 queue_index)
 {
 #ifdef CONFIG_NETDEVICES_MULTIQUEUE

@@ -1007,6 +1007,11 @@ static inline int netif_subqueue_stopped(const struct net_device *dev,
 #endif
 }
 
+static inline int netif_subqueue_stopped(const struct net_device *dev,
+					 struct sk_buff *skb)
+{
+	return __netif_subqueue_stopped(dev, skb_get_queue_mapping(skb));
+}
 
 /**
  *	netif_wake_subqueue - allow sending packets on subqueue
net/core/dev.c +2 -2
index 38b03da..8726589 100644
@@ -1553,7 +1553,7 @@ gso:
 			return rc;
 		}
 		if (unlikely((netif_queue_stopped(dev) ||
-			     netif_subqueue_stopped(dev, skb->queue_mapping)) &&
+			     netif_subqueue_stopped(dev, skb)) &&
 			     skb->next))
 			return NETDEV_TX_BUSY;
 	} while (skb->next);

@@ -1692,7 +1692,7 @@ gso:
 			HARD_TX_LOCK(dev, cpu);
 
 			if (!netif_queue_stopped(dev) &&
-			    !netif_subqueue_stopped(dev, skb->queue_mapping)) {
+			    !netif_subqueue_stopped(dev, skb)) {
 				rc = 0;
 				if (!dev_hard_start_xmit(skb, dev)) {
 					HARD_TX_UNLOCK(dev);
net/core/netpoll.c +2 -2
index 95daba6..bf8d18f 100644
@@ -67,7 +67,7 @@ static void queue_process(struct work_struct *work)
 		local_irq_save(flags);
 		netif_tx_lock(dev);
 		if ((netif_queue_stopped(dev) ||
-		     netif_subqueue_stopped(dev, skb->queue_mapping)) ||
+		     netif_subqueue_stopped(dev, skb)) ||
 		     dev->hard_start_xmit(skb, dev) != NETDEV_TX_OK) {
 			skb_queue_head(&npinfo->txq, skb);
 			netif_tx_unlock(dev);

@@ -269,7 +269,7 @@ static void netpoll_send_skb(struct netpoll *np, struct sk_buff *skb)
 		     tries > 0; --tries) {
 			if (netif_tx_trylock(dev)) {
 				if (!netif_queue_stopped(dev) &&
-				    !netif_subqueue_stopped(dev, skb->queue_mapping))
+				    !netif_subqueue_stopped(dev, skb))
 					status = dev->hard_start_xmit(skb, dev);
 				netif_tx_unlock(dev);
net/core/pktgen.c +3 -3
index 8cae60c..d4d9a36 100644
@@ -3385,7 +3383,7 @@ static __inline__ void pktgen_xmit(struct pktgen_dev *pkt_dev)
 
 	if ((netif_queue_stopped(odev) ||
 	     (pkt_dev->skb &&
-	      netif_subqueue_stopped(odev, pkt_dev->skb->queue_mapping))) ||
+	      netif_subqueue_stopped(odev, pkt_dev->skb))) ||
 	    need_resched()) {
 		idle_start = getCurUs();
 

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

@@ -3431,7 +3429,7 @@ static __inline__ void pktgen_xmit(struct pktgen_dev *pkt_dev)
 
 	netif_tx_lock_bh(odev);
 	if (!netif_queue_stopped(odev) &&
-	    !netif_subqueue_stopped(odev, pkt_dev->skb->queue_mapping)) {
+	    !netif_subqueue_stopped(odev, pkt_dev->skb)) {
 
 		atomic_inc(&(pkt_dev->skb->users));
 	      retry_now:
net/sched/sch_teql.c +2 -2
index be57cf3..421281d 100644
@@ -284,7 +284,7 @@ restart:
 		if (slave->qdisc_sleeping != q)
 			continue;
 		if (netif_queue_stopped(slave) ||
-		    netif_subqueue_stopped(slave, subq) ||
+		    __netif_subqueue_stopped(slave, subq) ||
 		    !netif_running(slave)) {
 			busy = 1;
 			continue;

@@ -294,7 +294,7 @@ restart:
 		case 0:
 			if (netif_tx_trylock(slave)) {
 				if (!netif_queue_stopped(slave) &&
-				    !netif_subqueue_stopped(slave, subq) &&
+				    !__netif_subqueue_stopped(slave, subq) &&
 				    slave->hard_start_xmit(skb, slave) == 0) {
 					netif_tx_unlock(slave);
 					master->slaves = NEXT_SLAVE(q);
maintainer Applied
#6 lore raw

Re: [PATCH 3/4] Hide the queue_mapping field inside netif_subqueue_stopped

From: Pavel Emelyanov <[email protected]>
Date: Fri, 19 Oct 2007 13:04:13 +0400

Quoted 6 lines

Many places get the queue_mapping field from skb to pass it to the netif_subqueue_stopped() which will be 0 in any case.

Make the helper that works with sk_buff

Signed-off-by Pavel Emelyanov <[email protected]>

Applied, thanks!

author
#7 lore raw

PATCH 4/4 Cut off the queue_mapping field from sk_buff

In reply to Pavel Emelyanov

Just hide it behind the #ifdef, because nobody wants it now.

Signed-off-by Pavel Emelyanov <[email protected]>

---

include/linux/skbuff.h +2 -1
index f93f22b..580ed1f 100644
@@ -301,8 +301,9 @@ struct sk_buff {
 #endif
 
 	int			iif;
+#ifdef CONFIG_NETDEVICES_MULTIQUEUE
 	__u16			queue_mapping;
-
+#endif
 #ifdef CONFIG_NET_SCHED
 	__u16			tc_index;	/* traffic control index */
 #ifdef CONFIG_NET_CLS_ACT