lkml.org 
[lkml]   [2009]   [May]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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
>



\
 
 \ /
  Last update: 2009-05-25 18:03    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site