Messages in this thread | | | Date | Mon, 25 May 2009 17:59:40 +0200 | From | Frederic Weisbecker <> | Subject | Re: [PATCH 2/3] tracing/stat: simplify rbtree freeing code |
| |
On Mon, May 25, 2009 at 04:46:29PM +0800, Li Zefan wrote: > When closing a trace_stat file, we destroy the rbtree constructed during > file open, but there is memory leak that the root node is not freed: > > static struct rb_node *release_next(struct rb_node *node) > { > ... > else { > if (!parent) <-- here we leak @node > return NULL; > ... > } > > This patch keeps removing root node until the tree is empty. We regress > from O(n) to O(nlogn), but since both open() and read() are O(nlogn) and > it's a slow path, this change won't affect scalibility. > > [ Impact: fix memory leak when closing a trace_stat file ] > > Signed-off-by: Li Zefan <lizf@cn.fujitsu.com> > --- > kernel/trace/trace_stat.c | 39 +++++---------------------------------- > 1 files changed, 5 insertions(+), 34 deletions(-) > > diff --git a/kernel/trace/trace_stat.c b/kernel/trace/trace_stat.c > index 6efbcb4..ed18701 100644 > --- a/kernel/trace/trace_stat.c > +++ b/kernel/trace/trace_stat.c > @@ -42,47 +42,18 @@ static DEFINE_MUTEX(all_stat_sessions_mutex); > /* The root directory for all stat files */ > static struct dentry *stat_dir; > > -/* > - * Iterate through the rbtree using a post order traversal path > - * to release the next node. > - * It won't necessary release one at each iteration > - * but it will at least advance closer to the next one > - * to be released. > - */ > -static struct rb_node *release_next(struct rb_node *node) > +static void reset_stat_session(struct stat_session *session) > { > struct stat_node *snode; > - struct rb_node *parent = rb_parent(node); > - > - if (node->rb_left) > - return node->rb_left; > - else if (node->rb_right) > - return node->rb_right; > - else { > - if (!parent) > - return NULL; > - if (parent->rb_left == node) > - parent->rb_left = NULL; > - else > - parent->rb_right = NULL; > + struct rb_root *sroot = &session->stat_root; > > - snode = container_of(node, struct stat_node, node); > + while (!RB_EMPTY_ROOT(sroot)) { > + snode = rb_entry(sroot->rb_node, struct stat_node, node); > + rb_erase(&snode->node, sroot);
Why not just keep the previous version but sligthly modified:
while (node) node = release_next(node);
if (!RB_EMPTY_ROOT(root)) { node = rb_entry(...) kfree(....) root = RB_ROOT }
Because the problem with rb_erase() is the wasteful color based rebalancing performed whereas here we just need to walk into the tree and free the nodes.
Hm?
Frederic.
> kfree(snode); > - > - return parent; > } > } > > -static void reset_stat_session(struct stat_session *session) > -{ > - struct rb_node *node = session->stat_root.rb_node; > - > - while (node) > - node = release_next(node); > - > - session->stat_root = RB_ROOT; > -} > - > static void destroy_session(struct stat_session *session) > { > debugfs_remove(session->file); > -- > 1.5.4.rc3 >
| |