flexible array implementation v3

RFC PATCH In review Dave Hansen 4 participants 8 messages Jul 22, 2009 – Jul 23, 2009 Library code

Share & tools

Dave Hansen

author

2009-07-22 17:54 UTC #0 lore raw

RFC PATCH 1/1 flexible array implementation v3

Changes from v2:
- renamed some of the index functions
- added preallocation function
- added flex_array_free_parts() for use with
statically allocated bases
- killed append() function

Changes from v1:
- to vs too typo
- added __check_part_and_nr() and gave it a warning
- fixed off-by-one check on __nr_part_ptrs()
- added FLEX_ARRAY_INIT() macro
- some kerneldoc comments about the capacity with various sized objects
- comments to note lack of locking semantice

Once a structure goes over PAGE_SIZE*2, we see occasional allocation failures. Some people have chosen to switch over to things like vmalloc() that will let them keep array-like access to such a large structures. But, vmalloc() has plenty of downsides.

Here's an alternative. I think it's what Andrew was suggesting here:

	http://lkml.org/lkml/2009/7/2/518

I call it a flexible array. It does all of its work in PAGE_SIZE bits, so never does an order>0 allocation. The base level has PAGE_SIZE-2*sizeof(int) bytes of storage for pointers to the second level. So, with a 32-bit arch, you get about 4MB (4183112 bytes) of total storage when the objects pack nicely into a page. It is half that on 64-bit because the pointers are twice the size. There's a table detailing this in the code.

There are kerneldocs for the functions, but here's an overview:

flex_array_alloc() - dynamically allocate a base structure flex_array_free() - free the array and all of the
second-level pages
flex_array_free_parts() - free the second-level pages, but
not the base (for static bases)
flex_array_put() - copy into the array at the given index flex_array_get() - copy out of the array at the given index flex_array_prealloc() - preallocate the second-level pages
between the given indexes to guarantee no allocs will occur at put() time.

We could also potentially just pass the "element_size" into each of the API functions instead of storing it internally. That would get us one more base pointer on 64-bit.

The last improvement that I thought about was letting the individual array members span pages. In this implementation, if you have a 2049-byte object, it will only pack one of them into each "part" with no attempt to pack them. At this point, I don't think the added complexity would be worth it.

Signed-off-by Dave Hansen <[email protected]>

---

 linux-2.6.git-dave/include/linux/flex_array.h |   45 ++++
 linux-2.6.git-dave/lib/Makefile               |    2 
 linux-2.6.git-dave/lib/flex_array.c           |  240 ++++++++++++++++++++++++++
 3 files changed, 286 insertions(+), 1 deletion(-)

include/linux/flex_array.h new file +45 -0

diff -puN /dev/null include/linux/flex_array.h
@@ -0,0 +1,45 @@
+#ifndef _FLEX_ARRAY_H
+#define _FLEX_ARRAY_H
+
+#include <linux/types.h>
+#include <asm/page.h>
+
+#define FLEX_ARRAY_PART_SIZE PAGE_SIZE
+#define FLEX_ARRAY_BASE_SIZE PAGE_SIZE
+
+struct flex_array_part;
+
+/*
+ * This is meant to replace cases where an array-like
+ * structure has gotten to big too fit into kmalloc()
+ * and the developer is getting tempted to use
+ * vmalloc().
+ */
+
+struct flex_array {
+	union {
+		struct {
+			int element_size;
+			struct flex_array_part *parts[0];
+		};
+		/*
+		 * This little trick makes sure that
+		 * sizeof(flex_array) == PAGE_SIZE
+		 */
+		char padding[FLEX_ARRAY_BASE_SIZE];
+	};
+};
+
+#define FLEX_ARRAY_INIT(size, total) { { {\
+	.element_size = (size),		\
+	.nr_elements = 0,		\
+} } }
+
+struct flex_array *flex_array_alloc(int element_size, int total, gfp_t flags);
+int flex_array_prealloc(struct flex_array *fa, int start, int end, gfp_t flags);
+void flex_array_free(struct flex_array *fa);
+void flex_array_free_parts(struct flex_array *fa);
+int flex_array_put(struct flex_array *fa, int element_nr, void *src, gfp_t flags);
+void *flex_array_get(struct flex_array *fa, int element_nr);
+
+#endif /* _FLEX_ARRAY_H */

lib/flex_array.c new file +240 -0

240 lines not on this page · raw message

lib/Makefile renamed from lib/Makefile~fa +1 -1

diff -puN lib/Makefile~fa lib/Makefile
@@ -12,7 +12,7 @@ lib-y := ctype.o string.o vsprintf.o cmd
 	 idr.o int_sqrt.o extable.o prio_tree.o \
 	 sha1.o irq_regs.o reciprocal_div.o argv_split.o \
 	 proportions.o prio_heap.o ratelimit.o show_mem.o \
-	 is_single_threaded.o plist.o decompress.o
+	 is_single_threaded.o plist.o decompress.o flex_array.o
 
 lib-$(CONFIG_MMU) += ioremap.o
 lib-$(CONFIG_SMP) += cpumask.o

KAMEZAWA Hiroyuki

2009-07-23 01:44 UTC #1 lore raw

Quoted 9 lines · On Wed, 22 Jul 2009 10:53:45 -0700 Dave Hansen…

Changes from v2:
- renamed some of the index functions
- added preallocation function
- added flex_array_free_parts() for use with
statically allocated bases
- killed append() function

Quoted 9 lines

Changes from v1:
- to vs too typo
- added __check_part_and_nr() and gave it a warning
- fixed off-by-one check on __nr_part_ptrs()
- added FLEX_ARRAY_INIT() macro
- some kerneldoc comments about the capacity with various sized objects
- comments to note lack of locking semantice

Seems nice, thank you. but a nitpick..

+struct flex_array *flex_array_alloc(int element_size, int total, gfp_t flags) +{
+ struct flex_array *ret;
+ int max_size = nr_base_part_ptrs() * __elements_per_part(element_size);
+
+ /* max_size will end up 0 if element_size > PAGE_SIZE */
+ if (total > max_size)
+ return NULL;

Can't we store "total" somewhere and do error check ?

And some magic value as following can't be defined for 'total' ?

#define FLEX_ARRAY_MAX_ELEMENTS (-1) // Use maximum flex array.

Quoted 8 lines

lib/flex_array.c new file +8 -0

+/**
+ * flex_array_get - pull data back out of the array
+ * @element_nr:	index of the element to fetch from the array
+ *
+ * Returns a pointer to the data at index @element_nr.  Note
+ * that this is a copy of the data that was passed in.  If you
+ * are using this to store pointers, you'll get back &ptr.
+ *

lib/flex_array.c line 233

Quoted 10 lines

lib/flex_array.c new file +10 -0

+ * Locking must be provided by the caller.
+ */
+void *flex_array_get(struct flex_array *fa, int element_nr)
+{
+	int part_nr = fa_element_to_part_nr(fa, element_nr);
+	struct flex_array_part *part;
+	int offset;
+
+	if (__check_part_nr(fa, part_nr))
+		return NULL;

return -EINVAL, here ?
(Can't we compared with stored "total" ?)

lib/flex_array.c line 236

Quoted 3 lines

lib/flex_array.c new file +3 -0
+	if (!fa->parts[part_nr])
+		return NULL;
+

The caller can't know whether
- there are no entry or
- NULL(0) is stored at the array.

Then, he has to check gotten value is valid or not by himself.

Can't we return -ENOENT here(especially when prealloc() is called) ? But ah, anyway, all-zero elements in allocated array exists ;( and the user can get value from an entry never be put.

If we can fill the first 4bytes of _unused_ index by some magic code like this
#define FLEX_ARRAY_UNUSED_MAGIC (0xa5a5a5a5)
(if maintaining bitmap/status of usage is nonsense)
and flex_array_get() can return -ENOENT, the users will feel easy.

Overprotection ;) ?

Thanks,
-Kame

Quoted 4 lines

lib/flex_array.c new file +4 -0

+	part = fa->parts[part_nr];
+	offset = index_inside_part(fa, element_nr);
+	return &part->elements[index_inside_part(fa, element_nr)];
+}

KAMEZAWA Hiroyuki

2009-07-23 01:46 UTC #2 lore raw

In reply to Dave Hansen

Quoted 9 lines · On Wed, 22 Jul 2009 10:53:45 -0700 Dave Hansen <[email protected]> wrote:

Changes from v2:
- renamed some of the index functions
- added preallocation function
- added flex_array_free_parts() for use with
statically allocated bases
- killed append() function

Quoted 9 lines

Changes from v1:
- to vs too typo
- added __check_part_and_nr() and gave it a warning
- fixed off-by-one check on __nr_part_ptrs()
- added FLEX_ARRAY_INIT() macro
- some kerneldoc comments about the capacity with various sized objects
- comments to note lack of locking semantice

Seems nice, thank you. but a nitpick..

Can't we store "total" somewhere and do error check ?

And some magic value as following can't be defined for 'total' ?

#define FLEX_ARRAY_MAX_ELEMENTS (-1) // Use maximum flex array.

Quoted 8 lines

lib/flex_array.c new file +8 -0

+/**
+ * flex_array_get - pull data back out of the array
+ * @element_nr:	index of the element to fetch from the array
+ *
+ * Returns a pointer to the data at index @element_nr.  Note
+ * that this is a copy of the data that was passed in.  If you
+ * are using this to store pointers, you'll get back &ptr.
+ *

lib/flex_array.c line 233

Quoted 10 lines

lib/flex_array.c new file +10 -0

+ * Locking must be provided by the caller.
+ */
+void *flex_array_get(struct flex_array *fa, int element_nr)
+{
+	int part_nr = fa_element_to_part_nr(fa, element_nr);
+	struct flex_array_part *part;
+	int offset;
+
+	if (__check_part_nr(fa, part_nr))
+		return NULL;

return -EINVAL, here ?
(Can't we compared with stored "total" ?)

lib/flex_array.c line 236

Quoted 3 lines

lib/flex_array.c new file +3 -0
+	if (!fa->parts[part_nr])
+		return NULL;
+

The caller can't know whether
- there are no entry or
- NULL(0) is stored at the array.

Then, he has to check gotten value is valid or not by himself.

Can't we return -ENOENT here(especially when prealloc() is called) ? But ah, anyway, all-zero elements in allocated array exists ;( and the user can get value from an entry never be put.

Overprotection ;) ?

Thanks,
-Kame

Quoted 4 lines

lib/flex_array.c new file +4 -0

+	part = fa->parts[part_nr];
+	offset = index_inside_part(fa, element_nr);
+	return &part->elements[index_inside_part(fa, element_nr)];
+}

Dave Hansen

author

2009-07-23 14:05 UTC #3 lore raw

Quoted 29 lines · KAMEZAWA Hiroyuki

Quoted 7 lines · On Wed, 22 Jul 2009 10:53:45 -0700 Dave Hansen…
Changes from v2:
- renamed some of the index functions
- added preallocation function
- added flex_array_free_parts() for use with
statically allocated bases
- killed append() function
Quoted 8 lines
Changes from v1:
- to vs too typo
- added __check_part_and_nr() and gave it a warning
- fixed off-by-one check on __nr_part_ptrs()
- added FLEX_ARRAY_INIT() macro
- some kerneldoc comments about the capacity with various sized objects
- comments to note lack of locking semantice
Seems nice, thank you. but a nitpick..
lib/flex_array.c new file +8 -0
+struct flex_array *flex_array_alloc(int element_size, int total, gfp_t flags)
+{
+	struct flex_array *ret;
+	int max_size = nr_base_part_ptrs() * __elements_per_part(element_size);
+
+	/* max_size will end up 0 if element_size > PAGE_SIZE */
+	if (total > max_size)
+		return NULL;

Quoted 5 lines

Can't we store "total" somewhere and do error check ?
And some magic value as following can't be defined for 'total' ?
#define FLEX_ARRAY_MAX_ELEMENTS (-1) // Use maximum flex array.

This would all be reasonable to add. The only reason I haven't at this point is that it simplifies things to not have it. When bounds checking, for instance, we just check against the data structure's bounds, which we have to do anyway. There's also no need to worry about resizing since the maximum size is already fixed.

Quoted 12 lines

lib/flex_array.c new file +12 -0

+/**
+ * flex_array_get - pull data back out of the array
+ * @element_nr:	index of the element to fetch from the array
+ *
+ * Returns a pointer to the data at index @element_nr.  Note
+ * that this is a copy of the data that was passed in.  If you
+ * are using this to store pointers, you'll get back &ptr.
+ *
+ * Locking must be provided by the caller.
+ */
+void *flex_array_get(struct flex_array *fa, int element_nr)
+{

Quoted 8 lines

lib/flex_array.c line 233
Quoted 6 lines
lib/flex_array.c new file +6 -0
+	int part_nr = fa_element_to_part_nr(fa, element_nr);
+	struct flex_array_part *part;
+	int offset;
+
+	if (__check_part_nr(fa, part_nr))
+		return NULL;
return -EINVAL, here ?
(Can't we compared with stored "total" ?)

Heh. The radix_tree.c code just BUG_ON()s when you pass it bad indexes. I guess the ranges that might be dealt with here are significantly smaller.

Quoted 9 lines

lib/flex_array.c line 236
Quoted 3 lines
lib/flex_array.c new file +3 -0
+	if (!fa->parts[part_nr])
+		return NULL;
+
The caller can't know whether
- there are no entry or
- NULL(0) is stored at the array.
Then, he has to check gotten value is valid or not by himself.

I think you may be reading this bit wrong. Either that, or I badly miscoded it. :) The flex_array_get() code should returns NULL in cases of error or the *address* of the data element. It will actually return an address pointing into the second-level page.

NULL's two meaning here are:
1. No element was ever put() here and thus there's no data page
2. Your index was out of bounds

(1) is a little fuzzy here. It of course doesn't absolutely guarantee that no put() was done since we just use the data page's existence to tell. This is certainly no worse than a normal C array would be, though.

Quoted 10 lines

Can't we return -ENOENT here(especially when prealloc() is called) ? But ah, anyway, all-zero elements in allocated array exists ;( and the user can get value from an entry never be put.
If we can fill the first 4bytes of _unused_ index by some magic code like this
#define FLEX_ARRAY_UNUSED_MAGIC (0xa5a5a5a5)
(if maintaining bitmap/status of usage is nonsense)
and flex_array_get() can return -ENOENT, the users will feel easy.
Overprotection ;) ?

I intentionally didn't use kzalloc() for the data pages. I figured that users will either initialize it themselves or pass in __GFP_ZERO. I've added a comment to clarify this.

-- Dave

KAMEZAWA Hiroyuki

Reviewed-by

2009-07-23 14:48 UTC #4 lore raw

Quoted 13 lines · Dave Hansen

Quoted 5 lines · KAMEZAWA Hiroyuki
The caller can't know whether
- there are no entry or
- NULL(0) is stored at the array.
Then, he has to check gotten value is valid or not by himself.
I think you may be reading this bit wrong. Either that, or I badly miscoded it. :) The flex_array_get() code should returns NULL in cases of error or the *address* of the data element. It will actually return an address pointing into the second-level page.

Quoted 9 lines

NULL's two meaning here are:
1. No element was ever put() here and thus there's no data page
2. Your index was out of bounds
(1) is a little fuzzy here. It of course doesn't absolutely guarantee that no put() was done since we just use the data page's existence to tell. This is certainly no worse than a normal C array would be, though.

ok, maybe not necessary to handle.

Quoted 10 lines

Quoted 10 lines
Can't we return -ENOENT here(especially when prealloc() is called) ? But ah, anyway, all-zero elements in allocated array exists ;( and the user can get value from an entry never be put.
If we can fill the first 4bytes of _unused_ index by some magic code like this
#define FLEX_ARRAY_UNUSED_MAGIC (0xa5a5a5a5)
(if maintaining bitmap/status of usage is nonsense)
and flex_array_get() can return -ENOENT, the users will feel easy.

Quoted 6 lines

Quoted 1 line
Overprotection ;) ?
I intentionally didn't use kzalloc() for the data pages. I figured that users will either initialize it themselves or pass in __GFP_ZERO. I've added a comment to clarify this.

