Simplify net/flow.c
Subsystems (2)
PATCH 1/1 Simplify net/flow.c
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.
net/core/flow.c +15 -66
@@ -66,24 +66,18 @@ static struct timer_list flow_hash_rnd_tstruct 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(voistruct 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(voivoid 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_prepflow_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.
Quoted 4 lines · On Thu, 22 Jan 2004 18:49:37 +1100 Rusty Russell <[email protected]> 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.
Quoted 9 lines · David S. Miller
Quoted 4 lines · On Thu, 22 Jan 2004 18:49:37 +1100 Rusty Russell <[email protected]> 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?
Quoted 4 lines
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.
net/core/flow.c renamed from net/core/flow.c~flow-cpucontrol-fix +2 -2
@@ -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)
include/linux/cpu.h renamed from include/linux/cpu.h~flow-cpucontrol-fix +9 -0
@@ -37,7 +37,12 @@ extern int register_cpu_notifier(structextern 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;
_
Quoted 9 lines · On Thu, 22 Jan 2004 19:51:04 -0800 Andrew Morton <[email protected]> wrote:
Quoted 2 lines · David S. Miller
So I'm applying this now, thanks Rusty.
It doesn't link if
CONFIG_SMP=n. semaphore `cpucontrol', used inflow_cache_flush()is defined inkernel/cpu.cwhich 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.
In message <[email protected]> you write:
Quoted 5 lines
It doesn't link if
CONFIG_SMP=n. semaphore `cpucontrol', used inflow_cache_flush()is defined inkernel/cpu.cwhich is not included in uniprocessor builds.Here's one possible fix.
....
net/core/flow.c void flow_cache_flush(void) · line 289
Quoted 4 lines
net/core/flow.c renamed from net/core/flow.c~flow-cpucontrol-fix +1
-1/* 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.
From: Rusty Russell <[email protected]>
Date: Fri, 23 Jan 2004 17:55:28 +1100
In message <[email protected]> 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 :)