I wonder why we don't have type safe object allocators a-la new() in
C++ or g_new() in glib?
fooptr = k_new(struct foo, GFP_KERNEL);
is nicer and more descriptive than
fooptr = kmalloc(sizeof(*fooptr), GFP_KERNEL);
and more safe than
fooptr = kmalloc(sizeof(struct foo), GFP_KERNEL);
And we have zillions of both variants.
Note, I'm not advocating mass replacement, but using this in new code,
and gradually converting old ones whenever they need touching anyway.
Signed-off-by: Miklos Szeredi <[email protected]>
---
Index: linux-2.6.22/include/linux/slab.h
===================================================================
--- linux-2.6.22.orig/include/linux/slab.h 2007-07-09 01:32:17.000000000 +0200
+++ linux-2.6.22/include/linux/slab.h 2007-08-01 10:42:45.000000000 +0200
@@ -110,6 +110,38 @@ static inline void *kcalloc(size_t n, si
return __kzalloc(n * size, flags);
}
+/**
+ * k_new - allocate given type object
+ * @type: the type of the object to allocate
+ * @flags: the type of memory to allocate.
+ */
+#define k_new(type, flags) ((type *) kmalloc(sizeof(type), flags))
+
+/**
+ * k_new0 - allocate given type object, zero out allocated space
+ * @type: the type of the object to allocate
+ * @flags: the type of memory to allocate.
+ */
+#define k_new0(type, flags) ((type *) kzalloc(sizeof(type), flags))
+
+/**
+ * k_new_array - allocate array of given type object
+ * @type: the type of the object to allocate
+ * @len: the length of the array
+ * @flags: the type of memory to allocate.
+ */
+#define k_new_array(type, len, flags) \
+ ((type *) kmalloc(sizeof(type) * (len), flags))
+
+/**
+ * k_new0_array - allocate array of given type object, zero out allocated space
+ * @type: the type of the object to allocate
+ * @len: the length of the array
+ * @flags: the type of memory to allocate.
+ */
+#define k_new0_array(type, len, flags) \
+ ((type *) kzalloc(sizeof(type) * (len), flags))
+
/*
* Allocator specific definitions. These are mainly used to establish optimized
* ways to convert kmalloc() calls to kmem_cache_alloc() invocations by selecting
On Wed, Aug 01, 2007 at 11:06:46AM +0200, Miklos Szeredi wrote:
> I wonder why we don't have type safe object allocators a-la new() in
> C++ or g_new() in glib?
>
> fooptr = k_new(struct foo, GFP_KERNEL);
>
> is nicer and more descriptive than
>
> fooptr = kmalloc(sizeof(*fooptr), GFP_KERNEL);
>...
But it's much more likely to break when someone converts fooptr to a
different struct.
It might not be a common case but it sometimes happens - and your type
safe variant introduces the possibility for really nasty bugs.
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
> > I wonder why we don't have type safe object allocators a-la new() in
> > C++ or g_new() in glib?
> >
> > fooptr = k_new(struct foo, GFP_KERNEL);
> >
> > is nicer and more descriptive than
> >
> > fooptr = kmalloc(sizeof(*fooptr), GFP_KERNEL);
> >...
>
> But it's much more likely to break when someone converts fooptr to a
> different struct.
>
> It might not be a common case but it sometimes happens - and your type
> safe variant introduces the possibility for really nasty bugs.
The compiler would emit a warning about assigning to a pointer of
different type. That's a fairly strong hint that something just
broke.
Miklos
Miklos Szeredi <[email protected]> writes:
> I wonder why we don't have type safe object allocators a-la new() in
> C++ or g_new() in glib?
>
> fooptr = k_new(struct foo, GFP_KERNEL);
>
> is nicer and more descriptive than
>
> fooptr = kmalloc(sizeof(*fooptr), GFP_KERNEL);
>
> and more safe than
>
> fooptr = kmalloc(sizeof(struct foo), GFP_KERNEL);
How is it more safe? It seems 100% equivalent to me,
just a different syntax.
>
> And we have zillions of both variants.
In my own non kernel code i tend to define a pascal style NEW()
#define NEW(p) ((p) = malloc(sizeof(*(p))))
But I'm not sure such a untraditional solution would too popular.
Also I don't think we have too many bugs in this area anyways; so
it might be better to concentrate on more fruitful areas.
-Andi
> > I wonder why we don't have type safe object allocators a-la new() in
> > C++ or g_new() in glib?
> >
> > fooptr = k_new(struct foo, GFP_KERNEL);
> >
> > is nicer and more descriptive than
> >
> > fooptr = kmalloc(sizeof(*fooptr), GFP_KERNEL);
> >
> > and more safe than
> >
> > fooptr = kmalloc(sizeof(struct foo), GFP_KERNEL);
>
> How is it more safe? It seems 100% equivalent to me,
> just a different syntax.
Note the (type *) cast:
#define k_new(type, flags) ((type *) kmalloc(sizeof(type), flags))
Miklos
Miklos Szeredi <[email protected]> writes:
>
> #define k_new(type, flags) ((type *) kmalloc(sizeof(type), flags))
The cast doesn't make it more safe in any way
(at least as long as you don't care about portability to C++;
the kernel doesn't)
-Andi
> >
> > #define k_new(type, flags) ((type *) kmalloc(sizeof(type), flags))
>
> The cast doesn't make it more safe in any way
I does, since a warning will be issued, if the type of the assigned
pointer doesn't match the requested allocation.
And yes, warnings are _very_ useful in C for enforcing type safety.
Miklos
On Aug 1 2007 12:45, Miklos Szeredi wrote:
>> >
>> > #define k_new(type, flags) ((type *) kmalloc(sizeof(type), flags))
>>
>> The cast doesn't make it more safe in any way
>
>I does, since a warning will be issued, if the type of the assigned
>pointer doesn't match the requested allocation.
>
>And yes, warnings are _very_ useful in C for enforcing type safety.
void *p;
p = (struct foo *)kmalloc(sizeof(struct foo), GFP_KERNEL);
Jan
--
> >> >
> >> > #define k_new(type, flags) ((type *) kmalloc(sizeof(type), flags))
> >>
> >> The cast doesn't make it more safe in any way
> >
> >I does, since a warning will be issued, if the type of the assigned
> >pointer doesn't match the requested allocation.
> >
> >And yes, warnings are _very_ useful in C for enforcing type safety.
>
> void *p;
> p = (struct foo *)kmalloc(sizeof(struct foo), GFP_KERNEL);
Using void pointers is _obviously_ not type safe. What has that got
to do with k_new()?
Miklos
On Wed, 1 Aug 2007, Miklos Szeredi wrote:
>
> I wonder why we don't have type safe object allocators a-la new() in
> C++ or g_new() in glib?
>
> fooptr = k_new(struct foo, GFP_KERNEL);
I would object to this if only because of the horrible name.
C++ is not a good language to take ideas from, and "new()" was not it's
best feature to begin with. "k_new()" is just disgusting.
I'd call it something like "alloc_struct()" instead, which tells you
exactly what it's all about. Especially since we try to avoid typedefs in
the kernel, and as a result, it's basically almost always a struct thing.
That said, I'm not at all sure it's worth it. Especially not with all the
various variations on a theme (zeroed, arrays, etc etc).
Quite frankly, I suspect you would be better off just instrumenting
"sparse" instead, and matching up the size of the allocation with the type
it gets assigned to.
Linus
On Wed, 1 Aug 2007, Miklos Szeredi wrote:
> I wonder why we don't have type safe object allocators a-la new() in
> C++ or g_new() in glib?
>
> fooptr = k_new(struct foo, GFP_KERNEL);
>
> is nicer and more descriptive than
>
> fooptr = kmalloc(sizeof(*fooptr), GFP_KERNEL);
>
> and more safe than
>
> fooptr = kmalloc(sizeof(struct foo), GFP_KERNEL);
>
> And we have zillions of both variants.
Hmmm yes I think that would be good. However, please clean up the naming.
The variant on zeroing on zering get to be too much.
> + * k_new - allocate given type object
> + * @type: the type of the object to allocate
> + * @flags: the type of memory to allocate.
> + */
> +#define k_new(type, flags) ((type *) kmalloc(sizeof(type), flags))
kalloc?
> +
> + * k_new0 - allocate given type object, zero out allocated space
> + * @type: the type of the object to allocate
> + * @flags: the type of memory to allocate.
> + */
> +#define k_new0(type, flags) ((type *) kzalloc(sizeof(type), flags))
A new notation for zeroing! This is equivalent to
kalloc(type, flags | __GFP_ZERO)
maybe define new GFP_xxx instead?
> +/**
> + * k_new_array - allocate array of given type object
> + * @type: the type of the object to allocate
> + * @len: the length of the array
> + * @flags: the type of memory to allocate.
> + */
> +#define k_new_array(type, len, flags) \
> + ((type *) kmalloc(sizeof(type) * (len), flags))
We already have array initializations using kcalloc.
> +#define k_new0_array(type, len, flags) \
> + ((type *) kzalloc(sizeof(type) * (len), flags))
Same as before.
I do not see any _node variants?
How about the following minimal set
kmalloc(size, flags) kalloc(struct, flags)
kmalloc_node(size, flags, node) kalloc_node(struct, flags, node)
The array variants translate into kmalloc anyways and are used
in an inconsistent manner. Sometime this way sometimes the other. Leave
them?
kcalloc(n, size, flags) == kmalloc(size, flags)
Then kzalloc is equivalent to adding the __GFP_ZERO flag. Thus
kzalloc(size, flags) == kmalloc(size, flags | __GFPZERO)
If you define a new flag like GFP_ZERO_ATOMIC and GFP_ZERO_KERNEL you
could do
kalloc(struct, GFP_ZERO_KERNEL)
instead of adding new variants?
Hi Miklos,
On Wed, 1 Aug 2007, Miklos Szeredi wrote:
> I wonder why we don't have type safe object allocators a-la new() in
> C++ or g_new() in glib?
>
> fooptr = k_new(struct foo, GFP_KERNEL);
>
> is nicer and more descriptive than
>
> fooptr = kmalloc(sizeof(*fooptr), GFP_KERNEL);
>
> and more safe than
>
> fooptr = kmalloc(sizeof(struct foo), GFP_KERNEL);
>
> And we have zillions of both variants.
>
> Note, I'm not advocating mass replacement, but using this in new code,
> and gradually converting old ones whenever they need touching anyway.
> [...]
>
> +/**
> + * k_new - allocate given type object
> + * @type: the type of the object to allocate
> + * @flags: the type of memory to allocate.
> + */
> +#define k_new(type, flags) ((type *) kmalloc(sizeof(type), flags))
What others already said, plus:
kmalloc()'ing sizeof(struct foo) is not always what we want in C either.
Several kernel structs have zero-length / variable-length array members
and space must be allocated for them only at alloc() time ... would be
impossible to make them work with this scheme.
Satyam
> >
> > I wonder why we don't have type safe object allocators a-la new() in
> > C++ or g_new() in glib?
> >
> > fooptr = k_new(struct foo, GFP_KERNEL);
>
> I would object to this if only because of the horrible name.
>
> C++ is not a good language to take ideas from, and "new()" was not it's
> best feature to begin with. "k_new()" is just disgusting.
>
> I'd call it something like "alloc_struct()" instead, which tells you
> exactly what it's all about. Especially since we try to avoid typedefs in
> the kernel, and as a result, it's basically almost always a struct thing.
Yeah, I'm not strongly attached to the "new" name, although I got used
to it in glib. The glib API is broken in lots of ways, but g_new()
and friends are nice and useful.
> That said, I'm not at all sure it's worth it. Especially not with all the
> various variations on a theme (zeroed, arrays, etc etc).
The number of variations can be reduced to just zeroing/nonzeroing, by
making the array length mandatory. That's what glib does in g_new().
> Quite frankly, I suspect you would be better off just instrumenting
> "sparse" instead, and matching up the size of the allocation with the type
> it gets assigned to.
But that just can't be done, because kmalloc() doesn't tell us the
_intent_ of the allocation. The allocation could be for an array, or
for a struct with a variable length string at the end, or it could be
multiple structs concatenated. We have all sorts of weird stuff in
there that sparse would not be able to handle.
That's why alloc_struct() is better: it describes the intention exacly
"allocate the given object and return an appropriately typed pointer".
While kmalloc() just says "allocate a given sized memory chunk and
return an untyped pointer".
Miklos
> > I wonder why we don't have type safe object allocators a-la new() in
> > C++ or g_new() in glib?
> >
> > fooptr = k_new(struct foo, GFP_KERNEL);
> >
> > is nicer and more descriptive than
> >
> > fooptr = kmalloc(sizeof(*fooptr), GFP_KERNEL);
> >
> > and more safe than
> >
> > fooptr = kmalloc(sizeof(struct foo), GFP_KERNEL);
> >
> > And we have zillions of both variants.
>
> Hmmm yes I think that would be good. However, please clean up the naming.
> The variant on zeroing on zering get to be too much.
OK, there seems to be a consensus on that ;)
[snip]
> I do not see any _node variants?
Well, those are _very_ rare, I'd only add those if there's a demand
for them.
> The array variants translate into kmalloc anyways and are used
> in an inconsistent manner. Sometime this way sometimes the other. Leave
> them?
If the too many variants are bothersome, then I'd rather just have the
array variant, and give 1 as an array size for the non-array case.
> kcalloc(n, size, flags) == kmalloc(size, flags)
>
> Then kzalloc is equivalent to adding the __GFP_ZERO flag. Thus
>
> kzalloc(size, flags) == kmalloc(size, flags | __GFPZERO)
>
> If you define a new flag like GFP_ZERO_ATOMIC and GFP_ZERO_KERNEL you
> could do
>
> kalloc(struct, GFP_ZERO_KERNEL)
>
> instead of adding new variants?
I don't really like this, introducing new gfp flags just makes
grepping harder.
I do think that at least having a zeroing and a non-zeroing variant
makes sense.
Miklos
> >
> > +/**
> > + * k_new - allocate given type object
> > + * @type: the type of the object to allocate
> > + * @flags: the type of memory to allocate.
> > + */
> > +#define k_new(type, flags) ((type *) kmalloc(sizeof(type), flags))
>
> What others already said, plus:
>
> kmalloc()'ing sizeof(struct foo) is not always what we want in C either.
>
> Several kernel structs have zero-length / variable-length array members
> and space must be allocated for them only at alloc() time ... would be
> impossible to make them work with this scheme.
Exactly. We can, and should use kmalloc() for that.
Miklos
On Thu, Aug 02, 2007 at 09:27:57AM +0200, Miklos Szeredi wrote:
> > Quite frankly, I suspect you would be better off just instrumenting
> > "sparse" instead, and matching up the size of the allocation with the type
> > it gets assigned to.
>
> But that just can't be done, because kmalloc() doesn't tell us the
> _intent_ of the allocation.
Of course it can be done. When argument has form sizeof(...), attach
the information about target of pointer to the resulting void *; have
? : between pointer to object and that one warn if types do not match,
? : between void * and that one lose that information, ? : between
two such warn when types do not match. On assigment-type operations
(assignment, passing argument to function, initializer, return) when
the type of target is pointer to object (and not void *) warn if
types do not match. Have typeof lose that information.
Not even hard to implement; just let us finish cleaning the lazy examination
logics up first (~5-6 patches away).
FWIW, I object against the original proposal, no matter how you name it.
Reason: we are introducing more magical constructs that have to be known
to human reader in order to parse the damn code.
Folks, this is serious. _We_ might be used to having in effect a C dialect
with extensions implemented by preprocessor. That's fine, but for a fresh
reader it becomes a problem; sure, they can dig in include/linux/*.h and
to some extent they clearly have to. However, it doesn't come for free
and we really ought to keep that in mind - amount of local idioms (and
anything that doesn't look like a normal function call with normal arguments
_does_ become an idiom to be learnt before one can fluently RTFS) is a thing
to watch out for.
IOW, whenever we add to that pile we ought to look hard at whether it's worth
the trouble.
> Folks, this is serious. _We_ might be used to having in effect a C dialect
> with extensions implemented by preprocessor. That's fine, but for a fresh
> reader it becomes a problem; sure, they can dig in include/linux/*.h and
> to some extent they clearly have to. However, it doesn't come for free
> and we really ought to keep that in mind - amount of local idioms (and
> anything that doesn't look like a normal function call with normal arguments
> _does_ become an idiom to be learnt before one can fluently RTFS) is a thing
> to watch out for.
That's why the g_new() form that glib uses makes some sense. It
borrows an idiom from C++, and although we all know C++ is a horrid
language, to some extent lots of people are familiar with it.
> IOW, whenever we add to that pile we ought to look hard at whether it's worth
> the trouble.
Well, this is not some earth-shattering stuff, but I think it would be
good to have. I got used to it in glib, and I miss it in linux.
I understand the knee-jerk reaction of most people who are unfamiliar
with it, and I can do nothing about that. If there's no positive
feedback I'll just give up, it's not that I can't live with the
current situation.
Miklos
On Thu, 2 Aug 2007, Miklos Szeredi wrote:
>
> The number of variations can be reduced to just zeroing/nonzeroing, by
> making the array length mandatory. That's what glib does in g_new().
Quite frankly, you don't need the zeroing. That's what __GFP_ZERO does in
the flags.
That said, I'm not at all sure that it's at all more readable to add some
new abstraction layer and do
struct random_struct *ptr;
ptr = alloc_struct(random_struct, 1, GFP_KERNEL | __GFP_ZERO);
than just doing a
ptr = kmalloc(sizeof(*ptr), GFP_KERNEL | __GFP_ZERO);
or
ptr = kzalloc(sizeof(*ptr), GFP_KERNEL);
(and adding the zeroing variant of alloc_struct() just adds *more*
confusing issues).
The fact is, type safety in this area is probably less important than the
code just being readable. And have fifteen different interfaces to memory
allocation just isn't ever going to readable - regardless of how good they
are individually.
Linus
On Thu, 2 Aug 2007, Miklos Szeredi wrote:
> > If you define a new flag like GFP_ZERO_ATOMIC and GFP_ZERO_KERNEL you
> > could do
> >
> > kalloc(struct, GFP_ZERO_KERNEL)
> >
> > instead of adding new variants?
>
> I don't really like this, introducing new gfp flags just makes
> grepping harder.
The __GFP_ZERO flag has been around for a long time. GFP_ZERO_ATOMIC and
GFP_ZERO_KERNEL or so could just be a shorthand notation.
Maybe
#define GFP_ZATOMIC (GFP_ATOMIC | __GFP_ZERO)
#define GFP_ZKERNEL (GFP_KERNEL | __GFP_ZERO)
?
> I do think that at least having a zeroing and a non-zeroing variant
> makes sense.
They require a duplication of the API and have led to inconsistencies
because the complete API was not available with zeroing capabilities
(there is still no kzalloc_node f.e.).
Using a gfp flag allows all allocation functions to optionally zero data
without having to define multiple functions.
The definition of new variants is a bit complicated since the allocator
functions contain lots of smarts to do inline constant folding. This is
necessary to determine the correct slab at compile time. I'd rather have
as few of those as possible.