[PATCH] net: reset skb queue mapping when rx'ing over tunnel

[PATCH] net: reset skb queue mapping when rx'ing over tunnel

[Tom Herbert]

Reset queue mapping when an skb is reentering the stack via a tunnel.
On second pass, the queue mapping from the original device is no
longer valid.

Signed-off-by: Tom Herbert <therbert@google.com>
---
diff --git a/include/net/dst.h b/include/net/dst.h
index 81d1413..0238650 100644
--- a/include/net/dst.h
+++ b/include/net/dst.h
@@ -242,6 +242,7 @@ static inline void skb_tunnel_rx(struct sk_buff *skb, struct net_device *dev)
 	dev->stats.rx_packets++;
 	dev->stats.rx_bytes += skb->len;
 	skb->rxhash = 0;
+	skb_set_queue_mapping(skb, 0);
 	skb_dst_drop(skb);
 	nf_reset(skb);
 }

Re: [PATCH] net: reset skb queue mapping when rx'ing over tunnel

[Eric Dumazet]

Le jeudi 23 septembre 2010 à 14:19 -0700, Tom Herbert a écrit :
> Reset queue mapping when an skb is reentering the stack via a tunnel.
> On second pass, the queue mapping from the original device is no
> longer valid.
> 
> Signed-off-by: Tom Herbert <therbert@google.com>
> ---
> diff --git a/include/net/dst.h b/include/net/dst.h
> index 81d1413..0238650 100644
> --- a/include/net/dst.h
> +++ b/include/net/dst.h
> @@ -242,6 +242,7 @@ static inline void skb_tunnel_rx(struct sk_buff *skb, struct net_device *dev)
>  	dev->stats.rx_packets++;
>  	dev->stats.rx_bytes += skb->len;
>  	skb->rxhash = 0;
> +	skb_set_queue_mapping(skb, 0);
>  	skb_dst_drop(skb);
>  	nf_reset(skb);
>  }
> --

Hmm...

This would need to be reverted later when tunnels are updated to be
multiqueue aware ? I made an attempt with GRE some days ago.

I dont understand why this patch is needed, since get_rps_cpu() has a
check anyway

        if (skb_rx_queue_recorded(skb)) {
                u16 index = skb_get_rx_queue(skb);
                if (unlikely(index >= dev->num_rx_queues)) {
                        WARN_ONCE(dev->num_rx_queues > 1, "%s received packet "
                                "on queue %u, but number of RX queues is %u\n",
                                dev->name, index, dev->num_rx_queues);
                        goto done;
                }
                rxqueue = dev->_rx + index;
        } else
                rxqueue = dev->_rx;

Re: [PATCH] net: reset skb queue mapping when rx'ing over tunnel

[Tom Herbert]

> Hmm...
>
> This would need to be reverted later when tunnels are updated to be
> multiqueue aware ? I made an attempt with GRE some days ago.
>
> I dont understand why this patch is needed, since get_rps_cpu() has a
> check anyway
>
I think the skb queue_mapping should correspond to a queue in the
skb's device as an invariant.  In the case that an skb's device is
change from a multiqueue device to single queue device (like GRE), the
inconsistency in the queue_mapping is fairly innocuous, we get one
warning but will pretty much take the unlikely branch for GRE packets
then on.  But imagine a case where skb's device was change from one
multiqueue device to another, but the queue mapping was not also
updated.  This  would cause poor weighting in get_rps_cpus.  For
example, if the new device had fewer queues than the old one,
get_rps_cpu will bias toward using queue 0's rps mask.

>        if (skb_rx_queue_recorded(skb)) {
>                u16 index = skb_get_rx_queue(skb);
>                if (unlikely(index >= dev->num_rx_queues)) {
>                        WARN_ONCE(dev->num_rx_queues > 1, "%s received packet "
>                                "on queue %u, but number of RX queues is %u\n",
>                                dev->name, index, dev->num_rx_queues);
>                        goto done;
>                }
>                rxqueue = dev->_rx + index;
>        } else
>                rxqueue = dev->_rx;
>
>
>
>

Re: [PATCH] net: reset skb queue mapping when rx'ing over tunnel

[Eric Dumazet]

Le vendredi 24 septembre 2010 à 09:18 -0700, Tom Herbert a écrit :
> > Hmm...
> >
> > This would need to be reverted later when tunnels are updated to be
> > multiqueue aware ? I made an attempt with GRE some days ago.
> >
> > I dont understand why this patch is needed, since get_rps_cpu() has a
> > check anyway
> >
> I think the skb queue_mapping should correspond to a queue in the
> skb's device as an invariant.  In the case that an skb's device is
> change from a multiqueue device to single queue device (like GRE), the
> inconsistency in the queue_mapping is fairly innocuous, we get one
> warning but will pretty much take the unlikely branch for GRE packets
> then on.  But imagine a case where skb's device was change from one
> multiqueue device to another, but the queue mapping was not also
> updated.  This  would cause poor weighting in get_rps_cpus.  For
> example, if the new device had fewer queues than the old one,
> get_rps_cpu will bias toward using queue 0's rps mask.

I believe your patch is fine. Thanks

Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

Re: [PATCH] net: reset skb queue mapping when rx'ing over tunnel

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sat, 25 Sep 2010 08:49:10 +0200

> Le vendredi 24 septembre 2010 à 09:18 -0700, Tom Herbert a écrit :
>> > Hmm...
>> >
>> > This would need to be reverted later when tunnels are updated to be
>> > multiqueue aware ? I made an attempt with GRE some days ago.
>> >
>> > I dont understand why this patch is needed, since get_rps_cpu() has a
>> > check anyway
>> >
>> I think the skb queue_mapping should correspond to a queue in the
>> skb's device as an invariant.  In the case that an skb's device is
>> change from a multiqueue device to single queue device (like GRE), the
>> inconsistency in the queue_mapping is fairly innocuous, we get one
>> warning but will pretty much take the unlikely branch for GRE packets
>> then on.  But imagine a case where skb's device was change from one
>> multiqueue device to another, but the queue mapping was not also
>> updated.  This  would cause poor weighting in get_rps_cpus.  For
>> example, if the new device had fewer queues than the old one,
>> get_rps_cpu will bias toward using queue 0's rps mask.
> 
> I believe your patch is fine. Thanks
> 
> Acked-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied, thanks everyone.