sorry for noise.
I just felt that this code expects users know all they really do. As said, seems nice :) thank you.

Reviewed-by KAMEZAWA Hiroyuki <[email protected]>

KAMEZAWA Hiroyuki

Reviewed-by

2009-07-23 14:48 UTC #5 lore raw

In reply to Dave Hansen

Quoted 13 lines · Dave Hansen

Quoted 5 lines · KAMEZAWA Hiroyuki
The caller can't know whether
- there are no entry or
- NULL(0) is stored at the array.
Then, he has to check gotten value is valid or not by himself.
I think you may be reading this bit wrong. Either that, or I badly miscoded it. :) The flex_array_get() code should returns NULL in cases of error or the *address* of the data element. It will actually return an address pointing into the second-level page.

Quoted 9 lines

NULL's two meaning here are:
1. No element was ever put() here and thus there's no data page
2. Your index was out of bounds
(1) is a little fuzzy here. It of course doesn't absolutely guarantee that no put() was done since we just use the data page's existence to tell. This is certainly no worse than a normal C array would be, though.

ok, maybe not necessary to handle.

Quoted 10 lines

Quoted 10 lines
Can't we return -ENOENT here(especially when prealloc() is called) ? But ah, anyway, all-zero elements in allocated array exists ;( and the user can get value from an entry never be put.
If we can fill the first 4bytes of _unused_ index by some magic code like this
#define FLEX_ARRAY_UNUSED_MAGIC (0xa5a5a5a5)
(if maintaining bitmap/status of usage is nonsense)
and flex_array_get() can return -ENOENT, the users will feel easy.

