[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <OF19A2A36F.5B268C61-ON65257703.0012EB6A-65257703.0013F81C@in.ibm.com>
Date: Mon, 12 Apr 2010 09:08:34 +0530
From: Krishna Kumar2 <krkumar2@...ibm.com>
To: Eric Dumazet <eric.dumazet@...il.com>
Cc: David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
Denys Fedorysychenko <nuclearcat@...learcat.com>
Subject: Re: NULL pointer dereference panic in stable (2.6.33.2), amd64
Hi Eric,
Eric Dumazet <eric.dumazet@...il.com> wrote on 04/12/2010 04:05:53 AM:
> I believe the following lines from dev_pick_tx() are not the problem :
>
> if (sk && sk->sk_dst_cache)
> sk_tx_queue_set(sk, queue_index);
>
> It is IMHO not safe, because route for this socket might have just
> changed and we are transmitting an old packet (queued some milli seconds
> before, when route was different).
>
> We then memorize a queue_index that might be too big for the new device
> of new selected route.
>
> Next packet we want to transmit will take the cached value of
> queue_index, correct for old device, maybe not correct for new device.
When route changes, I think my patch had reset sk->sk_tx_queue_mapping
by calling sk_tx_queue_clear. I don't know if I missed any path where
the route changes and sk_dst_reset() was not called.
The following might be better to prove the panic is due to this, since
your suggestion will hide a panic that happens somewhat rare (according
to Denys):
if (sk_tx_queue_recorded(sk)) {
queue_index = sk_tx_queue_get(sk);
+ queue_index = dev_cap_txqueue(dev, queue_index);
} else {
Thanks,
- KK
> You could try to revert commit a4ee3ce3293dc931fab19beb472a8bde1295aebe
>
> commit a4ee3ce3293dc931fab19beb472a8bde1295aebe
> Author: Krishna Kumar <krkumar2@...ibm.com>
> Date: Mon Oct 19 23:50:07 2009 +0000
>
> net: Use sk_tx_queue_mapping for connected sockets
>
> For connected sockets, the first run of dev_pick_tx saves the
> calculated txq in sk_tx_queue_mapping. This is not saved if
> either the device has a queue select or the socket is not
> connected. Next iterations of dev_pick_tx uses the cached value
> of sk_tx_queue_mapping.
>
> Signed-off-by: Krishna Kumar <krkumar2@...ibm.com>
> Signed-off-by: David S. Miller <davem@...emloft.net>
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 28b0b9e..fa88dcd 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1791,13 +1791,25 @@ EXPORT_SYMBOL(skb_tx_hash);
> static struct netdev_queue *dev_pick_tx(struct net_device *dev,
> struct sk_buff *skb)
> {
> - const struct net_device_ops *ops = dev->netdev_ops;
> - u16 queue_index = 0;
> + u16 queue_index;
> + struct sock *sk = skb->sk;
> +
> + if (sk_tx_queue_recorded(sk)) {
> + queue_index = sk_tx_queue_get(sk);
> + } else {
> + const struct net_device_ops *ops = dev->netdev_ops;
>
> - if (ops->ndo_select_queue)
> - queue_index = ops->ndo_select_queue(dev, skb);
> - else if (dev->real_num_tx_queues > 1)
> - queue_index = skb_tx_hash(dev, skb);
> + if (ops->ndo_select_queue) {
> + queue_index = ops->ndo_select_queue(dev, skb);
> + } else {
> + 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);
> + }
> + }
>
> skb_set_queue_mapping(skb, queue_index);
> return netdev_get_tx_queue(dev, queue_index);
>
>
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists