Skip to contentSkip to conversation

bonding: bond_select_queue off by one

PATCH In review Phil Oester 4 participants 6 messages Feb 18, 2011 – Feb 21, 2011

Subsystems (3)
Networking Network drivers Bonding driver
lore
Share & tools
lore

Phil Oester

author
#0 lore raw

PATCH 1/1 bonding: bond_select_queue off by one

The bonding driver's bond_select_queue function simply returns skb->queue_mapping. However queue_mapping could be == 16 for queue #16. This causes the following message to be flooded to syslog:

kernel: bondx selects TX queue 16, but real number of TX queues is 16

ndo_select_queue wants a zero-based number, so bonding driver needs to subtract one to return the proper queue number. Also fix grammar in a comment while in the vicinity.

Phil Oester

Signed-off-by Phil Oester <[email protected]>
drivers/net/bonding/bond_main.c renamed from drivers/net/bonding/bond_main.c.orig +2 -2
@@ -4537,11 +4537,11 @@
 {
 	/*
 	 * This helper function exists to help dev_pick_tx get the correct
-	 * destination queue.  Using a helper function skips the a call to
+	 * destination queue.  Using a helper function skips a call to
 	 * skb_tx_hash and will put the skbs in the queue we expect on their
 	 * way down to the bonding driver.
 	 */
-	return skb->queue_mapping;
+	return skb->queue_mapping ? skb->queue_mapping - 1 : 0;
 }
 
 static netdev_tx_t bond_start_xmit(struct sk_buff *skb, struct net_device *dev)
#1 lore raw
Quoted 21 lines · Phil Oester

The bonding driver's bond_select_queue function simply returns skb->queue_mapping. However queue_mapping could be == 16 for queue #16. This causes the following message to be flooded to syslog:

kernel: bondx selects TX queue 16, but real number of TX queues is 16

ndo_select_queue wants a zero-based number, so bonding driver needs to subtract one to return the proper queue number. Also fix grammar in a comment while in the vicinity.

Phil Oester

Signed-off-by Phil Oester <[email protected]>
drivers/net/bonding/bond_main.c 2011-02-17 18:02:46.919050909 -0800 renamed from drivers/net/bonding/bond_main.c.orig 2011-01-30 09:15:09.813843817 -0800 +0 -0
@@ -4537,11 +4537,11 @@
 {
        /*
         * This helper function exists to help dev_pick_tx get the correct

drivers/net/bonding/bond_main.c 2011-02-17 18:02:46.919050909 -0800 line 4547

Quoted 10 lines
drivers/net/bonding/bond_main.c 2011-02-17 18:02:46.919050909 -0800 renamed from drivers/net/bonding/bond_main.c.orig 2011-01-30 09:15:09.813843817 -0800 +2 -2
-        * destination queue.  Using a helper function skips the a call to
+        * destination queue.  Using a helper function skips a call to
         * skb_tx_hash and will put the skbs in the queue we expect on their
         * way down to the bonding driver.
         */
-       return skb->queue_mapping;
+       return skb->queue_mapping ? skb->queue_mapping - 1 : 0;
 }
 
 static netdev_tx_t bond_start_xmit(struct sk_buff *skb, struct net_device *dev)

This looks basically correct, but it should use the proper functions:

	skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) : 0;

Ben.

Signature · 3 lines
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
#2 lore raw
In reply to Phil Oester
Quoted 10 lines · Phil Oester

The bonding driver's bond_select_queue function simply returns skb->queue_mapping. However queue_mapping could be == 16 for queue #16. This causes the following message to be flooded to syslog:

kernel: bondx selects TX queue 16, but real number of TX queues is 16

ndo_select_queue wants a zero-based number, so bonding driver needs to subtract one to return the proper queue number. Also fix grammar in a comment while in the vicinity.

	Andy, can you comment on this?

	If memory serves, the omission of queue ID zero was on purpose;

is this patch going to break any of the functionality added by:

commit bb1d912323d5dd50e1079e389f4e964be14f0ae3

Author: Andy Gospodarek <[email protected]>
Date:   Wed Jun 2 08:40:18 2010 +0000

bonding: allow user-controlled output slave selection

Quoted 3 lines · Ben Hutchings

This looks basically correct, but it should use the proper functions:

	skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) : 0;

As Ben points out, skb_rx_queue_recorded, skb_record_rx_queue,
et al, do the offset by one internally, but the bond_slave_override function is comparing the slave's queue_id to the skb->queue_mapping.

That makes me wonder if this patch is going to mess things up,
and if bond_slave_override should also use the skb_rx_queue_recorded, et al, functions.

	-J
Quoted 11 lines

Phil Oester

Signed-off-by Phil Oester <[email protected]>
drivers/net/bonding/bond_main.c renamed from drivers/net/bonding/bond_main.c.orig +0 -0
@@ -4537,11 +4537,11 @@
 {
 	/*
 	 * This helper function exists to help dev_pick_tx get the correct

drivers/net/bonding/bond_main.c line 4547

Quoted 10 lines
drivers/net/bonding/bond_main.c renamed from drivers/net/bonding/bond_main.c.orig +2 -2
-	 * destination queue.  Using a helper function skips the a call to
+	 * destination queue.  Using a helper function skips a call to
 	 * skb_tx_hash and will put the skbs in the queue we expect on their
 	 * way down to the bonding driver.
 	 */
-	return skb->queue_mapping;
+	return skb->queue_mapping ? skb->queue_mapping - 1 : 0;
 }
 
 static netdev_tx_t bond_start_xmit(struct sk_buff *skb, struct net_device *dev)

---
-Jay Vosburgh, IBM Linux Technology Center, [email protected]

#3 lore raw
Quoted 16 lines · Jay Vosburgh
Quoted 10 lines · Phil Oester

The bonding driver's bond_select_queue function simply returns skb->queue_mapping. However queue_mapping could be == 16 for queue #16. This causes the following message to be flooded to syslog:

kernel: bondx selects TX queue 16, but real number of TX queues is 16

ndo_select_queue wants a zero-based number, so bonding driver needs to subtract one to return the proper queue number. Also fix grammar in a comment while in the vicinity.

	Andy, can you comment on this?

	If memory serves, the omission of queue ID zero was on purpose;
Quoted 8 lines

is this patch going to break any of the functionality added by:

commit bb1d912323d5dd50e1079e389f4e964be14f0ae3

Author: Andy Gospodarek <[email protected]>
Date:   Wed Jun 2 08:40:18 2010 +0000

bonding: allow user-controlled output slave selection

My original intent was that a queue_mapping == 0 would indicate that the mode's default transmit routine would be used. We could still operate under this assumption, however. I think the patch below will work.

Quoted 6 lines
Quoted 3 lines · Ben Hutchings

This looks basically correct, but it should use the proper functions:

	skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) : 0;
Quoted 8 lines

As Ben points out, skb_rx_queue_recorded, skb_record_rx_queue,
et al, do the offset by one internally, but the bond_slave_override function is comparing the slave's queue_id to the skb->queue_mapping.

That makes me wonder if this patch is going to mess things up,
and if bond_slave_override should also use the skb_rx_queue_recorded, et al, functions.

They could be use them, but I really dislike using functions with 'rx' in the name for options that are clearly for transmit. I would rather get rid of access to queue_id or queue_mapping in the code in question.

How about something like this? I have not fully tested this, but will and would appreciate feedback from Phil or anyone else.

Signed-off-by Andy Gospodarek <[email protected]>
---
 drivers/net/bonding/bond_main.c  |   11 ++++++-----
 drivers/net/bonding/bond_sysfs.c |    2 +-
 drivers/net/bonding/bonding.h    |   16 ++++++++++++++++
 include/linux/skbuff.h           |   15 +++++++++++++++
 net/core/dev.c                   |    4 ++--
 5 files changed, 40 insertions(+), 8 deletions(-)
