net/core/flow.c cpu handling?

net/core/flow.c cpu handling?

[Rusty Russell]

Hi again,

	Is there something I'm missing, or wouldn't the code in
net/core/flow.c be much simpler if:

1) flow_cache_cpu_prepare() were done for each possible cpu, not
   dynamically as they came up.

2) The cpucontrol lock is held in flow_cache_flush to guarantee that
   cpus don't go up and down.

3) The normal cpu_online_map were used instead of flow_cache_cpu_map.

The patch below is untested, but if you can't see anything wrong with
the approach, I'll test it.

Cheers,
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Name: Simplify CPU Handling in net/core/flow.c
Author: Rusty Russell
Status: Booted on 2.6.0-test9-bk6

D: The cpu handling in net/core/flow.c is flawed, because it uses an
D: unsigned long instead of a cpumask_t.  This replaces that code
D: with a much simpler version which allocates for every possible CPU,
D: (the same amount of work on most machines) and takes the cpucontrol
D: semaphore when flushing.

--- linux-2.6.0-test9-bk4/net/core/flow.c	2003-10-09 18:03:03.000000000 +1000
+++ working-2.6.0-test9-bk4-hotcpu-with-kthread/net/core/flow.c	2003-11-02 17:10:55.000000000 +1100
@@ -65,23 +65,18 @@
 
 struct flow_flush_info {
 	atomic_t cpuleft;
-	unsigned long cpumap;
 	struct completion completion;
 };
 static DEFINE_PER_CPU(struct tasklet_struct, flow_flush_tasklets) = { NULL };
 
 #define flow_flush_tasklet(cpu) (&per_cpu(flow_flush_tasklets, cpu))
 
