Re: [PATCH 2.6.20-rc1 00/10] Kernel memory leak detector 0.13
From: Ingo Molnar
Date: Thu Dec 28 2006 - 04:47:16 EST
* Catalin Marinas <catalin.marinas@xxxxxxxxx> wrote:
> >memleak info is at a negative offset from the allocated pointer. I.e.
> >that if kmalloc() returns 'ptr', the memleak info could be at
> >ptr-sizeof(memleak_info). That way you dont have to know the size of the
> >object beforehand and there's absolutely no need for a global hash of
> >any sort.
> It would probably need to be just a pointer embedded in the allocated
> block. With the current design, the memleak objects have a lifetime
> longer than the tracked block. This is mainly to avoid long locking
> during memory scanning and reporting.
this thing has to be /fast/ in the common path. I dont really see the
point in spreading out allocations like that for small objects. Access
to the object has to be refcounted /anyway/ due to scanning. Just move
that refcounting to the object freeing stage. Keep freed-but-used
buffers in some sort of per-CPU list that the scanning code flushes
after it's done. (Also maybe hold the cache_chain_mutex to prevent slab
caches from being destroyed during scanning.)
> > (it gets a bit more complex for page aligned allocations for the
> > buddy and for vmalloc - but that could be solved by adding one extra
> > pointer into struct page. [...]
> This still leaves the issue of marking objects as not being leaks or
> being of a different type. This is done by calling memleak_* functions
> at the allocation point (outside allocator) where only the pointer is
> known. [...]
i dont see the problem. By having the pointer we have access to the
memleak descriptor too.
> [...] In the vmalloc case, it would need to call find_vm_area. This
> might not be a big problem, only that memory resources are no longer
> treated in a unified way by kmemleak (and might not be trivial to add
> support for new allocators).
the pretty horrible locking dependencies in the current one are just as
bad. (which could be softened by a simplified allocator - but that
brings in other problems, which problems can only be solved via
allocator complexity ...)
If 'unification' means global locking and bad overhead and leak
descriptor maintainance complexity then yes, we very much dont want to
treat them in a unified way. Unless there's some killer counter-argument
against embedding the memleak descriptor in the object that we allocate
it is pretty much a must.
btw., you made the argument before that what matters most is the SLAB
allocator. (when i argued that we might want to extend this to other
allocators as well) You cant have it both ways :)
> > [...] That is a far more preferable cost than the locking/cache
> > overhead of a global hash.)
> A global hash would need to be re-built for every scan (and destroyed
> afterwards), making this operation longer since the pointer values
> together with their aliases (resulted from using container_of) are
> added to the hash.
by 'global hash' i mean the current code.
> I understand the benefits but I personally favor simplicity over
> performance, [...]
i think you are trying to clinge to a bad design detail. You spent time
on it, and that time was still worthwile, believe me. I often change
design details dozens of times before a patch hits lkml. It doesnt
matter, really - it's the path you walk that matters. This thing /has/
to be fast/scalable to be used in distributions.
> [...] Global structures are indeed a scalability problem but for a
> reasonable number of CPUs their overhead might not be that big.
lets forget you ever said this, ok? ;-)
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/