[BUG][2.6.13.4] Memoryleak - idr_layer_cache slab - inotify?

[BUG][2.6.13.4] Memoryleak - idr_layer_cache slab - inotify?

[Stefan Jones]

Hi all,

I noticed this a while back when gam_server (new fam replacement)
started playing up and the idr_layer_cache slab used up 300Mb of RAM.

To reproduce:

Run gnome and make sure it is using gam_server for fam.

In one console do:

while true ;do sleep 0.1 ; killall -w gam_server; done

In another try slabtop and see the idr_layer_cache slab climb, eating
memory (abit slowly).
( Gnome restarts gam_server after each kill for you and gets it doing
stuff )

I looked though the source and inotify does use the idr_layer_cache slab
and gam_server also uses inotify.

Thank for your time, shall I use bugzilla.kernel or is this ok? More
info needed?

Stefan

Re: [BUG][2.6.13.4] Memoryleak - idr_layer_cache slab - inotify?

[Stefan Jones]

Stefan Jones wrote:

> I noticed this a while back when gam_server (new fam replacement)
> started playing up and the idr_layer_cache slab used up 300Mb of RAM.
>
> To reproduce:
>
> Run gnome and make sure it is using gam_server for fam.
>
> In one console do:
>
> while true ;do sleep 0.1 ; killall -w gam_server; done
>
> In another try slabtop and see the idr_layer_cache slab climb, eating
> memory (abit slowly).
> ( Gnome restarts gam_server after each kill for you and gets it doing
> stuff )
>
> I looked though the source and inotify does use the idr_layer_cache slab
> and gam_server also uses inotify.

Made a standalone testcase, run this and the kernel will eat up your 
memory (seen via slabtop):

( this should compile everywhere )

#include <sys/wait.h>
#include <signal.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#include <unistd.h>
#include <sys/select.h>
#include <asm/unistd.h>
#include <errno.h>


#define __NR_inotify_init    291
#define __NR_inotify_add_watch    292
#define __NR_inotify_rm_watch    293


_syscall0(int, inotify_init);
_syscall3(int, inotify_add_watch, int, fd, const char *, name, unsigned 
int, mask);
_syscall2(int, inotify_rm_watch, int, fd, unsigned int, wd);

int main() {
    pid_t pid;
    int fd;
    int wd;
    const char *dirname=".";
    unsigned int mask = 0xffffffff;
    int stat;
   
    for(;;) {
        if(!(pid = fork())) {
            fd = inotify_init ();

            wd = inotify_add_watch (fd, dirname, mask);
            if (wd < 0)
                perror ("inotify_add_watch");

            kill(getpid(),SIGTERM);
        }
        waitpid(pid,&stat,0);
    }
   
    return 0;
}

Re: [BUG][2.6.13.4] Memoryleak - idr_layer_cache slab - inotify?

[Stefan Jones]

Stefan Jones wrote:

Made a standalone testcase, run this and the kernel will eat up your
memory (seen via slabtop):

[ creates a inotify_dev, and a watch and exits ; repeat via fork ... ]

Tracked it down me thinks:

struct inotify_device {
...
	struct idr		idr;		/* idr mapping wd -> watch */
...
}

idr gets allocated each time inotify_init() is called:

asmlinkage long sys_inotify_init(void)
{
..
idr_init(&dev->idr);
..
}

Looking in lib/idr.c you see:

  * You can release ids at any time. When all ids are released, most of
  * the memory is returned (we keep IDR_FREE_MAX) in a local pool so we
  * don't need to go to the memory "store" during an id allocate, just
  * so you don't need to be too concerned about locking and conflicts
  * with the slab allocator.

So even if you free all ids which create_watch->inotify_dev_get_wd 
creates you will still have menory in your struct idr.

So when
static inline void put_inotify_dev(struct inotify_device *dev)
{
	if (atomic_dec_and_test(&dev->count)) {
		atomic_dec(&dev->user->inotify_devs);
		free_uid(dev->user);
		kfree(dev);
	}
}

is called I think this is whre the memory gets lost. ( linux/idr.h has 
not free function I see )

Stefan

Re: [BUG][2.6.13.4] Memoryleak - idr_layer_cache slab - inotify?

[Andrew Morton]

Stefan Jones <stefan.jones@churchillrandoms.co.uk> wrote:
>
> Stefan Jones wrote:
> 
> Made a standalone testcase, run this and the kernel will eat up your
> memory (seen via slabtop):
> 
> [ creates a inotify_dev, and a watch and exits ; repeat via fork ... ]
> 
> Tracked it down me thinks:
> 
> struct inotify_device {
> ...
> 	struct idr		idr;		/* idr mapping wd -> watch */
> ...
> }
> 
> idr gets allocated each time inotify_init() is called:
> 
> asmlinkage long sys_inotify_init(void)
> {
> ..
> idr_init(&dev->idr);
> ..
> }
> 
> Looking in lib/idr.c you see:
> 
>   * You can release ids at any time. When all ids are released, most of
>   * the memory is returned (we keep IDR_FREE_MAX) in a local pool so we
>   * don't need to go to the memory "store" during an id allocate, just
>   * so you don't need to be too concerned about locking and conflicts
>   * with the slab allocator.
> 
> So even if you free all ids which create_watch->inotify_dev_get_wd 
> creates you will still have menory in your struct idr.
> 
> So when
> static inline void put_inotify_dev(struct inotify_device *dev)
> {
> 	if (atomic_dec_and_test(&dev->count)) {
> 		atomic_dec(&dev->user->inotify_devs);
> 		free_uid(dev->user);
> 		kfree(dev);
> 	}
> }
> 
> is called I think this is whre the memory gets lost. ( linux/idr.h has 
> not free function I see )
> 

