On Mon, Jul 17, 2023 at 5:51 PM Yury Norov [off-list ref] wrote:

On Mon, Jul 17, 2023 at 01:37:04PM +0200, Alexander Potapenko wrote:

quoted

The two new functions allow setting/getting values of length up to
BITS_PER_LONG bits at arbitrary position in the bitmap.

The code was taken from "bitops: Introduce the for_each_set_clump macro"
by Syed Nayyar Waris with a couple of minor changes:
 - instead of using roundup(), which adds an unnecessary dependency
   on <linux/math.h>, we calculate space as BITS_PER_LONG-offset;
 - indentation is reduced by not using else-clauses (suggested by
   checkpatch for bitmap_get_value())

Please preserve Syed's authorship ('From' field in git log).

Done

quoted

  *  bitmap_set_value8(map, value, start)        Set 8bit value to map at start
+ *  bitmap_set_value(map, value, start, nbits)  Set bit value of size 'nbits'
+ *                                              of map at start

The 'bit value of size' sounds more confusing than it should. The size
of bit is actually a bit... Can you rephrase?

How about "Get an nbits-sized value from map at start" and "Set an
nbits-sized value to map at start"?

Moreover, 'set bits' has
a meaning of actually setting them, i.e. switching to '1'. Maybe:
"Copy 'nbits' to bitmap starting at 'start'"?

Right now it is in line with the comment for bitmap_set_value8 (and
the names of the functions also have _set_ in them).
Shall I also change that comment?
WDYT about "Put an nbits-sized value to map at start"?

quoted

+/**
+ * bitmap_get_value - get a value of n-bits from the memory region
+ * @map: address to the bitmap memory region
+ * @start: bit offset of the n-bit value
+ * @nbits: size of value in bits

 * @nbits: size of value in bits, up to BITS_PER_LONG

Ok

quoted

+ *
+ * Returns value of nbits located at the @start bit offset within the @map
+ * memory region.
+ */
+static inline unsigned long bitmap_get_value(const unsigned long *map,
+                                          unsigned long start,
+                                          unsigned long nbits)
+{
+     const size_t index = BIT_WORD(start);
+     const unsigned long offset = start % BITS_PER_LONG;
+     const unsigned long space = BITS_PER_LONG - offset;
+     unsigned long value_low, value_high;
+
+     if (space >= nbits)
+             return (map[index] >> offset) & GENMASK(nbits - 1, 0);
+     value_low = map[index] & BITMAP_FIRST_WORD_MASK(start);
+     value_high = map[index + 1] & BITMAP_LAST_WORD_MASK(start + nbits);
+     return (value_low >> offset) | (value_high << space);
+}

When nbits == 0, copy-like functions shouldn't touch any memory. See how
other bitmap and find_bit functions hold it.

I think this is different from what other bitmap functions do, but it
should be enough to bail out on !nbits, i.e.:

    if (!nbits)
        return 0;

You probably meant adding a __builtin_constant_p() (which is used all
over the place in bitmap.h), but:
 - the compiler won't have problem optimizing away the code for a
constant nbits=0;
 - we anyway need a dynamic check for the case nbits is not constant
(for both bitmap_get_value() and bitmap_set_value(), I assume).

What do you think?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel