lib/bitmap: add bitmap_{read,write}()
Subsystems (2)
PATCH v12 1/3 lib/bitmap: add bitmap_{read,write}()
From: Syed Nayyar Waris <[email protected]>
The two new functions allow reading/writing 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 number of changes and simplifications:
- 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());
- bitmap_get_value()/bitmap_set_value() are renamed to bitmap_read() and bitmap_write();
- some redundant computations are omitted.
---
This patch was previously part of the "Implement MTE tag compression for swapped pages" series
(https://lore.kernel.org/linux-arm-kernel/[email protected]/T/)
This patch was previously called "lib/bitmap: add
bitmap_{set,get}_value()"
(https://lore.kernel.org/lkml/[email protected]/)
v11:
- rearrange whitespace as requested by Andy Shevchenko, add Reviewed-by:, update a comment
v10:
- update comments as requested by Andy Shevchenko
v8:
- as suggested by Andy Shevchenko, handle reads/writes of more than BITS_PER_LONG bits, add a note for 32-bit systems
v7:
- Address comments by Yury Norov, Andy Shevchenko, Rasmus Villemoes:
- update code comments;
- get rid of GENMASK();
- s/assign_bit/__assign_bit;
- more vertical whitespace for better readability;
- more compact code for bitmap_write() (now for real)
v6:
- As suggested by Yury Norov, do not require bitmap_read(..., 0) to return 0.
v5:
- Address comments by Yury Norov:
- updated code comments and patch title/description
- replace GENMASK(nbits - 1, 0) with BITMAP_LAST_WORD_MASK(nbits)
- more compact bitmap_write() implementation
v4:
- Address comments by Andy Shevchenko and Yury Norov:
- prevent passing values >= 64 to GENMASK()
- fix commit authorship
- change comments
- check for unlikely(nbits==0)
- drop unnecessary const declarations
- fix kernel-doc comments
- rename bitmap_{get,set}_value() to bitmap_{read,write}()
--- include/linux/bitmap.h | 77 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 77 insertions(+)
include/linux/bitmap.h +77 -0
@@ -77,6 +77,10 @@ struct device;* bitmap_to_arr64(buf, src, nbits) Copy nbits from buf to u64[] dst * bitmap_get_value8(map, start) Get 8bit value from map at start * bitmap_set_value8(map, value, start) Set 8bit value to map at start +* bitmap_read(map, start, nbits) Read an nbits-sized value from +* map at start +* bitmap_write(map, value, start, nbits) Write an nbits-sized value to +* map at start * * Note, bitmap_zero() and bitmap_fill() operate over the region of * unsigned longs, that is, bits behind bitmap till the unsigned long
@@ -599,6 +603,79 @@ static inline void bitmap_set_value8(unsigned long *map, unsigned long value, map[index] |= value << offset; } +/** +* bitmap_read - read 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, nonzero, up to BITS_PER_LONG +* +* Returns: value of @nbits bits located at the @start bit offset within the +* @map memory region. For @nbits = 0 and @nbits > BITS_PER_LONG the return +* value is undefined. +*/ +static inline unsigned long bitmap_read (const unsigned long *map, +unsigned long start, +unsigned long nbits) +{ +size_t index =BIT_WORD (start); +unsigned long offset = start %BITS_PER_LONG ; +unsigned long space =BITS_PER_LONG - offset; +unsigned long value_low, value_high; + +if (unlikely (!nbits || nbits >BITS_PER_LONG )) +return 0 ; + +if (space >= nbits) +return (map[index] >> offset) &BITMAP_LAST_WORD_MASK (nbits); + + 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); +} + +/** +* bitmap_write - write n-bit value within a memory region +* @map: address to the bitmap memory region +* @value: value to write, clamped to nbits +* @start: bit offset of the n-bit value +* @nbits: size of value in bits, nonzero, up to BITS_PER_LONG. +* +* bitmap_write() behaves as-if implemented as @nbits calls of __assign_bit(), +* i.e. bits beyond @nbits are ignored: +* +* for (bit = 0; bit < nbits; bit++) +* __assign_bit(start + bit, bitmap, val & BIT(bit)); +* +* For @nbits == 0 and @nbits > BITS_PER_LONG no writes are performed. +*/ +static inline void bitmap_write (unsigned long *map,unsigned long value, +unsigned long start,unsigned long nbits) +{ +size_t index; +unsigned long offset; +unsigned long space; +unsigned long mask; +bool fit; + +if (unlikely (!nbits || nbits >BITS_PER_LONG )) +return ; + + mask =BITMAP_LAST_WORD_MASK (nbits); + value &= mask; + offset = start %BITS_PER_LONG ; + space =BITS_PER_LONG - offset; + fit = space >= nbits; + index =BIT_WORD (start); + + map[index] &= (fit ? (~(mask << offset)) : ~BITMAP_FIRST_WORD_MASK (start)); + map[index] |= value << offset; +if (fit) +return ; + + map[index +1 ] &=BITMAP_FIRST_WORD_MASK (start + nbits); + map[index +1 ] |= (value >> space); +} +#endif /* __ASSEMBLY__ */ #endif /* __LINUX_BITMAP_H */
Signature · 1 line
2.42.0.869.gea05f2083d-goog
PATCH v12 2/3 lib/test_bitmap: add tests for bitmap_{read,write}()
Add basic tests ensuring that values can be added at arbitrary positions of the bitmap, including those spanning into the adjacent unsigned longs.
Two new performance tests, test_bitmap_read_perf() and test_bitmap_write_perf(), can be used to assess future performance improvements of bitmap_read() and bitmap_write():
[ 0.431119][ T1] test_bitmap: Time spent in test_bitmap_read_perf: 615253 [ 0.433197][ T1] test_bitmap: Time spent in test_bitmap_write_perf: 916313
(numbers from a Intel(R) Xeon(R) Gold 6154 CPU @ 3.00GHz machine running QEMU).
---
This patch was previously part of the "Implement MTE tag compression for swapped pages" series
(https://lore.kernel.org/linux-arm-kernel/[email protected]/T/)
This patch was previously called
"lib/test_bitmap: add tests for bitmap_{set,get}_value()"
(https://lore.kernel.org/lkml/[email protected]/)
and
"lib/test_bitmap: add tests for bitmap_{set,get}_value_unaligned"
(https://lore.kernel.org/lkml/[email protected]/)
v12:
- as suggested by Alexander Lobakin, replace expect_eq_uint() with expect_eq_ulong() and a cast
v9:
- use WRITE_ONCE() to prevent optimizations in test_bitmap_read_perf()
- update patch description
v8:
- as requested by Andy Shevchenko, add tests for reading/writing sizes > BITS_PER_LONG
v7:
- as requested by Yury Norov, add performance tests for bitmap_read() and bitmap_write()
v6:
- use bitmap API to initialize test bitmaps
- as requested by Yury Norov, do not check the return value of bitmap_read(..., 0)
- fix a compiler warning on 32-bit systems
v5:
- update patch title
- address Yury Norov's comments:
- rename the test cases
- factor out test_bitmap_write_helper() to test writing over
different background patterns;
- add a test case copying a nontrivial value bit-by-bit;
- drop volatile
v4:
- Address comments by Andy Shevchenko: added Reviewed-by: and a link to the previous discussion
- Address comments by Yury Norov:
- expand the bitmap to catch more corner cases
- add code testing that bitmap_set_value() does not touch adjacent bits
- add code testing the nbits==0 case
- rename bitmap_{get,set}_value() to bitmap_{read,write}()
v3:
- switch to using bitmap_{set,get}_value()
- change the expected bit pattern in test_set_get_value(), as the test was incorrectly assuming 0 is the LSB.
--- lib/test_bitmap.c | 179 ++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 172 insertions(+), 7 deletions(-)
lib/test_bitmap.c +172 -7
@@ -60,18 +60,17 @@ static const unsigned long exp3_1_0[] __initconst = { };static bool __init -__check_eq_uint (const char *srcfile,unsigned int line, -const unsigned int exp_uint,unsigned int x) +__check_eq_ulong (const char *srcfile,unsigned int line, +const unsigned long exp_ulong,unsigned long x) { -if (exp_uint != x) { -pr_err ("[%s:%u] expected %u, got %u\n" , - srcfile, line, exp_uint, x); +if (exp_ulong != x) { +pr_err ("[%s:%u] expected %lu, got %lu\n" , + srcfile, line, exp_ulong, x);return false; }return true; } - static bool __init __check_eq_bitmap(const char *srcfile, unsigned int line, const unsigned long *exp_bmap, const unsigned long *bmap,
@@ -185,7 +184,8 @@ __check_eq_str(const char *srcfile, unsigned int line, result; \ }) -#define expect_eq_uint (...) __expect_eq(uint, ##__VA_ARGS__) +#define expect_eq_ulong (...) __expect_eq(ulong, ##__VA_ARGS__) +#define expect_eq_uint (x, y) expect_eq_ulong((unsigned int)(x), (unsigned int)(y))#define expect_eq_bitmap (...) __expect_eq(bitmap, ##__VA_ARGS__)#define expect_eq_pbl (...) __expect_eq(pbl, ##__VA_ARGS__)#define expect_eq_u32_array (...) __expect_eq(u32_array, ##__VA_ARGS__)
@@ -1222,6 +1222,168 @@ static void __init test_bitmap_const_eval(void)BUILD_BUG_ON (~var != ~BIT (25 )); } +/* +* Test bitmap should be big enough to include the cases when start is not in +* the first word, and start+nbits lands in the following word. +*/ +#define TEST_BIT_LEN (1000) + +/* +* Helper function to test bitmap_write() overwriting the chosen byte pattern. +*/ +static void __init test_bitmap_write_helper (const char *pattern) +{ +DECLARE_BITMAP (bitmap,TEST_BIT_LEN ); +DECLARE_BITMAP (exp_bitmap,TEST_BIT_LEN ); +DECLARE_BITMAP (pat_bitmap,TEST_BIT_LEN ); +unsigned long w, r, bit; +int i, n, nbits; + +/* +* Only parse the pattern once and store the result in the intermediate +* bitmap. +*/ +bitmap_parselist (pattern, pat_bitmap,TEST_BIT_LEN ); + +/* +* Check that writing a single bit does not accidentally touch the +* adjacent bits. +*/ +for (i =0 ; i <TEST_BIT_LEN ; i++) { +bitmap_copy (bitmap, pat_bitmap,TEST_BIT_LEN ); +bitmap_copy (exp_bitmap, pat_bitmap,TEST_BIT_LEN ); +for (bit =0 ; bit <=1 ; bit++) { +bitmap_write (bitmap, bit, i,1 ); +__assign_bit (i, exp_bitmap, bit); +expect_eq_bitmap (exp_bitmap, bitmap, +TEST_BIT_LEN ); + } + } + +/* Ensure writing 0 bits does not change anything. */ +bitmap_copy (bitmap, pat_bitmap,TEST_BIT_LEN ); +bitmap_copy (exp_bitmap, pat_bitmap,TEST_BIT_LEN ); +for (i =0 ; i <TEST_BIT_LEN ; i++) { +bitmap_write (bitmap, ~0UL , i,0 ); +expect_eq_bitmap (exp_bitmap, bitmap,TEST_BIT_LEN ); + } + +for (nbits =BITS_PER_LONG ; nbits >=1 ; nbits--) { + w =IS_ENABLED (CONFIG_64BIT ) ?0xdeadbeefdeadbeefUL + :0xdeadbeefUL ; + w >>= (BITS_PER_LONG - nbits); +for (i =0 ; i <=TEST_BIT_LEN - nbits; i++) { +bitmap_copy (bitmap, pat_bitmap,TEST_BIT_LEN ); +bitmap_copy (exp_bitmap, pat_bitmap,TEST_BIT_LEN ); +for (n =0 ; n < nbits; n++) +__assign_bit (i + n, exp_bitmap, w &BIT (n)); +bitmap_write (bitmap, w, i, nbits); +expect_eq_bitmap (exp_bitmap, bitmap,TEST_BIT_LEN ); + r =bitmap_read (bitmap, i, nbits); +expect_eq_ulong (r, w); + } + } +} + +static void __init test_bitmap_read_write (void ) +{ +unsigned char *pattern[3 ] = {"" ,"all:1/2" ,"all" }; +DECLARE_BITMAP (bitmap,TEST_BIT_LEN ); +unsigned long zero_bits =0 , bits_per_long =BITS_PER_LONG ; +unsigned long val; +int i, pi; + +/* +* Reading/writing zero bits should not crash the kernel. +* READ_ONCE() prevents constant folding. +*/ +bitmap_write (NULL ,0 ,0 ,READ_ONCE (zero_bits)); +/* Return value of bitmap_read() is undefined here. */ +bitmap_read (NULL ,0 ,READ_ONCE (zero_bits)); + +/* +* Reading/writing more than BITS_PER_LONG bits should not crash the +* kernel. READ_ONCE() prevents constant folding. +*/ +bitmap_write (NULL ,0 ,0 ,READ_ONCE (bits_per_long) +1 ); +/* Return value of bitmap_read() is undefined here. */ +bitmap_read (NULL ,0 ,READ_ONCE (bits_per_long) +1 ); + +/* +* Ensure that bitmap_read() reads the same value that was previously +* written, and two consequent values are correctly merged. +* The resulting bit pattern is asymmetric to rule out possible issues +* with bit numeration order. +*/ +for (i =0 ; i <TEST_BIT_LEN -7 ; i++) { +bitmap_zero (bitmap,TEST_BIT_LEN ); + +bitmap_write (bitmap,0b10101UL , i,5 ); + val =bitmap_read (bitmap, i,5 ); +expect_eq_ulong (0b10101UL , val); + +bitmap_write (bitmap,0b101UL , i +5 ,3 ); + val =bitmap_read (bitmap, i +5 ,3 ); +expect_eq_ulong (0b101UL , val); + + val =bitmap_read (bitmap, i,8 ); +expect_eq_ulong (0b10110101UL , val); + } + +for (pi =0 ; pi <ARRAY_SIZE (pattern); pi++) +test_bitmap_write_helper (pattern[pi]); +} + +static void __init test_bitmap_read_perf (void ) +{ +DECLARE_BITMAP (bitmap,TEST_BIT_LEN ); +unsigned int cnt, nbits, i; +unsigned long val; +ktime_t time; + +bitmap_fill (bitmap,TEST_BIT_LEN ); + time =ktime_get (); +for (cnt =0 ; cnt <5 ; cnt++) { +for (nbits =1 ; nbits <=BITS_PER_LONG ; nbits++) { +for (i =0 ; i <TEST_BIT_LEN ; i++) { +if (i + nbits >TEST_BIT_LEN ) +break ; +/* +* Prevent the compiler from optimizing away the +* bitmap_read() by using its value. +*/ +WRITE_ONCE (val,bitmap_read (bitmap, i, nbits)); + } + } + } + time =ktime_get () - time; +pr_err ("Time spent in %s:\t%llu\n" , __func__, time); +} + +static void __init test_bitmap_write_perf (void ) +{ +DECLARE_BITMAP (bitmap,TEST_BIT_LEN ); +unsigned int cnt, nbits, i; +unsigned long val =0xfeedface ; +ktime_t time; + +bitmap_zero (bitmap,TEST_BIT_LEN ); + time =ktime_get (); +for (cnt =0 ; cnt <5 ; cnt++) { +for (nbits =1 ; nbits <=BITS_PER_LONG ; nbits++) { +for (i =0 ; i <TEST_BIT_LEN ; i++) { +if (i + nbits >TEST_BIT_LEN ) +break ; +bitmap_write (bitmap, val, i, nbits); + } + } + } + time =ktime_get () - time; +pr_err ("Time spent in %s:\t%llu\n" , __func__, time); +} + +#undef TEST_BIT_LEN +static void __init selftest (void ) {test_zero_clear ();
@@ -1237,6 +1399,9 @@ static void __init selftest(void)test_bitmap_cut ();test_bitmap_print_buf ();test_bitmap_const_eval (); +test_bitmap_read_write (); +test_bitmap_read_perf (); +test_bitmap_write_perf ();test_find_nth_bit (); test_for_each_set_bit();
Signature · 1 line
2.42.0.869.gea05f2083d-goog
PATCH v12 3/3 lib/test_bitmap: use pr_info() for non-error messages
In reply to Alexander Potapenkopr_err() messages may be treated as errors by some log readers, so let us only use them for test failures. For non-error messages, replace them with pr_info().
--- lib/test_bitmap.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
lib/test_bitmap.c +5 -5
@@ -484,7 +484,7 @@ static void __init test_bitmap_parselist(void) }if (ptest.flags &PARSE_TIME ) -pr_err ("parselist: %d: input is '%s' OK, Time: %llu\n" , +pr_info ("parselist: %d: input is '%s' OK, Time: %llu\n" , i, ptest.in, time);#undef ptest
@@ -523,7 +523,7 @@ static void __init test_bitmap_printlist(void) goto out; } -pr_err ("bitmap_print_to_pagebuf: input is '%s', Time: %llu\n" , buf, time); +pr_info ("bitmap_print_to_pagebuf: input is '%s', Time: %llu\n" , buf, time); out:kfree (buf);kfree (bmap);
@@ -601,7 +601,7 @@ static void __init test_bitmap_parse(void) }if (test.flags &PARSE_TIME ) -pr_err ("parse: %d: input is '%s' OK, Time: %llu\n" , +pr_info ("parse: %d: input is '%s' OK, Time: %llu\n" , i, test.in, time); } }
@@ -1357,7 +1357,7 @@ static void __init test_bitmap_read_perf(void) } } time =ktime_get () - time; -pr_err ("Time spent in %s:\t%llu\n" , __func__, time); +pr_info ("Time spent in %s:\t%llu\n" , __func__, time); }static void __init test_bitmap_write_perf (void )
@@ -1379,7 +1379,7 @@ static void __init test_bitmap_write_perf(void) } } time =ktime_get () - time; -pr_err ("Time spent in %s:\t%llu\n" , __func__, time); +pr_info ("Time spent in %s:\t%llu\n" , __func__, time); }#undef TEST_BIT_LEN
Signature · 1 line
2.42.0.869.gea05f2083d-goog
Quoted 16 lines · Alexander Potapenko
From: Syed Nayyar Waris <[email protected]>
The two new functions allow reading/writing values of length up to
BITS_PER_LONGbits at arbitrary position in the bitmap.The code was taken from "bitops: Introduce the
for_each_set_clumpmacro" by Syed Nayyar Waris with a number of changes and simplifications:
- instead of usingroundup(), 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 forbitmap_get_value());
- bitmap_get_value()/bitmap_set_value() are renamed tobitmap_read()andbitmap_write();
- some redundant computations are omitted.
Quoted 8 lines
Cc Arnd Bergmann <[email protected]>Signed-off-by Syed Nayyar Waris <[email protected]>Signed-off-by William Breathitt Gray <[email protected]>Link https://lore.kernel.org/lkml/fe12eedf3666f4af5138de0e70b67a07c7f40338.1592224129.git.syednwaris@gmail.com/ open hereSuggested-by Yury Norov <[email protected]>Co-developed-by Alexander Potapenko <[email protected]>Signed-off-by Alexander Potapenko <[email protected]>Reviewed-by Andy Shevchenko <[email protected]>
Yury, Andy, do you think anything else is needed for this series? If not, whom should we ask to take it?
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Yury Norov
maintainerQuoted 26 lines · Alexander Potapenko
Quoted 16 lines · Alexander Potapenko
From: Syed Nayyar Waris <[email protected]>
The two new functions allow reading/writing values of length up to
BITS_PER_LONGbits at arbitrary position in the bitmap.The code was taken from "bitops: Introduce the
for_each_set_clumpmacro" by Syed Nayyar Waris with a number of changes and simplifications:
- instead of usingroundup(), 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 forbitmap_get_value());
- bitmap_get_value()/bitmap_set_value() are renamed tobitmap_read()andbitmap_write();
- some redundant computations are omitted.Quoted 8 lines
Cc Arnd Bergmann <[email protected]>Signed-off-by Syed Nayyar Waris <[email protected]>Signed-off-by William Breathitt Gray <[email protected]>Link https://lore.kernel.org/lkml/fe12eedf3666f4af5138de0e70b67a07c7f40338.1592224129.git.syednwaris@gmail.com/ open hereSuggested-by Yury Norov <[email protected]>Co-developed-by Alexander Potapenko <[email protected]>Signed-off-by Alexander Potapenko <[email protected]>Reviewed-by Andy Shevchenko <[email protected]>
Quoted 2 lines
Yury, Andy, do you think anything else is needed for this series? If not, whom should we ask to take it?
I probably need to revisit it, but generally it looks OK.
But I can't take new API without having real users. Now that your MTE series is delayed, and Alexander's 'ip_tunnel flags' is not moving as well, there's non-zero chance to merge dead code.
If you want it to be merged, I'd advise you to walk through the kernel sources and find good cases where bitmap_read() and bitmap_write() can be used. Then append this series with such conversions, and I'll be able to move it.
Thanks,
Yury
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
From: Yury Norov <[email protected]>
Date: Mon, 4 Dec 2023 06:22:20 -0800
Quoted 27 lines · Alexander Potapenko
Quoted 26 lines · Alexander Potapenko
Quoted 24 lines
From: Syed Nayyar Waris <[email protected]>
The two new functions allow reading/writing values of length up to
BITS_PER_LONGbits at arbitrary position in the bitmap.The code was taken from "bitops: Introduce the
for_each_set_clumpmacro" by Syed Nayyar Waris with a number of changes and simplifications:
- instead of usingroundup(), 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 forbitmap_get_value());
- bitmap_get_value()/bitmap_set_value() are renamed tobitmap_read()andbitmap_write();
- some redundant computations are omitted.Cc Arnd Bergmann <[email protected]>Signed-off-by Syed Nayyar Waris <[email protected]>Signed-off-by William Breathitt Gray <[email protected]>Link https://lore.kernel.org/lkml/fe12eedf3666f4af5138de0e70b67a07c7f40338.1592224129.git.syednwaris@gmail.com/ open hereSuggested-by Yury Norov <[email protected]>Co-developed-by Alexander Potapenko <[email protected]>Signed-off-by Alexander Potapenko <[email protected]>Reviewed-by Andy Shevchenko <[email protected]>
Quoted 8 lines
Quoted 2 lines
Yury, Andy, do you think anything else is needed for this series? If not, whom should we ask to take it?
I probably need to revisit it, but generally it looks OK.
But I can't take new API without having real users. Now that your MTE series is delayed, and Alexander's '
ip_tunnelflags' is not moving as well, there's non-zero chance to merge dead code.
One of the PFCP developers had a quick vacation, we'll send PFCP with the ip_tunnel bitmap conversion this week.
I'll tell Jakub he'd need to pull boys' tree :D
Quoted 8 lines
If you want it to be merged, I'd advise you to walk through the kernel sources and find good cases where
bitmap_read()andbitmap_write()can be used. Then append this series with such conversions, and I'll be able to move it.Thanks,
Yury
Thanks,
Olek