On Fri, 19 Sep 2014, Drokin, Oleg wrote: > Hello! > > First, thanks for your patches and efforts spent on these cleanups. > > On Sep 19, 2014, at 12:45 AM, Julia Lawall wrote: > > > With respect to the upper case lower case issue, does the thing need to be > > a macro? I think that the lowercase is more or less fine, but only if > > what is behind it is a function. > > I don't have a strong opinion either way as long as we have all the functionality > we need. > > > I say more or less fine, because normally in the kernel the special > > allocators have special purposes, eg allocating and initializing the xyz > > structure. Here what is wanted is a general purpose allocator with lots > > of special tracing features, so it is not quite the same thing. And one > > can wonder why all of these special tracing features are not relevant to > > the kernel as a whole? > > Like I explained in my previous email, many of the tracing features are already > possible to replace with other existing in-kernel mechanisms like kmemleak. > > Except the total tally of allocations/frees so that a memleak could be visibly > easily seen on module unload time. I think this would be useful for other > kinds of modules too, not just lustre, so having that as a generic allocator > feature would be cool too. > > > In reading through the description of the needed features, it seems like > > only the _ptr extension requires being a macro. Do we need that? The > > We only need that as a less error-prone way of having > x = obd_kzalloc(sizeof(*x), ….) > … > obd_free(…, sizeof(*x)) > > Real free function does not take size argument, but we need that for > total allocated/freed accounting. Easy to have disconnect with > the size argument of obd_free to be wrong. > > > rest of the kernel manages to do x = kzalloc(sizeof(*x),...) ok. It's > > unpleasant to have an assignment hidden in this way. And currently it is > > not used consistently. There are some OBD_ALLOCs that have the same form. > > Yes, those are converted as thy are noticed. > > > Sorry for overlooking the frees. I was focusing on trying one thing at a > > time... > > I kind of think it's a related issue. > Touching ones needs to touch the other if not in the same patch then in > a next patch. And that's why I think consideations for what FREEs would need > is needed from the start, so the FREEs removal patch does not goes and patches a bunch of just patched allocs. Sure, it's fine. Where do you want to go from here? Should I propose function definitions for obd_alloc and obd_free? Could we leave the vmalloc issue for a future set of changes? julia
