Re: [PATCH] check the return value of ndo_select_queue()

[Eric Dumazet <eric.dumazet@gmail.com>]

Changli Gao a écrit :
> 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 <xiaosuo@gmail.com>
> ----
>  net/core/dev.c |   33 ++++++++++++++++++++++++---------
>  1 file changed, 24 insertions(+), 9 deletions(-)
> diff --git a/net/core/dev.c b/net/core/dev.c
> index bf629ac..74a3824 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1849,22 +1849,37 @@ static struct netdev_queue *dev_pick_tx(struct
> net_device *dev,
>  {
>  	u16 queue_index;
>  	struct sock *sk = skb->sk;
> +	unsigned int real_num_tx_queues = dev->real_num_tx_queues;
> 
>  	if (sk_tx_queue_recorded(sk)) {
>  		queue_index = sk_tx_queue_get(sk);
> +		if (unlikely(queue_index >= real_num_tx_queues)) {
> +			do {
> +				queue_index -= real_num_tx_queues;
> +			} while (queue_index >= real_num_tx_queues);
> +			sk_tx_queue_set(sk, queue_index);
> +		}
>  	} else {
> -		const struct net_device_ops *ops = dev->netdev_ops;
> +		u16 (*select_queue)(struct net_device *, struct sk_buff *);
> 
> -		if (ops->ndo_select_queue) {
> -			queue_index = ops->ndo_select_queue(dev, skb);
> -		} else {
> +		if (real_num_tx_queues == 1) {
>  			queue_index = 0;
> -			if (dev->real_num_tx_queues > 1)
> -				queue_index = skb_tx_hash(dev, skb);
> -
> -			if (sk && sk->sk_dst_cache)
> -				sk_tx_queue_set(sk, queue_index);
> +		} else if((select_queue = dev->netdev_ops->ndo_select_queue)) {
> +			queue_index = 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);
>  		}
> +
> +		if (sk && sk->sk_dst_cache)
> +			sk_tx_queue_set(sk, queue_index);
>  	}
> 
>  	skb_set_queue_mapping(skb, queue_index);


Please always use scripts/checkpatch.pl before sending patches

} else if((select_queue = dev->netdev_ops->ndo_select_queue)) {

A space is required after the "if"

It seems to me this patch is not matching its ChangeLog :

You moved the :

if (sk && sk->sk_dst_cache)
	sk_tx_queue_set(sk, queue_index);

Why not using much readable patch like :


[PATCH] 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 <xiaosuo@gmail.com>
Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
----

diff --git a/net/core/dev.c b/net/core/dev.c
index ad8e320..bbc9a41 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -1845,6 +1845,20 @@ u16 skb_tx_hash(const struct net_device *dev, const struct sk_buff *skb)
 }
 EXPORT_SYMBOL(skb_tx_hash);
 
+static inline u16 dev_cap_txqueue(struct net_device *dev, u16 queue_index)
+{
+	if (unlikely(queue_index >= dev->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,
+			     dev->real_num_tx_queues);
+		}
+		return 0;
+	}
+	return queue_index;
+}
+
 static struct netdev_queue *dev_pick_tx(struct net_device *dev,
 					struct sk_buff *skb)
 {
@@ -1858,6 +1872,7 @@ static struct netdev_queue *dev_pick_tx(struct net_device *dev,
 
 		if (ops->ndo_select_queue) {
 			queue_index = ops->ndo_select_queue(dev, skb);
+			queue_index = dev_cap_txqueue(dev, queue_index);
 		} else {
 			queue_index = 0;
 			if (dev->real_num_tx_queues > 1)