12:57 p.m.
Hello!
On Sep 18, 2014, at 7:43 PM, Dan Carpenter wrote:
On Thu, Sep 18, 2014 at 10:24:02PM +0200, Julia Lawall wrote:
> From: Julia Lawall <Julia.Lawall(a)lip6.fr>
>
> This patch removes some kzalloc-related macros and rewrites the
> associated null tests to use !x rather than x == NULL.
This is sort of exactly what Oleg asked us not to do in his previous
email. ;P
Hey, Thanks for remembering me ;)
I think there might be ways to get good enough tracing using
standard
kernel features but it's legitimately tricky to update everything to
using mempools or whatever. Maybe we should give Oleg some breathing
room to do this.
In fact I was mostly mourning ENTRY/EXIT/GOTO stuff back then - I don't know how to
replace anything like that even one bit at the scale we need.
the OBD_ALLOC()/FREE() served multiple purposes most of which could be done with other
ways now:
1. general accounting for lustre memory usage (all types directly allocated through the
macros), with a message present at the module unload if we freed less than allocated - a
warning is printed on unload which would set off a search for the leak (hey, at least we
know there is a leak somewhat fast, we also know how much was leaked, and we might
probably find out how many allocations were not freed if we wanted to add that stats). - I
don't know how to replace that, so perhaps a macro for this still be useful.
2. Hunting memory leaks (this is what the variable allocated, where it was allocated,
where it was freed, and the address of the allocation printed) - on non-production systems
this could be replaced with kernel memory leak detector now - in fact it's even more
convenient since you don't need to match up logs with a script to see what allocated
what, and there's even a convenient backtrace printed as a bonus. I used it and really
liked the result.
This is not really fitting in production, as kmemleak tends to eat memory like there's
no tomorrow (at least in my config) and also might need a kernel rebuild. But it's not
like getting people
to gather proper debug logs was easy too. So we can probably do away with that.
3. Fault injections - there's now a way to do this in the kernel, so we probably can
do away with this too.
4. Sometimes we need large allocations. general kmalloc is less reliable as system lives
on and memory fragmentation worsens. So we have this "allocations over 2-4 pages get
switched to vmalloc" logic,
if there's a way to do that automatically - that would be great.
I hate looking at the OBD_ALLOC* macros, but really it's not as
if we
don't have allocation helper functions anywhere in the rest of the
kernel. It's just that the style of the lustre helpers is so very very
ugly. It took me a while to spot that OBD_ALLOC() zeroes memory, for
example.
It should be relatively easy to re-write the macros so we can change
formats like this:
old: OBD_ALLOC(ptr, size);
new: ptr = obd_zalloc(size, GFP_NOFS);
old: OBD_ALLOC_WAIT(ptr, size);
new: ptr = obd_zalloc(size, GFP_KERNEL);
old: OBD_ALLOC_PTR(ptr);
new: ptr = obd_zalloc(sizeof(*ptr), GFP_NOFS);
etc...
Writing it this way means that we can't put the name of the pointer
we're allocating in the debug output but we could use the file and line
number instead or something. Oleg, what do you think?
I think we don't really need the allocated pointer and the name all that much now with
kmemleak.
But we still need to remember the allocation amount like we do now (and when freeing them
later).
This is where OBD_ALLOC_PTR/OBD_FREE_PTR come handy - the size is derived from structure
size
automatically - less space for error or unintentional mismatch (since kfree does not
really care
about number of bytes freed).
so if you prefer to just have everything lowercased, we probably can do obd_zalloc and
obd_zallc_ptr still?
(of course in some other world, there might have been a "context-aware" general
alloc/free functions
that would have known if an allocation came from a module context and did the tallying
internally,
and then warned on module unload if something did not match. But I imagine such module
context determination
would not be easy to do. Perhaps registered callbacks for pools that could be called on
alloc and on free - though such pools would also need to allow to allocate different sized
chunks too).
If we decide to mass convert to standard functions later then
it's dead
simple to do that with sed.
It's more ugly with OBD_FREE*, though, where the size is needed, while kfree/vfee does
not take size.
Also if you convert allocs while not converting frees, that makes code even more ugly (see
the current patch at hand even for the example of that).
The __OBD_MALLOC_VERBOSE() is hard to read. It has side effect bugs
if
you try to call OBD_ALLOC(ptr++, size); The kernel already has a way to
inject kmalloc() failures for a specific module so that bit can be
removed. Read Documentation/fault-injection/fault-injection.txt
Yes, I think I agree here.
Bye,
Oleg