[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <4AFC2563.1020902@trash.net>
Date: Thu, 12 Nov 2009 16:10:27 +0100
From: Patrick McHardy <kaber@...sh.net>
To: Changli Gao <xiaosuo@...il.com>
CC: "David S. Miller" <davem@...emloft.net>,
Stephen Hemminger <shemminger@...tta.com>,
Eric Dumazet <eric.dumazet@...il.com>,
Tom Herbert <therbert@...gle.com>, netdev@...r.kernel.org
Subject: Re: [PATCH] ifb: add multi-queue support
Changli Gao wrote:
> On Wed, Nov 11, 2009 at 11:59 PM, Patrick McHardy <kaber@...sh.net> wrote:
>>> +static int numtxqs = 1;
>>> +module_param(numtxqs, int, 0444);
>>> +MODULE_PARM_DESC(numtxqs, "Number of TX queues per ifb");
>> unsigned?
>
> Yea, unsigned is better, and I need to check whether its value is
> smaller than 1 somewhere. The same will be done with IFLA_NTXQ.
Good point.
>>> + rcu_read_lock();
>>> + skb->dev = dev_get_by_index_rcu(&init_net, skb->iif);
>>> + if (!skb->dev) {
>>> + rcu_read_unlock();
>>> + dev_kfree_skb(skb);
>>> + txq->tx_dropped++;
>>> + break;
>>> + }
>>> + rcu_read_unlock();
>>> + skb->iif = dev->ifindex;
>> What protects the device from disappearing here and below during
>> dev_queue_xmit() and netif_rx_ni()?
>
> For dev_queue_xmit(), dev is holded by skb->_dst, so there is no
> problem. But for netif_rx_ni(), I don't know how to prevent the device
> disappearing, and it seems that all the NIC drivers have this problem.
> Maybe there was the assumption about the execution context of
> netif_rx() before. Now softirq can't be executed by softirqd, so the
> packet receiving path maybe interleaved. I don't know how to prevent
> it happening.
You can either take a reference of move the rcu_read_unlock()
after the skb submission.
>>> + break;
>>> + case __constant_htons(ETH_P_IPV6):
>>> + ip_proto = ipv6_hdr(skb)->nexthdr;
>>> + addr1 = ipv6_hdr(skb)->saddr.s6_addr32[3];
>>> + addr2 = ipv6_hdr(skb)->daddr.s6_addr32[3];
>>> + ihl = 10;
>> Where does 10 come from?
>
> It should be 40, after reviewing IPV6, I found that I need to loop
> until finding the right protocol value.
There is a helper for this (ipv6_skip_exthdr).
>>> +static int ifb_get_tx_queues(struct net *net, struct nlattr *tb[],
>>> + unsigned int *num_tx_queues,
>>> + unsigned int *real_num_tx_queues)
>>> +{
>>> + if (tb[IFLA_NTXQ]) {
>>> + *num_tx_queues = nla_get_u16(tb[IFLA_NTXQ]);
>> We currently use unsigned ints for the queue number, so please
>> use an u32 for the attribute as well.
>>
>>> + *real_num_tx_queues = *num_tx_queues;
>>> + } else {
>>> + *num_tx_queues = numtxqs;
>>> + *real_num_tx_queues = numtxqs;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
> u16 (*ndo_select_queue)(struct net_device *dev,
> struct sk_buff *skb);
> use u16 as the return value so ..., and I think u16 is big enough. If
> you insist on this, I'll use u32 instead.
Well, get_tx_queues() uses unsigned int, as does struct net_device.
I agree that it probably won't ever be needed, but there's no harm
in using the correct type, the attribute encoding doesn't even need
more room.
--
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