Nick Piggin <[EMAIL PROTECTED]> writes: > On Tue, Aug 21, 2007 at 03:48:42PM -0400, Jeff Moyer wrote: >> Hi, >> >> A while back, Nick Piggin introduced a patch to reduce the node memory >> usage for small files (commit cfd9b7df4abd3257c9e381b0e445817b26a51c0c): >> >> -#define RADIX_TREE_MAP_SHIFT 6 >> +#define RADIX_TREE_MAP_SHIFT (CONFIG_BASE_SMALL ? 4 : 6) >> >> Unfortunately, he didn't take into account the fact that the >> calculation of the maximum path was based on an assumption of having >> to round up: >> >> #define RADIX_TREE_MAX_PATH (RADIX_TREE_INDEX_BITS/RADIX_TREE_MAP_SHIFT + 2) >> >> So, if CONFIG_BASE_SMALL is set, you will end up with a >> RADIX_TREE_MAX_PATH that is one greater than necessary. The practical >> upshot of this is just a bit of wasted memory (one long in the >> height_to_maxindex array, an extra pre-allocated radix tree node per >> cpu, and extra stack usage in a couple of functions), but it seems >> worth getting right. >> >> It's also worth noting that I never build with CONFIG_BASE_SMALL. >> What I did to test this was duplicate the code in a small user-space >> program and check the results of the calculations for max path and the >> contents of the height_to_maxindex array. >> > > OK, after you DIV_ROUND_UP, what is the extra 1 for? For paths, it is because > they are NULL terminated paths I guess (without remembering too hard), and for > height_to_maxindex array it is needed for 0-height trees I think. So it would > be kinda cleaner to have the _real_ MAX_PATH, and two other constants for > this array and the paths arrays (that just happen to be identical due to > implementation). Don't you think? > > But that's not to nack this patch. On the contrary I think your logic is > correct, and it should be fixed. I didn't check the maths myself but I trust > you :)
Hi, Nick, As I said, I wasn't sure what to name the constants for the path and height arrays, so I just commented the +1 there. I've been running this on one of my 64bit test systems successfully for about a day, now. The one code path I was concerned about was radix_tree_node_alloc, since the prealloc array changes size with this patch. I stepped through the code by hand and it looks right to me. Additionally, I wrote a kprobe that would simulate the kmem_cache_alloc failure. I then inserted a node at max height into an empty tree with preemption disabled. I verified, using the crash utility, that the tree was constructed and filled in properly. You can find the kprobe at http://people.redhat.com/jmoyer/radix-tree/. Signed-off-by: Jeff Moyer <[EMAIL PROTECTED]> diff --git a/lib/radix-tree.c b/lib/radix-tree.c index 514efb2..d2655cb 100644 --- a/lib/radix-tree.c +++ b/lib/radix-tree.c @@ -60,9 +60,14 @@ struct radix_tree_path { }; #define RADIX_TREE_INDEX_BITS (8 /* CHAR_BIT */ * sizeof(unsigned long)) -#define RADIX_TREE_MAX_PATH (RADIX_TREE_INDEX_BITS/RADIX_TREE_MAP_SHIFT + 2) +#define RADIX_TREE_MAX_PATH (DIV_ROUND_UP(RADIX_TREE_INDEX_BITS, \ + RADIX_TREE_MAP_SHIFT)) -static unsigned long height_to_maxindex[RADIX_TREE_MAX_PATH] __read_mostly; +/* + * The height_to_maxindex array needs to be one deeper than the maximum + * path as height 0 holds only 1 entry. + */ +static unsigned long height_to_maxindex[RADIX_TREE_MAX_PATH + 1] __read_mostly; /* * Radix tree node cache. @@ -487,7 +492,11 @@ EXPORT_SYMBOL(radix_tree_tag_set); void *radix_tree_tag_clear(struct radix_tree_root *root, unsigned long index, unsigned int tag) { - struct radix_tree_path path[RADIX_TREE_MAX_PATH], *pathp = path; + /* + * The radix tree path needs to be one longer than the maximum path + * since the "list" is null terminated. + */ + struct radix_tree_path path[RADIX_TREE_MAX_PATH + 1], *pathp = path; struct radix_tree_node *slot = NULL; unsigned int height, shift; @@ -882,7 +891,11 @@ static inline void radix_tree_shrink(struct radix_tree_root *root) */ void *radix_tree_delete(struct radix_tree_root *root, unsigned long index) { - struct radix_tree_path path[RADIX_TREE_MAX_PATH], *pathp = path; + /* + * The radix tree path needs to be one longer than the maximum path + * since the "list" is null terminated. + */ + struct radix_tree_path path[RADIX_TREE_MAX_PATH + 1], *pathp = path; struct radix_tree_node *slot = NULL; struct radix_tree_node *to_free; unsigned int height, shift; - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/