drivers/net/bonding/bond_main.c +6 -5
index 77e3c6a..02d8161 100644
@@ -4511,20 +4511,21 @@ static inline int bond_slave_override(struct bonding *bond,
 
 	read_lock(&bond->lock);
 
-	if (!BOND_IS_OK(bond) || !skb->queue_mapping)
+	if (!BOND_IS_OK(bond) || !skb_tx_queue_recorded(skb))
 		goto out;
 
 	/* Find out if any slaves have the same mapping as this skb. */
 	bond_for_each_slave(bond, check_slave, i) {
-		if (check_slave->queue_id == skb->queue_mapping) {
+		if (slave_tx_queue_recorded(check_slave) &&
+		    slave_get_tx_queue(check_slave) == skb_get_tx_queue(skb)) {
 			slave = check_slave;
 			break;
 		}
 	}
 
 	/* If the slave isn't UP, use default transmit policy. */
-	if (slave && slave->queue_id && IS_UP(slave->dev) &&
-	    (slave->link == BOND_LINK_UP)) {
+	if (slave && slave_tx_queue_recorded(slave) &&
+	    IS_UP(slave->dev) && (slave->link == BOND_LINK_UP)) {
 		res = bond_dev_queue_xmit(bond, skb, slave->dev);
 	}
 

@@ -4541,7 +4542,7 @@ static u16 bond_select_queue(struct net_device *dev, struct sk_buff *skb)
 	 * skb_tx_hash and will put the skbs in the queue we expect on their
 	 * way down to the bonding driver.
 	 */
-	return skb->queue_mapping;
+	return skb_tx_queue_recorded(skb) ? skb_get_tx_queue(skb) : 0;
 }
 
 static netdev_tx_t bond_start_xmit(struct sk_buff *skb, struct net_device *dev)
drivers/net/bonding/bond_sysfs.c +1 -1
index 72bb0f6..b3bc092 100644
@@ -1499,7 +1499,7 @@ static ssize_t bonding_store_queue_id(struct device *d,
 	/* Check buffer length, valid ifname and queue id */
 	if (strlen(buffer) > IFNAMSIZ ||
 	    !dev_valid_name(buffer) ||
-	    qid > bond->params.tx_queues)
+	    qid >= bond->params.tx_queues)
 		goto err_no_cmd;
 
 	/* Get the pointer to that interface if it exists */
drivers/net/bonding/bonding.h +16 -0
index 31fe980..75b5798 100644
@@ -422,4 +422,20 @@ static inline void bond_unregister_ipv6_notifier(void)
 }
 #endif
 
+static inline void slave_record_tx_queue(struct slave *slave, u16 tx_queue)
+{
+	slave->queue_id = tx_queue + 1;
+}
+
+static inline u16 slave_get_tx_queue(const struct slave *slave)
+{
+	return slave->queue_id - 1;
+}
+
+static inline bool slave_tx_queue_recorded(const struct slave *slave)
+{
+	return slave->queue_id != 0;
+}
+
+
 #endif /* _LINUX_BONDING_H */
include/linux/skbuff.h +15 -0
index 31f02d0..49d101c 100644
@@ -2194,6 +2194,21 @@ static inline bool skb_rx_queue_recorded(const struct sk_buff *skb)
 	return skb->queue_mapping != 0;
 }
 
+static inline void skb_record_tx_queue(struct sk_buff *skb, u16 tx_queue)
+{
+	skb->queue_mapping = tx_queue + 1;
+}
+
+static inline u16 skb_get_tx_queue(const struct sk_buff *skb)
+{
+	return skb->queue_mapping - 1;
+}
+
+static inline bool skb_tx_queue_recorded(const struct sk_buff *skb)
+{
+	return skb->queue_mapping != 0;
+}
+
 extern u16 __skb_tx_hash(const struct net_device *dev,
 			 const struct sk_buff *skb,
 			 unsigned int num_tx_queues);
