Messages in this thread | | | Date | Wed, 18 Jan 2006 22:07:53 -0800 | From | Paul Jackson <> | Subject | Re: [PATCH] bitmap: Support for pages > BITS_PER_LONG. |
| |
It looks to me like I never properly reviewed James patch when it was originally submitted. It has some minor confusions that have gotten slightly worse with this patch.
Just reading the patch, I see the following possible opportunities for improvement:
* Could the calculation of mask: mask = (1ul << (pages - 1)); mask += mask - 1; be simplified to: mask = (1UL << pages) - 1; (and note that 1UL is easier to read than 1ul)
* The variable named 'pages' is misnamed. It happens that the current user of this code is using one bit to represent one page, but that is not something that this code has any business knowing. For this code, it might better be 'nbits' or some such.
* With your 'pages > BITS_PER_LONG' patch, this 'pages' variable becomes more confused. Apparently, it is the number of bits to consider in each word scanned, and 'scan' is the number of words to scan. Hmmm ... that's not quite right either. It seems that 'pages' is first set (in bitmask_find_free_region) to the number of bits total to find, which value is used to compute 'scan', the number of words needed to hold that many bits; then 'pages' is recomputed to be the number of bits in a single word to examine, which value is used to compute the 'mask'. This sort of dual use of a poorly named variable is confusing. I'd suggest two variables - 'nbitstotal' and 'nbitsinword', or some such. Or short variable names, and a comment: int bt; /* total number of bits to find free */ int bw; /* how many bits to consider in one word */
* The block comment states: allocate a memory region from a bitmap This is another confusion between the user of this code, and this current code. I think we are allocating a sequence of consecutive bits of size and allignment some power of two ('order' is that power), not allocating a 'memory region.'
* There is no function block comment for bitmap_allocate_region().
* The include/linux/bitmap.h header comment is missing the three lines specifying these three routines.
-- I won't rest till it's the best ... Programmer, Linux Scalability Paul Jackson <pj@sgi.com> 1.925.600.0401 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |