From: Andi Kleen <andi@firstfloor•org>
To: Tom Herbert <therbert@google•com>
Cc: netdev@vger•kernel.org, David Miller <davem@davemloft•net>
Subject: Re: [PATCH v2] Receive Packet Steering
Date: Mon, 04 May 2009 09:59:07 +0200 [thread overview]
Message-ID: <873abli9is.fsf@basil.nowhere.org> (raw)
In-Reply-To: <65634d660905032103h614225dbg9911e290f5537fbf@mail.gmail.com> (Tom Herbert's message of "Sun, 3 May 2009 21:03:01 -0700")
Tom Herbert <therbert@google•com> writes:
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 052dd47..3107544 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -1906,6 +1906,142 @@ int weight_p __read_mostly = 64; /*
> old backlog weight */
>
> DEFINE_PER_CPU(struct netif_rx_stats, netdev_rx_stat) = { 0, };
>
> +static u32 simple_hashrnd;
This should be __read_mostly
> +static int simple_hashrnd_initialized;
Also I suspect you can just use 0 as uninitialized
and avoid one variable. If the RNG reports 0 you get
worst case another initialization.
> +static int get_rps_cpu(struct net_device *dev, struct sk_buff *skb)
> +{
> + u32 addr1, addr2, ports;
> + struct ipv6hdr *ip6;
> + struct iphdr *ip;
> + u32 hash, ihl;
> + u8 ip_proto;
> + int cpu;
> +
> + if (!dev->rps_map_len)
> + return -1;
> +
> + if (unlikely(!simple_hashrnd_initialized)) {
> + get_random_bytes(&simple_hashrnd, 4);
> + simple_hashrnd_initialized = 1;
> + }
The usual problem of this is that if the kernel gets a packet sufficiently
early the random state will be always the default state, which is not
very random.
So either you use a timeout to reinit regularly (like other subsystems
do) or just use a fixed value for reproducibility. I suspect the
later would be nicer for benchmakers.
+ case __constant_htons(ETH_P_IPV6):
+ if (!pskb_may_pull(skb, sizeof(*ip6)))
+ return -1;
+
+ ip6 = (struct ipv6hdr *) skb->data;
+ ip_proto = ip6->nexthdr;
+ addr1 = ip6->saddr.s6_addr32[3];
+ addr2 = ip6->daddr.s6_addr32[3];
Wouldn't it be better to hash in everything in the ipv6 address in this case?
> +
> + hash = jhash_3words(addr1, addr2, ports, simple_hashrnd);
> +
> + cpu = skb->dev->rps_map[((u64) hash * dev->rps_map_len) >> 32];
For 32bit systems it would be nice to avoid the u64 cast. gcc doesn't
generate very good code for that.
> + return cpu_online(cpu) ? cpu : -1;
I suspect this is still racy with cpu hotunplug.
> + cpus_clear(__get_cpu_var(rps_remote_softirq_cpus));
> +
> + local_irq_enable();
> +}
> +
> +/**
> + * enqueue_to_backlog is called to queue an skb to a per CPU backlog
> + * queue (may be a remote CPU queue).
> + */
> +static int enqueue_to_backlog(struct sk_buff *skb, int cpu)
> +{
> + struct softnet_data *queue;
> + unsigned long flags;
> +
> + queue = &per_cpu(softnet_data, cpu);
Are you sure preemption is disabled here? Otherwise this must be
one line below (can be tested by enabling preempt & preempt debug)
> +
> + if (!capable(CAP_NET_ADMIN))
> + return -EPERM;
> +
> + err = bitmap_parse(buf, len, cpumask_bits(&mask), nr_cpumask_bits);
> + if (err)
> + return err;
> +
> + rtnl_lock();
> + if (dev_isalive(net)) {
> + if (!net->rps_map) {
> + net->rps_map = kzalloc(sizeof(u16) *
> + num_possible_cpus(), GFP_KERNEL);
> + if (!net->rps_map)
> + return -ENOMEM;
You don't unlock rtnl_lock here.
-Andi
--
ak@linux•intel.com -- Speaking for myself only.
next prev parent reply other threads:[~2009-05-04 7:59 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2009-05-04 4:03 [PATCH v2] Receive Packet Steering Tom Herbert
2009-05-04 5:30 ` Eric Dumazet
2009-05-04 6:10 ` Eric Dumazet
2009-05-04 7:13 ` Eric Dumazet
2009-05-12 17:28 ` Tom Herbert
2009-05-04 7:08 ` Eric Dumazet
2009-05-04 7:59 ` Andi Kleen [this message]
2009-05-04 18:22 ` Jarek Poplawski
2009-05-04 20:43 ` Jarek Poplawski
2009-06-10 8:23 ` David Miller
2009-06-15 5:54 ` Tom Herbert
[not found] ` <65634d660906142252y6f7fc021l844b172995c10044@mail.gmail.com>
2009-06-15 9:02 ` David Miller
2009-06-15 16:39 ` Tom Herbert
2009-06-15 23:18 ` David Miller
2009-07-13 17:49 ` David Miller
2009-07-13 22:04 ` Tom Herbert
2009-07-14 19:33 ` David Miller
2009-07-14 23:28 ` Tom Herbert
2009-07-17 2:48 ` David Miller
2009-07-17 18:05 ` Tom Herbert
2009-07-17 18:08 ` David Miller
2009-07-17 19:59 ` Tom Herbert
2009-07-18 3:54 ` David Miller
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=873abli9is.fsf@basil.nowhere.org \
--to=andi@firstfloor$(echo .)org \
--cc=davem@davemloft$(echo .)net \
--cc=netdev@vger$(echo .)kernel.org \
--cc=therbert@google$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox