Re: [PATCH] bitmap: Support for pages > BITS_PER_LONG.

[Paul Jackson]

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@xxxxxxx> 1.925.600.0401
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/