-static DECLARE_MUTEX(flow_cache_cpu_sem);
-static unsigned long flow_cache_cpu_map;
-static unsigned int flow_cache_cpu_count;
-
 static void flow_cache_new_hashrnd(unsigned long arg)
 {
 	int i;
 
 	for (i = 0; i < NR_CPUS; i++)
-		if (test_bit(i, &flow_cache_cpu_map))
+		if (cpu_possible(i))
 			flow_hash_rnd_recalc(i) = 1;
 
 	flow_hash_rnd_timer.expires = jiffies + FLOW_HASH_RND_PERIOD;
@@ -178,7 +173,9 @@
 	cpu = smp_processor_id();
 
 	fle = NULL;
-	if (!test_bit(cpu, &flow_cache_cpu_map))
+	/* Packet really early in init?  Making flow_cache_init a
+	 * pre-smp initcall would solve this.  --RR */
+	if (!flow_table(cpu))
 		goto nocache;
 
 	if (flow_hash_rnd_recalc(cpu))
@@ -277,9 +274,6 @@
 	struct tasklet_struct *tasklet;
 
 	cpu = smp_processor_id();
-	if (!test_bit(cpu, &info->cpumap))
-		return;
-
 	tasklet = flow_flush_tasklet(cpu);
 	tasklet->data = (unsigned long)info;
 	tasklet_schedule(tasklet);
@@ -288,29 +282,23 @@
 void flow_cache_flush(void)
 {
 	struct flow_flush_info info;
-	static DECLARE_MUTEX(flow_flush_sem);
-
-	down(&flow_cache_cpu_sem);
-	info.cpumap = flow_cache_cpu_map;
-	atomic_set(&info.cpuleft, flow_cache_cpu_count);
-	up(&flow_cache_cpu_sem);
 
+	/* Don't want cpus going down or up during this, also protects
+	 * against multiple callers. */
+	down(&cpucontrol);
+	atomic_set(&info.cpuleft, num_online_cpus());
 	init_completion(&info.completion);
 
-	down(&flow_flush_sem);
-
 	local_bh_disable();
 	smp_call_function(flow_cache_flush_per_cpu, &info, 1, 0);
-	if (test_bit(smp_processor_id(), &info.cpumap))
-		flow_cache_flush_tasklet((unsigned long)&info);
+	flow_cache_flush_tasklet((unsigned long)&info);
 	local_bh_enable();
 
 	wait_for_completion(&info.completion);
-
-	up(&flow_flush_sem);
+	up(&cpucontrol);
 }
 
-static int __devinit flow_cache_cpu_prepare(int cpu)
+static void __devinit flow_cache_cpu_prepare(int cpu)
 {
 	struct tasklet_struct *tasklet;
 	unsigned long order;
@@ -323,9 +311,8 @@
 
 	flow_table(cpu) = (struct flow_cache_entry **)
 		__get_free_pages(GFP_KERNEL, order);
-
 	if (!flow_table(cpu))
-		return NOTIFY_BAD;
+		panic("NET: failed to allocate flow cache order %lu\n", order);
 
 	memset(flow_table(cpu), 0, PAGE_SIZE << order);
 
@@ -334,39 +321,8 @@
 
 	tasklet = flow_flush_tasklet(cpu);
 	tasklet_init(tasklet, flow_cache_flush_tasklet, 0);
-
-	return NOTIFY_OK;
-}
-
-static int __devinit flow_cache_cpu_online(int cpu)
-{
-	down(&flow_cache_cpu_sem);
-	set_bit(cpu, &flow_cache_cpu_map);
-	flow_cache_cpu_count++;
-	up(&flow_cache_cpu_sem);
-
-	return NOTIFY_OK;
-}
-
-static int __devinit flow_cache_cpu_notify(struct notifier_block *self,
-					   unsigned long action, void *hcpu)
-{
-	unsigned long cpu = (unsigned long)cpu;
-	switch (action) {
-	case CPU_UP_PREPARE:
-		return flow_cache_cpu_prepare(cpu);
-		break;
-	case CPU_ONLINE:
-		return flow_cache_cpu_online(cpu);
-		break;
-	}
-	return NOTIFY_OK;
 }
 
-static struct notifier_block __devinitdata flow_cache_cpu_nb = {
-	.notifier_call = flow_cache_cpu_notify,
-};
-
 static int __init flow_cache_init(void)
 {
 	int i;
@@ -388,15 +344,9 @@
 	flow_hash_rnd_timer.expires = jiffies + FLOW_HASH_RND_PERIOD;
 	add_timer(&flow_hash_rnd_timer);
 
-	register_cpu_notifier(&flow_cache_cpu_nb);
-	for (i = 0; i < NR_CPUS; i++) {
-		if (!cpu_online(i))
-			continue;
-		if (flow_cache_cpu_prepare(i) == NOTIFY_OK &&
-		    flow_cache_cpu_online(i) == NOTIFY_OK)
-			continue;
-		panic("NET: failed to initialise flow cache hash table\n");
-	}
+	for (i = 0; i < NR_CPUS; i++)
+		if (cpu_possible(i))
+			flow_cache_cpu_prepare(i);
 
 	return 0;
 }

Re: net/core/flow.c cpu handling?

[David S. Miller]

Very likely, the code is how it is in order to make the
2.4.x backport of this code and the 2.6.x version as
similar as humanly possible.

Re: net/core/flow.c cpu handling?

[Rusty Russell]

In message <20031101225050.4de96a8b.davem@redhat.com> you write:
> 
> Very likely, the code is how it is in order to make the
> 2.4.x backport of this code and the 2.6.x version as
> similar as humanly possible.

Hmm, AFAICT the patch I sent should be easier, not harder to backport.

Anyway, I'm carrying the patch as part of the hotplug CPU patches: we
can discuss it once 2.6.0 is out.

Thanks,
Rusty.
--
  Anyone who quotes me in their sig is an idiot. -- Rusty Russell.

Re: net/core/flow.c cpu handling?

[Christoph Hellwig]

On Sat, Nov 01, 2003 at 10:50:50PM -0800, David S. Miller wrote:
> 
> Very likely, the code is how it is in order to make the
> 2.4.x backport of this code and the 2.6.x version as
> similar as humanly possible.

Well, the current code is wrong for 2.6 and breaks badly for machines
with more than 32/64 cpus.

-- 
Christoph Hellwig <hch@lst.de>		-	Freelance Hacker
Contact me for driver hacking and kernel development consulting

Re: net/core/flow.c cpu handling?

[David S. Miller]

On Mon, 03 Nov 2003 21:26:40 +1100
Rusty Russell <rusty@rustcorp.com.au> wrote:

> In message <20031101225050.4de96a8b.davem@redhat.com> you write:
> > 
> > Very likely, the code is how it is in order to make the
> > 2.4.x backport of this code and the 2.6.x version as
> > similar as humanly possible.
> 
> Hmm, AFAICT the patch I sent should be easier, not harder to backport.
> 
> Anyway, I'm carrying the patch as part of the hotplug CPU patches: we
> can discuss it once 2.6.0 is out.

I intend to review your patch today, there is no justification
for the problems you've pointed out in this code regardless of
how difficult or easy fixing it makes a backport.

Re: net/core/flow.c cpu handling?

[David S. Miller]

On Mon, 3 Nov 2003 15:11:45 +0000
Christoph Hellwig <hch@infradead.org> wrote:

> On Sat, Nov 01, 2003 at 10:50:50PM -0800, David S. Miller wrote:
> > 
> > Very likely, the code is how it is in order to make the
> > 2.4.x backport of this code and the 2.6.x version as
> > similar as humanly possible.
> 
> Well, the current code is wrong for 2.6 and breaks badly for machines
> with more than 32/64 cpus.

I totally understand, I am in no way trying to justify what
the code is doing.