Quoted 6 lines

Quoted 1 line
Overprotection ;) ?
I intentionally didn't use kzalloc() for the data pages. I figured that users will either initialize it themselves or pass in __GFP_ZERO. I've added a comment to clarify this.

sorry for noise.
I just felt that this code expects users know all they really do. As said, seems nice :) thank you.

Reviewed-by KAMEZAWA Hiroyuki <[email protected]>

Oren Laadan

2009-07-23 15:44 UTC #6 lore raw

In reply to Dave Hansen

Quoted 42 lines · Dave Hansen

Changes from v2:
- renamed some of the index functions
- added preallocation function
- added flex_array_free_parts() for use with
statically allocated bases
- killed append() function
Changes from v1:
- to vs too typo
- added __check_part_and_nr() and gave it a warning
- fixed off-by-one check on __nr_part_ptrs()
- added FLEX_ARRAY_INIT() macro
- some kerneldoc comments about the capacity with various sized objects
- comments to note lack of locking semantice
--
Once a structure goes over PAGE_SIZE*2, we see occasional allocation failures. Some people have chosen to switch over to things like vmalloc() that will let them keep array-like access to such a large structures. But, vmalloc() has plenty of downsides.
Here's an alternative. I think it's what Andrew was suggesting here:
	http://lkml.org/lkml/2009/7/2/518 
I call it a flexible array. It does all of its work in PAGE_SIZE bits, so never does an order>0 allocation. The base level has PAGE_SIZE-2*sizeof(int) bytes of storage for pointers to the second level. So, with a 32-bit arch, you get about 4MB (4183112 bytes) of total storage when the objects pack nicely into a page. It is half that on 64-bit because the pointers are twice the size. There's a table detailing this in the code.
There are kerneldocs for the functions, but here's an overview:

Quoted 11 lines

flex_array_alloc() - dynamically allocate a base structure flex_array_free() - free the array and all of the
second-level pages
flex_array_free_parts() - free the second-level pages, but
not the base (for static bases)
flex_array_put() - copy into the array at the given index flex_array_get() - copy out of the array at the given index flex_array_prealloc() - preallocate the second-level pages
between the given indexes to guarantee no allocs will occur at put() time.

Probably premature, but -- I wonder if it's worth adding interfaces to:

* copy a range of elements at once (perhaps to/from regular array ? or userspace ? -- depending on potential users)

* (macro ?) iterate through elements (better have it ready for users of flex_array before, than convert their code later on)

Oren.

Dave Hansen

author

2009-07-23 16:02 UTC #7 lore raw

Quoted 4 lines · Oren Laadan

Probably premature, but -- I wonder if it's worth adding interfaces to:
* copy a range of elements at once (perhaps to/from regular array ? or userspace ? -- depending on potential users)

I can see this making some sense, especially if you're dumping a bunch of stuff out to userspace and don't want to worry about page boundaries, etc... Or, if you have a ton of stuff to copy and some of the flex_array overhead is getting in the way. I'm not opposed to it, but I think I'd want to see a user first to make sure I got the implementation right.

Quoted 2 lines

* (macro ?) iterate through elements (better have it ready for users of flex_array before, than convert their code later on)

I've thought about this, but I don't think there's much advantage to doing it. It obfuscates things without any real gain in simplicity. We have LIST_FOR_EACH() because containerof() and some of the typing is a little non-obvious. But, here, I think it's mostly dummy-proof:

We need a 'tmp' variable here because unlike list traversal, there's no position implied in the variables. So, we're looking at (with a new macro):

	int tmp;
	FA_FOR_EACH(fa, var, tmp)
		foo(var);

vs.

	int i;
	for (i = 0; i < fa->total_nr_elements; i++)
		foo(flex_array_get(fa, i));

The macro *looks* cleaner, but I don't think it really buys us much.

-- Dave