That makes sense, thanks.

Something like this?


diff -puN lib/idr.c~inotify-idr-leak-fix lib/idr.c
--- 25/lib/idr.c~inotify-idr-leak-fix	Fri Oct 21 13:44:23 2005
+++ 25-akpm/lib/idr.c	Fri Oct 21 13:46:09 2005
@@ -346,6 +346,19 @@ void idr_remove(struct idr *idp, int id)
 EXPORT_SYMBOL(idr_remove);
 
 /**
+ * idr_destroy - release all cached layers within an idr tree
+ * idp: idr handle
+ */
+void idr_destroy(struct idr *idp)
+{
+	while (idp->id_free_cnt) {
+		struct idr_layer *p = alloc_layer(idp);
+		kmem_cache_free(idr_layer_cache, p);
+	}
+}
+EXPORT_SYMBOL(idr_destroy);
+
+/**
  * idr_find - return pointer for given id
  * @idp: idr handle
  * @id: lookup key
diff -puN include/linux/idr.h~inotify-idr-leak-fix include/linux/idr.h
--- 25/include/linux/idr.h~inotify-idr-leak-fix	Fri Oct 21 13:44:23 2005
+++ 25-akpm/include/linux/idr.h	Fri Oct 21 13:46:19 2005
@@ -75,4 +75,5 @@ int idr_pre_get(struct idr *idp, unsigne
 int idr_get_new(struct idr *idp, void *ptr, int *id);
 int idr_get_new_above(struct idr *idp, void *ptr, int starting_id, int *id);
 void idr_remove(struct idr *idp, int id);
+void idr_destroy(struct idr *idp);
 void idr_init(struct idr *idp);
diff -puN fs/inotify.c~inotify-idr-leak-fix fs/inotify.c
--- 25/fs/inotify.c~inotify-idr-leak-fix	Fri Oct 21 13:47:27 2005
+++ 25-akpm/fs/inotify.c	Fri Oct 21 13:47:38 2005
@@ -176,6 +176,7 @@ static inline void put_inotify_dev(struc
 	if (atomic_dec_and_test(&dev->count)) {
 		atomic_dec(&dev->user->inotify_devs);
 		free_uid(dev->user);
+		idr_destroy(&dev->idr);
 		kfree(dev);
 	}
 }
_

Re: [BUG][2.6.13.4] Memoryleak - idr_layer_cache slab - inotify?

[Stefan Jones]

Andrew Morton wrote:

>Something like this?
>
>  
>
Yep, that fixes it. The  idr_layer_cache slab is now stable. Thanks

>diff -puN lib/idr.c~inotify-idr-leak-fix lib/idr.c
>--- 25/lib/idr.c~inotify-idr-leak-fix	Fri Oct 21 13:44:23 2005
>+++ 25-akpm/lib/idr.c	Fri Oct 21 13:46:09 2005
>@@ -346,6 +346,19 @@ void idr_remove(struct idr *idp, int id)
> EXPORT_SYMBOL(idr_remove);
> 
> /**
>+ * idr_destroy - release all cached layers within an idr tree
>+ * idp: idr handle
>+ */
>+void idr_destroy(struct idr *idp)
>+{
>+	while (idp->id_free_cnt) {
>+		struct idr_layer *p = alloc_layer(idp);
>+		kmem_cache_free(idr_layer_cache, p);
>+	}
>+}
>+EXPORT_SYMBOL(idr_destroy);
>+
>+/**
>  * idr_find - return pointer for given id
>  * @idp: idr handle
>  * @id: lookup key
>diff -puN include/linux/idr.h~inotify-idr-leak-fix include/linux/idr.h
>--- 25/include/linux/idr.h~inotify-idr-leak-fix	Fri Oct 21 13:44:23 2005
>+++ 25-akpm/include/linux/idr.h	Fri Oct 21 13:46:19 2005
>@@ -75,4 +75,5 @@ int idr_pre_get(struct idr *idp, unsigne
> int idr_get_new(struct idr *idp, void *ptr, int *id);
> int idr_get_new_above(struct idr *idp, void *ptr, int starting_id, int *id);
> void idr_remove(struct idr *idp, int id);
>+void idr_destroy(struct idr *idp);
> void idr_init(struct idr *idp);
>diff -puN fs/inotify.c~inotify-idr-leak-fix fs/inotify.c
>--- 25/fs/inotify.c~inotify-idr-leak-fix	Fri Oct 21 13:47:27 2005
>+++ 25-akpm/fs/inotify.c	Fri Oct 21 13:47:38 2005
>@@ -176,6 +176,7 @@ static inline void put_inotify_dev(struc
> 	if (atomic_dec_and_test(&dev->count)) {
> 		atomic_dec(&dev->user->inotify_devs);
> 		free_uid(dev->user);
>+		idr_destroy(&dev->idr);
> 		kfree(dev);
> 	}
> }
>_
>
>
>  
>