[PATCH] Simplify net/flow.c

[PATCH] Simplify net/flow.c

[Rusty Russell]

Hi Dave,

	I still have this cleanup sitting around, and frankly it gets
in the way for the hotplug CPU code.  It works fine here, and is
basically trivial, but I'm happy to put it in -mm first for wider
testing if you want?

Thanks,
Rusty.

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 complex: it tries to allocate
D: flow cache as each CPU comes up.  It might as well allocate them for
D: each possible CPU at boot.

diff -urpN --exclude TAGS -X /home/rusty/devel/kernel/kernel-patches/current-dontdiff --minimal .16782-linux-2.6.1-bk5/net/core/flow.c .16782-linux-2.6.1-bk5.updated/net/core/flow.c
--- .16782-linux-2.6.1-bk5/net/core/flow.c	2003-11-24 15:42:33.000000000 +1100
+++ .16782-linux-2.6.1-bk5.updated/net/core/flow.c	2004-01-20 17:54:01.000000000 +1100
@@ -66,24 +66,18 @@ static struct timer_list flow_hash_rnd_t
 
 struct flow_flush_info {
 	atomic_t cpuleft;
-	cpumask_t 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 cpumask_t 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 (cpu_isset(i, flow_cache_cpu_map))
-			flow_hash_rnd_recalc(i) = 1;
+	for_each_cpu(i)
+		flow_hash_rnd_recalc(i) = 1;
 
 	flow_hash_rnd_timer.expires = jiffies + FLOW_HASH_RND_PERIOD;
 	add_timer(&flow_hash_rnd_timer);
@@ -179,7 +173,9 @@ void *flow_cache_lookup(struct flowi *ke
 	cpu = smp_processor_id();
 
 	fle = NULL;
-	if (!cpu_isset(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))
@@ -278,8 +274,6 @@ static void flow_cache_flush_per_cpu(voi
 	struct tasklet_struct *tasklet;
 
 	cpu = smp_processor_id();
-	if (!cpu_isset(cpu, info->cpumap))
-		return;
 
 	tasklet = flow_flush_tasklet(cpu);
 	tasklet->data = (unsigned long)info;
@@ -289,29 +283,23 @@ static void flow_cache_flush_per_cpu(voi
 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 (cpu_isset(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;
@@ -324,9 +312,8 @@ static int __devinit flow_cache_cpu_prep
 
 	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);
 
@@ -335,39 +322,8 @@ static int __devinit flow_cache_cpu_prep
 
 	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);
-	cpu_set(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;
@@ -389,15 +345,8 @@ static int __init flow_cache_init(void)
 	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_each_cpu(i)
+		flow_cache_cpu_prepare(i);
 
 	return 0;
 }

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

Re: [PATCH] Simplify net/flow.c

[David S. Miller]

On Thu, 22 Jan 2004 18:49:37 +1100
Rusty Russell <rusty@rustcorp.com.au> wrote:

> 	I still have this cleanup sitting around, and frankly it gets
> in the way for the hotplug CPU code.  It works fine here, and is
> basically trivial, but I'm happy to put it in -mm first for wider
> testing if you want?

No, I made you wait long enough with this patch :) and I just read it
over again a few times and it looks perfectly fine.

So I'm applying this now, thanks Rusty.

Re: [PATCH] Simplify net/flow.c

[Andrew Morton]

"David S. Miller" <davem@redhat.com> wrote:
>
> On Thu, 22 Jan 2004 18:49:37 +1100
> Rusty Russell <rusty@rustcorp.com.au> wrote:
> 
> > 	I still have this cleanup sitting around, and frankly it gets
> > in the way for the hotplug CPU code.  It works fine here, and is
> > basically trivial, but I'm happy to put it in -mm first for wider
> > testing if you want?
> 
> No, I made you wait long enough with this patch :) and I just read it
> over again a few times and it looks perfectly fine.
> 
> So I'm applying this now, thanks Rusty.

It doesn't link if CONFIG_SMP=n.  semaphore `cpucontrol', used in
flow_cache_flush() is defined in kernel/cpu.c which is not included in
uniprocessor builds.

Here's one possible fix.


diff -puN net/core/flow.c~flow-cpucontrol-fix net/core/flow.c
--- 25/net/core/flow.c~flow-cpucontrol-fix	2004-01-22 19:39:54.000000000 -0800
+++ 25-akpm/net/core/flow.c	2004-01-22 19:43:58.000000000 -0800
@@ -286,7 +286,7 @@ void flow_cache_flush(void)
 
 	/* Don't want cpus going down or up during this, also protects
 	 * against multiple callers. */
-	down(&cpucontrol);
+	down_cpucontrol();
 	atomic_set(&info.cpuleft, num_online_cpus());
 	init_completion(&info.completion);
 
@@ -296,7 +296,7 @@ void flow_cache_flush(void)
 	local_bh_enable();
 
 	wait_for_completion(&info.completion);
-	up(&cpucontrol);
+	up_cpucontrol();
 }
 
 static void __devinit flow_cache_cpu_prepare(int cpu)
diff -puN include/linux/cpu.h~flow-cpucontrol-fix include/linux/cpu.h
--- 25/include/linux/cpu.h~flow-cpucontrol-fix	2004-01-22 19:42:23.000000000 -0800
+++ 25-akpm/include/linux/cpu.h	2004-01-22 19:43:35.000000000 -0800
@@ -37,7 +37,12 @@ extern int register_cpu_notifier(struct 
 extern void unregister_cpu_notifier(struct notifier_block *nb);
 
 int cpu_up(unsigned int cpu);
+
+#define down_cpucontrol()	down(&cpucontrol)
+#define up_cpucontrol()		up(&cpucontrol)
+
 #else
+
 static inline int register_cpu_notifier(struct notifier_block *nb)
 {
 	return 0;
@@ -45,6 +50,10 @@ static inline int register_cpu_notifier(
 static inline void unregister_cpu_notifier(struct notifier_block *nb)
 {
 }
+
+#define down_cpucontrol()	do { } while (0)
+#define up_cpucontrol()		do { } while (0)
+
 #endif /* CONFIG_SMP */
 extern struct sysdev_class cpu_sysdev_class;
 

_

Re: [PATCH] Simplify net/flow.c

[Rusty Russell]

In message <20040122195104.31cc2496.akpm@osdl.org> you write:
> It doesn't link if CONFIG_SMP=n.  semaphore `cpucontrol', used in
> flow_cache_flush() is defined in kernel/cpu.c which is not included in
> uniprocessor builds.
> 
> Here's one possible fix.

....

>  	/* Don't want cpus going down or up during this, also protects
>  	 * against multiple callers. */
> -	down(&cpucontrol);
> +	down_cpucontrol();

OK.  Although I think I prefer to have down_cpucontrol() defined under
#ifdef CONFIG_HOTPLUG_CPU, and revert to using a normal sem here as
well to cover the CONFIG_HOTPLUG_CPU=n CONFIG_SMP=y case.  But I will
produce an additional patch with the hotplug cpu patches.

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

Re: [PATCH] Simplify net/flow.c

[David S. Miller]

On Thu, 22 Jan 2004 19:51:04 -0800
Andrew Morton <akpm@osdl.org> wrote:

> "David S. Miller" <davem@redhat.com> wrote:
> >
> > So I'm applying this now, thanks Rusty.
> 
> It doesn't link if CONFIG_SMP=n.  semaphore `cpucontrol', used in
> flow_cache_flush() is defined in kernel/cpu.c which is not included in
> uniprocessor builds.
> 
> Here's one possible fix.

Crap, please push this to Linus if he takes the networking update I sent
him earlier today.

Thanks for catching this Andrew.

Re: [PATCH] Simplify net/flow.c

[David S. Miller]

   From: Rusty Russell <rusty@rustcorp.com.au>
   Date: Fri, 23 Jan 2004 17:55:28 +1100

   In message <20040122195104.31cc2496.akpm@osdl.org> you write:
   > -	down(&cpucontrol);
   > +	down_cpucontrol();
   
   OK.  Although I think I prefer to have down_cpucontrol() defined under
   #ifdef CONFIG_HOTPLUG_CPU, and revert to using a normal sem here as
   well to cover the CONFIG_HOTPLUG_CPU=n CONFIG_SMP=y case.  But I will
   produce an additional patch with the hotplug cpu patches.

Rusty take a look at Linus's current BK tree, it should be
to your satisfaction I think :)