Skip to contentSkip to conversation

check the return value of ndo_select_queue()

PATCH In review Changli Gao 2 participants 4 messages Nov 9, 2009

Subsystems (2)
Networking Network drivers
lore
Share & tools
lore

Changli Gao

author
#0 lore raw

PATCH 1/1 check the return value of ndo_select_queue()

check the return value of ndo_select_queue()

Check the return value of ndo_select_queue(). If the value isn't smaller than the real_num_tx_queues, print a warning message, and reset it to zero.

Signed-off-by Changli Gao <[email protected]>

----
net/core/dev.c | 11 +++++++++--

1 file changed, 9 insertions(+), 2 deletions(-)
net/core/dev.c +9 -2
index b8f74cf..0a6bf2e 100644
@@ -1794,10 +1794,17 @@ static struct netdev_queue *dev_pick_tx(struct net_device *dev,
 	const struct net_device_ops *ops = dev->netdev_ops;
 	u16 queue_index = 0;
 
-	if (ops->ndo_select_queue)
+	if (ops->ndo_select_queue) {
 		queue_index = ops->ndo_select_queue(dev, skb);
-	else if (dev->real_num_tx_queues > 1)
+		if (queue_index >= dev->real_num_tx_queues) {
+			WARN(1, "%s selects TX queue %d, "
+			     "but real number of TX queues is %d\n",
+			     dev->name, queue_index, dev->real_num_tx_queues);
+			queue_index = 0;
+		}
+	} else if (dev->real_num_tx_queues > 1) {
 		queue_index = skb_tx_hash(dev, skb);
+	}
 
 	skb_set_queue_mapping(skb, queue_index);
 	return netdev_get_tx_queue(dev, queue_index);
#1 lore raw
Quoted 24 lines · Changli Gao

check the return value of ndo_select_queue()

Check the return value of ndo_select_queue(). If the value isn't smaller than the real_num_tx_queues, print a warning message, and reset it to zero.

Signed-off-by Changli Gao <[email protected]>

----
net/core/dev.c | 11 +++++++++--

1 file changed, 9 insertions(+), 2 deletions(-)
net/core/dev.c +3 -2
index b8f74cf..0a6bf2e 100644
@@ -1794,10 +1794,17 @@ static struct netdev_queue *dev_pick_tx(struct net_device *dev,
 	const struct net_device_ops *ops = dev->netdev_ops;
 	u16 queue_index = 0;
 
-	if (ops->ndo_select_queue)
+	if (ops->ndo_select_queue) {
 		queue_index = ops->ndo_select_queue(dev, skb);
-	else if (dev->real_num_tx_queues > 1)
+		if (queue_index >= dev->real_num_tx_queues) {
+			WARN(1, "%s selects TX queue %d, "

net/core/dev.c static struct netdev_queue *dev_pick_tx(struct net_device *dev, · line 1810

Quoted 10 lines
net/core/dev.c +6 -0
+			     "but real number of TX queues is %d\n",
+			     dev->name, queue_index, dev->real_num_tx_queues);
+			queue_index = 0;
+		}
+	} else if (dev->real_num_tx_queues > 1) {
 		queue_index = skb_tx_hash(dev, skb);
+	}
 
 	skb_set_queue_mapping(skb, queue_index);
 	return netdev_get_tx_queue(dev, queue_index);

Yes, but this is a _very_ unlikely condition (a bug IMHO), so you could use :

if (unlikely(queue_index >= dev->real_num_tx_queues)) {
	...
}

(This unlikely() clause is implied in WARN... macros)

#2 lore raw
Quoted 9 lines · Eric Dumazet

Yes, but this is a _very_ unlikely condition (a bug IMHO), so you could use :

if (unlikely(queue_index >= dev->real_num_tx_queues)) {
	...
}

(This unlikely() clause is implied in WARN... macros)

Also, you want to trigger this message only once, or ratelimit it at least

author
#3 lore raw

2009/11/9 Eric Dumazet <[email protected]>:

Quoted 4 lines
Quoted 3 lines · Eric Dumazet

Yes, but this is a _very_ unlikely condition (a bug IMHO), so you could use :

Quoted 10 lines
Quoted 6 lines
if (unlikely(queue_index >= dev->real_num_tx_queues)) {
      ...
}

(This unlikely() clause is implied in WARN... macros)

Also, you want to trigger this message only once, or ratelimit it at least

How about this version:

        u16 queue_index;
        u16 (*ndo_select_queue)(struct net_device*, struct sk_buff*);
        unsigned int real_num_tx_queues = dev->real_num_tx_queues;

        if (real_num_tx_queues == 1) {
                queue_index = 0;
        } else if ((ndo_select_queue = dev->netdev_ops->ndo_select_queue)) {
                queue_index = ndo_select_queue(dev, skb);
                if (unlikely(queue_index >= real_num_tx_queues)) {
                        if (net_ratelimit())
                                WARN(1, "%s selects TX queue %d, "
                                     "but real number of TX queues is %d\n",
                                     dev->name, queue_index,
                                     real_num_tx_queues);
                        queue_index = 0;
                }
        } else {
                queue_index = skb_tx_hash(dev, skb);
        }

        skb_set_queue_mapping(skb, queue_index);
        return netdev_get_tx_queue(dev, queue_index);
Signature · 2 lines
Regards，
Changli Gao([email protected])