diff mbox

check the return value of ndo_select_queue()

Message ID 4AFE621C.1020004@gmail.com
State Accepted, archived
Delegated to: David Miller
Headers show

Commit Message

Eric Dumazet Nov. 14, 2009, 7:54 a.m. UTC
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>
----



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

Changli Gao Nov. 14, 2009, 12:14 p.m. UTC | #1
On Sat, Nov 14, 2009 at 3:54 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
>
>
> 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"
>

Thanks.

> 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);

Maybe I did more in my patch, but I think these little changes doesn't
need separate patches.

>
> 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)
>
>
>

It seems better than mime.
Eric Dumazet Nov. 14, 2009, 12:37 p.m. UTC | #2
Changli Gao a écrit :
> 
> It seems better than mime.
> 
> 

Well, it's still your patch :)

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Nov. 16, 2009, 6:09 a.m. UTC | #3
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sat, 14 Nov 2009 13:37:26 +0100

> Changli Gao a écrit :
>> 
>> It seems better than mime.
>> 
>> 
> 
> Well, it's still your patch :)

Applied, thanks everyone :-)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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)