[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date: Wed, 11 Nov 2009 16:59:32 +0100
From: Patrick McHardy <kaber@...sh.net>
To: 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:
> diff --git a/drivers/net/ifb.c b/drivers/net/ifb.c
> index 69c2566..ac04e85 100644
> --- a/drivers/net/ifb.c
> +++ b/drivers/net/ifb.c
> ...
> +/* Number of ifb devices to be set up by this module. */
> static int numifbs = 2;
> +module_param(numifbs, int, 0444);
> +MODULE_PARM_DESC(numifbs, "Number of ifb devices");
>
> -static void ri_tasklet(unsigned long dev);
> -static netdev_tx_t ifb_xmit(struct sk_buff *skb, struct net_device *dev);
> -static int ifb_open(struct net_device *dev);
> -static int ifb_close(struct net_device *dev);
> +/* Number of TX queues per ifb */
> +static int numtxqs = 1;
> +module_param(numtxqs, int, 0444);
> +MODULE_PARM_DESC(numtxqs, "Number of TX queues per ifb");
unsigned?
> + while ((skb = skb_dequeue(&pq->tq)) != NULL) {
> + u32 from = G_TC_FROM(skb->tc_verd);
> +
> + skb->tc_verd = 0;
> + skb->tc_verd = SET_TC_NCLS(skb->tc_verd);
> + txq->tx_packets++;
> + txq->tx_bytes +=skb->len;
> +
> + 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()?
> +
> + if (from & AT_EGRESS) {
> + dev_queue_xmit(skb);
> + } else if (from & AT_INGRESS) {
> + skb_pull(skb, skb->dev->hard_header_len);
> + netif_rx_ni(skb);
> + } else
> + BUG();
> }
> +static u32 simple_tx_hashrnd;
> +
> +static u16 ifb_select_queue(struct net_device *dev, struct sk_buff *skb)
> +{
> + u32 addr1, addr2;
> + u32 hash, ihl;
> + union {
> + u16 in16[2];
> + u32 in32;
> + } ports;
> + u8 ip_proto;
> +
> + if ((hash = skb_rx_queue_recorded(skb))) {
> + while (hash >= dev->real_num_tx_queues)
> + hash -= dev->real_num_tx_queues;
> + return hash;
> + }
> +
> + switch (skb->protocol) {
> + case __constant_htons(ETH_P_IP):
> + if (!(ip_hdr(skb)->frag_off & htons(IP_MF | IP_OFFSET)))
> + ip_proto = ip_hdr(skb)->protocol;
> + else
> + ip_proto = 0;
So fragments will get reordered?
> + addr1 = ip_hdr(skb)->saddr;
> + addr2 = ip_hdr(skb)->daddr;
> + ihl = ip_hdr(skb)->ihl << 2;
ip_hdrlen()?
> + 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?
> + break;
> + default:
> + return 0;
Perhaps hash on skb->protocol here.
> + }
> + if (addr1 > addr2)
> + swap(addr1, addr2);
> +
> + switch (ip_proto) {
> + case IPPROTO_TCP:
> + case IPPROTO_UDP:
> + case IPPROTO_DCCP:
> + case IPPROTO_ESP:
> + case IPPROTO_AH:
> + case IPPROTO_SCTP:
> + case IPPROTO_UDPLITE:
> + ports.in32 = *((u32 *) (skb_network_header(skb) + ihl));
> + if (ports.in16[0] > ports.in16[1])
> + swap(ports.in16[0], ports.in16[1]);
> + break;
> +
> + default:
> + ports.in32 = 0;
> + break;
> + }
> +
> + hash = jhash_3words(addr1, addr2, ports.in32,
> + simple_tx_hashrnd ^ ip_proto);
> +
> + return (u16) (((u64) hash * dev->real_num_tx_queues) >> 32);
> +}
> +
> +static int ifb_init(struct net_device *dev)
> +{
> + struct ifb_private *dp = netdev_priv(dev);
> + struct ifb_private_q *pq = dp->pq;
> + int i;
> +
> + pq = kmalloc(sizeof(*pq) * dev->real_num_tx_queues, GFP_KERNEL);
kcalloc()?
> + if (pq == NULL)
> + return -ENOMEM;
> + dp->pq = pq;
> +
> + for (i = 0; i < dev->real_num_tx_queues; i++) {
> + pq[i].dev = dev;
> + skb_queue_head_init(&pq[i].rq);
> + skb_queue_head_init(&pq[i].tq);
> + init_waitqueue_head(&pq[i].wq);
> + pq[i].rx_packets = 0;
> + pq[i].rx_bytes = 0;
> + pq[i].rx_dropped = 0;
> + }
> +
> + return 0;
> +}
> +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;
> +}
> +
--
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