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 <[email protected]> 1.925.600.0401
-
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/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