net/core/dev.c +2 -2
index a413276..50aa490 100644
@@ -2211,8 +2211,8 @@ u16 __skb_tx_hash(const struct net_device *dev, const struct sk_buff *skb,
 	u16 qoffset = 0;
 	u16 qcount = num_tx_queues;
 
-	if (skb_rx_queue_recorded(skb)) {
-		hash = skb_get_rx_queue(skb);
+	if (skb_tx_queue_recorded(skb)) {
+		hash = skb_get_tx_queue(skb);
 		while (unlikely(hash >= num_tx_queues))
 			hash -= num_tx_queues;
 		return hash;
#4 lore raw
Quoted 40 lines · Andy Gospodarek
Quoted 16 lines · Jay Vosburgh
Quoted 10 lines · Phil Oester

The bonding driver's bond_select_queue function simply returns skb->queue_mapping. However queue_mapping could be == 16 for queue #16. This causes the following message to be flooded to syslog:

kernel: bondx selects TX queue 16, but real number of TX queues is 16

ndo_select_queue wants a zero-based number, so bonding driver needs to subtract one to return the proper queue number. Also fix grammar in a comment while in the vicinity.

	Andy, can you comment on this?

	If memory serves, the omission of queue ID zero was on purpose;
Quoted 8 lines

is this patch going to break any of the functionality added by:

commit bb1d912323d5dd50e1079e389f4e964be14f0ae3

Author: Andy Gospodarek <[email protected]>
Date:   Wed Jun 2 08:40:18 2010 +0000

bonding: allow user-controlled output slave selection

My original intent was that a queue_mapping == 0 would indicate that the mode's default transmit routine would be used. We could still operate under this assumption, however. I think the patch below will work.

Quoted 10 lines
Quoted 3 lines · Ben Hutchings

This looks basically correct, but it should use the proper functions:

	skb_rx_queue_recorded(skb) ? skb_get_rx_queue(skb) : 0;

As Ben points out, skb_rx_queue_recorded, skb_record_rx_queue,
et al, do the offset by one internally, but the bond_slave_override function is comparing the slave's queue_id to the skb->queue_mapping.

Quoted 7 lines
Quoted 4 lines

That makes me wonder if this patch is going to mess things up,
and if bond_slave_override should also use the skb_rx_queue_recorded, et al, functions.

They could be use them, but I really dislike using functions with 'rx' in the name for options that are clearly for transmit.

This isn't an option for transmit, it is a record of the result of RX hashing (or steering). It may or may not then be used to select a TX queue.

[...]

Quoted 11 lines
include/linux/skbuff.h +5 -0
@@ -2194,6 +2194,21 @@ static inline bool skb_rx_queue_recorded(const struct sk_buff *skb)
        return skb->queue_mapping != 0;
 }
 
+static inline void skb_record_tx_queue(struct sk_buff *skb, u16 tx_queue)
+{
+       skb->queue_mapping = tx_queue + 1;
+}
+

include/linux/skbuff.h static inline bool skb_rx_queue_recorded(const struct sk_buff *skb) · line 2211

Quoted 10 lines
include/linux/skbuff.h +10 -0
+static inline u16 skb_get_tx_queue(const struct sk_buff *skb)
+{
+       return skb->queue_mapping - 1;
+}
+
+static inline bool skb_tx_queue_recorded(const struct sk_buff *skb)
+{
+       return skb->queue_mapping != 0;
+}
+

[...]

This is nonsense. After the TX queue has been selected, it's recorded in queue_mapping *without* the offset (skb_set_queue_mapping()).

Ben.

Signature · 3 lines
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
#5 lore raw
Quoted 24 lines · Ben Hutchings
Quoted 21 lines · Andy Gospodarek
include/linux/skbuff.h +15 -0
@@ -2194,6 +2194,21 @@ static inline bool skb_rx_queue_recorded(const struct sk_buff *skb)
        return skb->queue_mapping != 0;
 }
 
+static inline void skb_record_tx_queue(struct sk_buff *skb, u16 tx_queue)
+{
+       skb->queue_mapping = tx_queue + 1;
+}
+
+static inline u16 skb_get_tx_queue(const struct sk_buff *skb)
+{
+       return skb->queue_mapping - 1;
+}
+
+static inline bool skb_tx_queue_recorded(const struct sk_buff *skb)
+{
+       return skb->queue_mapping != 0;
+}
+

[...]

Quoted 3 lines

This is nonsense. After the TX queue has been selected, it's recorded in queue_mapping *without* the offset (skb_set_queue_mapping()).

I see that now. Yay for symmetry! :)

I'm actually looking over this now and will post a tested patch to address the original reporter's problem.