9 messages, 4 authors, 2008-01-15 · open the first message on its own page
From: David Miller <davem@davemloft.net>
Date: 2008-01-13 04:55:20
From: Stephen Hemminger <redacted> Date: Fri, 11 Jan 2008 22:45:17 -0800
Turn the unused size field into a useful counter for the number of routes. Signed-off-by: Stephen Hemminger <redacted>
It's not useful if nothing reports it's value. I'm dropping this. Unless you add code to provide this information via procfs or similar, better to just remove it.
From: Stephen Hemminger <hidden>
Date: 2008-01-13 05:33:32
David Miller wrote:
From: Stephen Hemminger <redacted> Date: Fri, 11 Jan 2008 22:45:17 -0800quoted
Turn the unused size field into a useful counter for the number of routes. Signed-off-by: Stephen Hemminger <redacted>It's not useful if nothing reports it's value. I'm dropping this. Unless you add code to provide this information via procfs or similar, better to just remove it.
The size field is added to /proc/net/fib_triestat that was the point.
From: David Miller <davem@davemloft.net>
Date: 2008-01-13 05:44:18
From: Stephen Hemminger <redacted> Date: Sat, 12 Jan 2008 21:33:16 -0800
The size field is added to /proc/net/fib_triestat that was the point.
Not from what I can see. davem@sunset:~/src/GIT/net-2.6.25$ git grep -e "->size" net/ipv4/fib_trie.c net/ipv4/fib_trie.c: t->size++; net/ipv4/fib_trie.c: t->size--; davem@sunset:~/src/GIT/net-2.6.25$ git grep -e "\.size" net/ipv4/fib_trie.c davem@sunset:~/src/GIT/net-2.6.25$ Nothing uses the field, it is merely incremented and decremented. trie_show_stats() and trie_show_usage() do not access this field.
From: Stephen Hemminger <hidden>
Date: 2008-01-14 21:01:10
Show number of entries in trie, the size field was being set but never used, but it only counted leaves, not all entries. Refactor the two cases in fib_triestat_seq_show into a single routine. Note: the stat structure was being malloc'd but the stack usage isn't so high (288 bytes) that it is worth the additional complexity. Signed-off-by: Stephen Hemminger <redacted> --- Patch against current net-2.6.25
--- a/net/ipv4/fib_trie.c 2008-01-14 10:16:06.000000000 -0800
+++ b/net/ipv4/fib_trie.c 2008-01-14 10:30:11.000000000 -0800@@ -148,10 +148,10 @@ struct trie_stat { struct trie { struct node *trie; + unsigned int size; #ifdef CONFIG_IP_FIB_TRIE_STATS struct trie_use_stats stats; #endif - int size; }; static void put_child(struct trie *t, struct tnode *tn, int i, struct node *n);
@@ -1045,7 +1045,6 @@ static struct list_head *fib_insert_node insert_leaf_info(&l->list, li); goto done; } - t->size++; l = leaf_new(); if (!l)
@@ -1258,6 +1257,8 @@ static int fn_trie_insert(struct fib_tab list_add_tail_rcu(&new_fa->fa_list, (fa ? &fa->fa_list : fa_head)); + t->size++; + rt_cache_flush(-1); rtmsg_fib(RTM_NEWROUTE, htonl(key), new_fa, plen, tb->tb_id, &cfg->fc_nlinfo, 0);
@@ -2128,50 +2129,34 @@ static void trie_show_usage(struct seq_f } #endif /* CONFIG_IP_FIB_TRIE_STATS */ +static void fib_trie_show(struct seq_file *seq, const char *name, struct trie *trie) +{ + struct trie_stat stat; + + seq_printf(seq, "%s: %d\n", name, trie->size); + trie_collect_stats(trie, &stat); + trie_show_stats(seq, &stat); +#ifdef CONFIG_IP_FIB_TRIE_STATS + trie_show_usage(seq, &trie->stats); +#endif +} static int fib_triestat_seq_show(struct seq_file *seq, void *v) { struct net *net = (struct net *)seq->private; - struct trie *trie_local, *trie_main; - struct trie_stat *stat; struct fib_table *tb; - trie_local = NULL; + seq_printf(seq, + "Basic info: size of leaf: %Zd bytes, size of tnode: %Zd bytes.\n", + sizeof(struct leaf), sizeof(struct tnode)); + tb = fib_get_table(net, RT_TABLE_LOCAL); if (tb) - trie_local = (struct trie *) tb->tb_data; - - trie_main = NULL; + fib_trie_show(seq, "Local", (struct trie *) tb->tb_data); + tb = fib_get_table(net, RT_TABLE_MAIN); if (tb) - trie_main = (struct trie *) tb->tb_data; - - - stat = kmalloc(sizeof(*stat), GFP_KERNEL); - if (!stat) - return -ENOMEM; - - seq_printf(seq, "Basic info: size of leaf: %Zd bytes, size of tnode: %Zd bytes.\n", - sizeof(struct leaf), sizeof(struct tnode)); - - if (trie_local) { - seq_printf(seq, "Local:\n"); - trie_collect_stats(trie_local, stat); - trie_show_stats(seq, stat); -#ifdef CONFIG_IP_FIB_TRIE_STATS - trie_show_usage(seq, &trie_local->stats); -#endif - } - - if (trie_main) { - seq_printf(seq, "Main:\n"); - trie_collect_stats(trie_main, stat); - trie_show_stats(seq, stat); -#ifdef CONFIG_IP_FIB_TRIE_STATS - trie_show_usage(seq, &trie_main->stats); -#endif - } - kfree(stat); + fib_trie_show(seq, "Main", (struct trie *) tb->tb_data); return 0; }
From: Stephen Hemminger <hidden>
Date: 2008-01-15 05:02:52
Initialization of the slab cache's should be done when IP is initialized to make sure of available memory, and that code can be marked __init. Signed-off-by: Stephen Hemminger <redacted> --- Applies after statistics (#1) patch for net-2.6.25 include/net/ip_fib.h | 5 +++-- net/ipv4/fib_frontend.c | 9 ++++++--- net/ipv4/fib_hash.c | 24 +++++++++++------------- net/ipv4/fib_trie.c | 16 ++++++++-------- 4 files changed, 28 insertions(+), 26 deletions(-)
--- a/include/net/ip_fib.h 2008-01-14 14:59:32.000000000 -0800
+++ b/include/net/ip_fib.h 2008-01-14 15:01:12.000000000 -0800@@ -231,8 +231,9 @@ extern int fib_sync_down(__be32 local, s extern int fib_sync_up(struct net_device *dev); extern __be32 __fib_res_prefsrc(struct fib_result *res); -/* Exported by fib_hash.c */ -extern struct fib_table *fib_hash_init(u32 id); +/* Exported by fib_{hash|trie}.c */ +extern void fib_hash_init(void); +extern struct fib_table *fib_hash_table(u32 id); static inline void fib_combine_itag(u32 *itag, struct fib_result *res) { --- a/net/ipv4/fib_frontend.c 2008-01-14 14:59:32.000000000 -0800 +++ b/net/ipv4/fib_frontend.c 2008-01-14 15:01:12.000000000 -0800
@@ -53,11 +53,11 @@ static int __net_init fib4_rules_init(st { struct fib_table *local_table, *main_table; - local_table = fib_hash_init(RT_TABLE_LOCAL); + local_table = fib_hash_table(RT_TABLE_LOCAL); if (local_table == NULL) return -ENOMEM; - main_table = fib_hash_init(RT_TABLE_MAIN); + main_table = fib_hash_table(RT_TABLE_MAIN); if (main_table == NULL) goto fail;
@@ -83,7 +83,8 @@ struct fib_table *fib_new_table(struct n tb = fib_get_table(net, id); if (tb) return tb; - tb = fib_hash_init(id); + + tb = fib_hash_table(id); if (!tb) return NULL; h = id & (FIB_TABLE_HASHSZ - 1);
@@ -1042,6 +1043,8 @@ void __init ip_fib_init(void) register_pernet_subsys(&fib_net_ops); register_netdevice_notifier(&fib_netdev_notifier); register_inetaddr_notifier(&fib_inetaddr_notifier); + + fib_hash_init(); } EXPORT_SYMBOL(inet_addr_type); --- a/net/ipv4/fib_hash.c 2008-01-14 14:59:32.000000000 -0800 +++ b/net/ipv4/fib_hash.c 2008-01-14 15:01:12.000000000 -0800
@@ -746,21 +746,19 @@ static int fn_hash_dump(struct fib_table return skb->len; } -struct fib_table *fib_hash_init(u32 id) +void __init fib_hash_init(void) { - struct fib_table *tb; + fn_hash_kmem = kmem_cache_create("ip_fib_hash", sizeof(struct fib_node), + 0, SLAB_HWCACHE_ALIGN|SLAB_PANIC, NULL); + + fn_alias_kmem = kmem_cache_create("ip_fib_alias", sizeof(struct fib_alias), + 0, SLAB_HWCACHE_ALIGN|SLAB_PANIC, NULL); + +} - if (fn_hash_kmem == NULL) - fn_hash_kmem = kmem_cache_create("ip_fib_hash", - sizeof(struct fib_node), - 0, SLAB_HWCACHE_ALIGN, - NULL); - - if (fn_alias_kmem == NULL) - fn_alias_kmem = kmem_cache_create("ip_fib_alias", - sizeof(struct fib_alias), - 0, SLAB_HWCACHE_ALIGN, - NULL); +struct fib_table *fib_hash_table(u32 id) +{ + struct fib_table *tb; tb = kmalloc(sizeof(struct fib_table) + sizeof(struct fn_hash), GFP_KERNEL); --- a/net/ipv4/fib_trie.c 2008-01-14 15:01:12.000000000 -0800 +++ b/net/ipv4/fib_trie.c 2008-01-14 20:57:41.000000000 -0800
@@ -1923,19 +1923,19 @@ out: return -1; } -/* Fix more generic FIB names for init later */ +void __init fib_hash_init(void) +{ + fn_alias_kmem = kmem_cache_create("ip_fib_alias", sizeof(struct fib_alias), + 0, SLAB_HWCACHE_ALIGN|SLAB_PANIC, NULL); +} -struct fib_table *fib_hash_init(u32 id) + +/* Fix more generic FIB names for init later */ +struct fib_table *fib_hash_table(u32 id) { struct fib_table *tb; struct trie *t; - if (fn_alias_kmem == NULL) - fn_alias_kmem = kmem_cache_create("ip_fib_alias", - sizeof(struct fib_alias), - 0, SLAB_HWCACHE_ALIGN, - NULL); - tb = kmalloc(sizeof(struct fib_table) + sizeof(struct trie), GFP_KERNEL); if (tb == NULL)
From: David Miller <davem@davemloft.net>
Date: 2008-01-15 07:14:37
From: Stephen Hemminger <redacted> Date: Mon, 14 Jan 2008 21:00:08 -0800
Initialization of the slab cache's should be done when IP is initialized to make sure of available memory, and that code can be marked __init. Signed-off-by: Stephen Hemminger <redacted>
Applied.
From: Eric Dumazet <hidden>
Date: 2008-01-15 06:56:13
Stephen Hemminger a écrit :
Show number of entries in trie, the size field was being set but never used, but it only counted leaves, not all entries. Refactor the two cases in fib_triestat_seq_show into a single routine. Note: the stat structure was being malloc'd but the stack usage isn't so high (288 bytes) that it is worth the additional complexity. Signed-off-by: Stephen Hemminger <redacted> --- Patch against current net-2.6.25--- a/net/ipv4/fib_trie.c 2008-01-14 10:16:06.000000000 -0800 +++ b/net/ipv4/fib_trie.c 2008-01-14 10:30:11.000000000 -0800@@ -148,10 +148,10 @@ struct trie_stat { struct trie { struct node *trie; + unsigned int size; #ifdef CONFIG_IP_FIB_TRIE_STATS struct trie_use_stats stats; #endif - int size; }; static void put_child(struct trie *t, struct tnode *tn, int i, struct node *n);@@ -1045,7 +1045,6 @@ static struct list_head *fib_insert_node insert_leaf_info(&l->list, li); goto done; } - t->size++; l = leaf_new(); if (!l)@@ -1258,6 +1257,8 @@ static int fn_trie_insert(struct fib_tab list_add_tail_rcu(&new_fa->fa_list, (fa ? &fa->fa_list : fa_head)); + t->size++; + rt_cache_flush(-1); rtmsg_fib(RTM_NEWROUTE, htonl(key), new_fa, plen, tb->tb_id, &cfg->fc_nlinfo, 0);@@ -2128,50 +2129,34 @@ static void trie_show_usage(struct seq_f } #endif /* CONFIG_IP_FIB_TRIE_STATS */ +static void fib_trie_show(struct seq_file *seq, const char *name, struct trie *trie) +{ + struct trie_stat stat; + + seq_printf(seq, "%s: %d\n", name, trie->size); + trie_collect_stats(trie, &stat); + trie_show_stats(seq, &stat); +#ifdef CONFIG_IP_FIB_TRIE_STATS + trie_show_usage(seq, &trie->stats); +#endif +} static int fib_triestat_seq_show(struct seq_file *seq, void *v) { struct net *net = (struct net *)seq->private; - struct trie *trie_local, *trie_main; - struct trie_stat *stat; struct fib_table *tb; - trie_local = NULL; + seq_printf(seq, + "Basic info: size of leaf: %Zd bytes, size of tnode: %Zd bytes.\n", + sizeof(struct leaf), sizeof(struct tnode)); + tb = fib_get_table(net, RT_TABLE_LOCAL); if (tb) - trie_local = (struct trie *) tb->tb_data; - - trie_main = NULL; + fib_trie_show(seq, "Local", (struct trie *) tb->tb_data); + tb = fib_get_table(net, RT_TABLE_MAIN); if (tb) - trie_main = (struct trie *) tb->tb_data; - - - stat = kmalloc(sizeof(*stat), GFP_KERNEL); - if (!stat) - return -ENOMEM; - - seq_printf(seq, "Basic info: size of leaf: %Zd bytes, size of tnode: %Zd bytes.\n", - sizeof(struct leaf), sizeof(struct tnode)); - - if (trie_local) { - seq_printf(seq, "Local:\n"); - trie_collect_stats(trie_local, stat); - trie_show_stats(seq, stat); -#ifdef CONFIG_IP_FIB_TRIE_STATS - trie_show_usage(seq, &trie_local->stats); -#endif - } - - if (trie_main) { - seq_printf(seq, "Main:\n"); - trie_collect_stats(trie_main, stat); - trie_show_stats(seq, stat); -#ifdef CONFIG_IP_FIB_TRIE_STATS - trie_show_usage(seq, &trie_main->stats); -#endif - } - kfree(stat); + fib_trie_show(seq, "Main", (struct trie *) tb->tb_data); return 0; }
Keeping a 'size' counter is not necessary, since trie_collect_stats() must go through all the tree and get this information for free. This 'size' field only slows down inserts/deletes and dirty a cacheline that readers will hit.
From: David Miller <davem@davemloft.net>
Date: 2008-01-15 07:28:22
From: Eric Dumazet <redacted> Date: Tue, 15 Jan 2008 07:55:57 +0100
Keeping a 'size' counter is not necessary, since trie_collect_stats() must go through all the tree and get this information for free. This 'size' field only slows down inserts/deletes and dirty a cacheline that readers will hit.
Good point, we can do this better in a follow-on patch.
From: David Miller <davem@davemloft.net>
Date: 2008-01-15 07:12:12
From: Stephen Hemminger <redacted> Date: Mon, 14 Jan 2008 12:57:55 -0800
Show number of entries in trie, the size field was being set but never used, but it only counted leaves, not all entries. Refactor the two cases in fib_triestat_seq_show into a single routine. Note: the stat structure was being malloc'd but the stack usage isn't so high (288 bytes) that it is worth the additional complexity. Signed-off-by: Stephen Hemminger <redacted>
Applied, thanks.