From: Yury Norov <yury.norov@gmail.com>
To: Paul Menzel <pmenzel@molgen.mpg.de>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
Rasmus Villemoes <linux@rasmusvillemoes.dk>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3] lib/bitmap: Make count and length parameters unsigned
Date: Sat, 16 Jul 2022 20:57:32 -0700 [thread overview]
Message-ID: <YtOIrI9WqZYN5a5k@yury-laptop> (raw)
In-Reply-To: <20220714164207.52410-1-pmenzel@molgen.mpg.de>
On Thu, Jul 14, 2022 at 06:42:07PM +0200, Paul Menzel wrote:
> Counts and the length are non-negative, so make them unsigned, and adapt the
> while condition in `__bitmap_set()` and `__bitmap_clear()` accordingly.
Hi Paul,
I applied your patch on top of bitmap-for-next, and kernel failed
to boot with it on a very early stage without any logs. I attached
the target with GDB, but there's no any meaningful backtrace.
0xffffc0f6aa39541c in ?? ()
(gdb) i threads
Id Target Id Frame
* 1 Thread 1.1 (CPU#0 [running]) 0xffffc0f6aa39541c in ?? ()
2 Thread 1.2 (CPU#1 [halted ]) 0x0000000000000000 in ?? ()
3 Thread 1.3 (CPU#2 [halted ]) 0x0000000000000000 in ?? ()
4 Thread 1.4 (CPU#3 [halted ]) 0x0000000000000000 in ?? ()
(gdb) bt
#0 0xffffc0f6aa39541c in ?? ()
Backtrace stopped: previous frame identical to this frame (corrupt stack?)
Reproduced on qemu/arm64 and x86_64 with the same results. Can you
try your patch on top of bitmap-for-next and/or next-20220715?
You can find bitmap-for-next here:
https://github.com/norov/linux/
Thanks,
Yury
> For `__bitmap_set()` six less intstructions are used as a result:
>
> ```
> __bitmap_set: __bitmap_set:
> movl %esi, %eax movl %esi, %eax
> movq %rdi, %r8 <
> movl %esi, %ecx movl %esi, %ecx
> movl %edx, %edi | movl %esi, %r8d
> > movl $64, %esi
> > andl $63, %ecx
> shrl $6, %eax shrl $6, %eax
> andl $63, %esi | movl %edx, %r9d
> movq $-1, %rdx | leaq (%rdi,%rax,8), %rax
> leaq (%r8,%rax,8), %rax | subl %ecx, %esi
> leal -64(%rsi,%rdi), %r8d | movq $-1, %rdi
> salq %cl, %rdx | salq %cl, %rdi
> testl %r8d, %r8d | cmpl %edx, %esi
> js .L88 | ja .L85
> movl %r8d, %r9d <
> shrl $6, %r9d <
> leal 1(%r9), %esi <
> leaq (%rax,%rsi,8), %rsi <
> .L86: .L86:
> orq %rdx, (%rax) | subl %esi, %edx
> > orq %rdi, (%rax)
> > movl $64, %esi
> addq $8, %rax addq $8, %rax
> movq $-1, %rdx | movq $-1, %rdi
> cmpq %rsi, %rax | cmpl $63, %edx
> jne .L86 | ja .L86
> sall $6, %r9d <
> subl %r9d, %r8d <
> .L85: .L85:
> testl %r8d, %r8d | testl %edx, %edx
> je .L84 je .L84
> addl %edi, %ecx | leal (%r8,%r9), %ecx
> movq $-1, %rax | movq $-1, %rdx
> negl %ecx negl %ecx
> shrq %cl, %rax | shrq %cl, %rdx
> andq %rax, %rdx | andq %rdx, %rdi
> orq %rdx, (%rsi) | orq %rdi, (%rax)
> .L84: .L84:
> ret ret
> .L88: <
> movq %rax, %rsi <
> movl %edi, %r8d <
> jmp .L85 <
> .size __bitmap_set, .-__bitmap_set .size __bitmap_set, .-__bitmap_set
> .p2align 4 .p2align 4
> .globl __bitmap_clear .globl __bitmap_clear
> .type __bitmap_clear, @function .type __bitmap_clear, @function
> ```
>
> $ diff lib/bitmap.1.S lib/bitmap.2.S | diffstat
> unknown | 55 ++++++++++++++++++++++++-------------------------------
> 1 file changed, 24 insertions(+), 31 deletions(-)
>
> Signed-off-by: Paul Menzel <pmenzel@molgen.mpg.de>
> ---
> v2: Update signature in header file
> v3: Adapt more functions to use unsigned int
>
> include/linux/bitmap.h | 20 ++++++++++----------
> lib/bitmap.c | 26 +++++++++++++-------------
> 2 files changed, 23 insertions(+), 23 deletions(-)
>
> diff --git a/include/linux/bitmap.h b/include/linux/bitmap.h
> index 2e6cd5681040..9f1a097cd5d4 100644
> --- a/include/linux/bitmap.h
> +++ b/include/linux/bitmap.h
> @@ -164,8 +164,8 @@ bool __bitmap_intersects(const unsigned long *bitmap1,
> bool __bitmap_subset(const unsigned long *bitmap1,
> const unsigned long *bitmap2, unsigned int nbits);
> int __bitmap_weight(const unsigned long *bitmap, unsigned int nbits);
> -void __bitmap_set(unsigned long *map, unsigned int start, int len);
> -void __bitmap_clear(unsigned long *map, unsigned int start, int len);
> +void __bitmap_set(unsigned long *map, unsigned int start, unsigned int len);
> +void __bitmap_clear(unsigned long *map, unsigned int start, unsigned int len);
>
> unsigned long bitmap_find_next_zero_area_off(unsigned long *map,
> unsigned long size,
> @@ -198,17 +198,17 @@ bitmap_find_next_zero_area(unsigned long *map,
> }
>
> int bitmap_parse(const char *buf, unsigned int buflen,
> - unsigned long *dst, int nbits);
> + unsigned long *dst, unsigned int nbits);
> int bitmap_parse_user(const char __user *ubuf, unsigned int ulen,
> - unsigned long *dst, int nbits);
> + unsigned long *dst, unsigned int nbits);
> int bitmap_parselist(const char *buf, unsigned long *maskp,
> - int nmaskbits);
> + unsigned int nmaskbits);
> int bitmap_parselist_user(const char __user *ubuf, unsigned int ulen,
> - unsigned long *dst, int nbits);
> + unsigned long *dst, unsigned int nbits);
> void bitmap_remap(unsigned long *dst, const unsigned long *src,
> const unsigned long *old, const unsigned long *new, unsigned int nbits);
> int bitmap_bitremap(int oldbit,
> - const unsigned long *old, const unsigned long *new, int bits);
> + const unsigned long *old, const unsigned long *new, unsigned int bits);
> void bitmap_onto(unsigned long *dst, const unsigned long *orig,
> const unsigned long *relmap, unsigned int bits);
> void bitmap_fold(unsigned long *dst, const unsigned long *orig,
> @@ -224,13 +224,13 @@ void bitmap_copy_le(unsigned long *dst, const unsigned long *src, unsigned int n
> #endif
> unsigned int bitmap_ord_to_pos(const unsigned long *bitmap, unsigned int ord, unsigned int nbits);
> int bitmap_print_to_pagebuf(bool list, char *buf,
> - const unsigned long *maskp, int nmaskbits);
> + const unsigned long *maskp, unsigned int nmaskbits);
>
> extern int bitmap_print_bitmask_to_buf(char *buf, const unsigned long *maskp,
> - int nmaskbits, loff_t off, size_t count);
> + unsigned int nmaskbits, loff_t off, size_t count);
>
> extern int bitmap_print_list_to_buf(char *buf, const unsigned long *maskp,
> - int nmaskbits, loff_t off, size_t count);
> + unsigned int nmaskbits, loff_t off, size_t count);
>
> #define BITMAP_FIRST_WORD_MASK(start) (~0UL << ((start) & (BITS_PER_LONG - 1)))
> #define BITMAP_LAST_WORD_MASK(nbits) (~0UL >> (-(nbits) & (BITS_PER_LONG - 1)))
> diff --git a/lib/bitmap.c b/lib/bitmap.c
> index b18e31ea6e66..d529f7dffc48 100644
> --- a/lib/bitmap.c
> +++ b/lib/bitmap.c
> @@ -348,14 +348,14 @@ int __bitmap_weight(const unsigned long *bitmap, unsigned int bits)
> }
> EXPORT_SYMBOL(__bitmap_weight);
>
> -void __bitmap_set(unsigned long *map, unsigned int start, int len)
> +void __bitmap_set(unsigned long *map, unsigned int start, unsigned int len)
> {
> unsigned long *p = map + BIT_WORD(start);
> const unsigned int size = start + len;
> int bits_to_set = BITS_PER_LONG - (start % BITS_PER_LONG);
> unsigned long mask_to_set = BITMAP_FIRST_WORD_MASK(start);
>
> - while (len - bits_to_set >= 0) {
> + while (len >= bits_to_set) {
> *p |= mask_to_set;
> len -= bits_to_set;
> bits_to_set = BITS_PER_LONG;
> @@ -369,7 +369,7 @@ void __bitmap_set(unsigned long *map, unsigned int start, int len)
> }
> EXPORT_SYMBOL(__bitmap_set);
>
> -void __bitmap_clear(unsigned long *map, unsigned int start, int len)
> +void __bitmap_clear(unsigned long *map, unsigned int start, unsigned int len)
> {
> unsigned long *p = map + BIT_WORD(start);
> const unsigned int size = start + len;
> @@ -445,7 +445,7 @@ EXPORT_SYMBOL(bitmap_find_next_zero_area_off);
> */
> int bitmap_parse_user(const char __user *ubuf,
> unsigned int ulen, unsigned long *maskp,
> - int nmaskbits)
> + unsigned int nmaskbits)
> {
> char *buf;
> int ret;
> @@ -478,7 +478,7 @@ EXPORT_SYMBOL(bitmap_parse_user);
> * actually printed to @buf, excluding terminating '\0'.
> */
> int bitmap_print_to_pagebuf(bool list, char *buf, const unsigned long *maskp,
> - int nmaskbits)
> + unsigned int nmaskbits)
> {
> ptrdiff_t len = PAGE_SIZE - offset_in_page(buf);
>
> @@ -499,7 +499,7 @@ EXPORT_SYMBOL(bitmap_print_to_pagebuf);
> * @count: the maximum number of bytes to print
> */
> static int bitmap_print_to_buf(bool list, char *buf, const unsigned long *maskp,
> - int nmaskbits, loff_t off, size_t count)
> + unsigned int nmaskbits, loff_t off, size_t count)
> {
> const char *fmt = list ? "%*pbl\n" : "%*pb\n";
> ssize_t size;
> @@ -542,7 +542,7 @@ static int bitmap_print_to_buf(bool list, char *buf, const unsigned long *maskp,
> * normal attribute with buf parameter and without offset, count::
> *
> * bitmap_print_to_pagebuf(bool list, char *buf, const unsigned long *maskp,
> - * int nmaskbits)
> + * unsigned int nmaskbits)
> * {
> * }
> *
> @@ -600,7 +600,7 @@ static int bitmap_print_to_buf(bool list, char *buf, const unsigned long *maskp,
> * Returns the number of characters actually printed to @buf
> */
> int bitmap_print_bitmask_to_buf(char *buf, const unsigned long *maskp,
> - int nmaskbits, loff_t off, size_t count)
> + unsigned int nmaskbits, loff_t off, size_t count)
> {
> return bitmap_print_to_buf(false, buf, maskp, nmaskbits, off, count);
> }
> @@ -618,7 +618,7 @@ EXPORT_SYMBOL(bitmap_print_bitmask_to_buf);
> * the print format.
> */
> int bitmap_print_list_to_buf(char *buf, const unsigned long *maskp,
> - int nmaskbits, loff_t off, size_t count)
> + unsigned int nmaskbits, loff_t off, size_t count)
> {
> return bitmap_print_to_buf(true, buf, maskp, nmaskbits, off, count);
> }
> @@ -793,7 +793,7 @@ static const char *bitmap_parse_region(const char *str, struct region *r)
> * - ``-ERANGE``: bit number specified too large for mask
> * - ``-EOVERFLOW``: integer overflow in the input parameters
> */
> -int bitmap_parselist(const char *buf, unsigned long *maskp, int nmaskbits)
> +int bitmap_parselist(const char *buf, unsigned long *maskp, unsigned int nmaskbits)
> {
> struct region r;
> long ret;
> @@ -836,7 +836,7 @@ EXPORT_SYMBOL(bitmap_parselist);
> */
> int bitmap_parselist_user(const char __user *ubuf,
> unsigned int ulen, unsigned long *maskp,
> - int nmaskbits)
> + unsigned int nmaskbits)
> {
> char *buf;
> int ret;
> @@ -893,7 +893,7 @@ static const char *bitmap_get_x32_reverse(const char *start,
> * Leading, embedded and trailing whitespace accepted.
> */
> int bitmap_parse(const char *start, unsigned int buflen,
> - unsigned long *maskp, int nmaskbits)
> + unsigned long *maskp, unsigned int nmaskbits)
> {
> const char *end = strnchrnul(start, buflen, '\n') - 1;
> int chunks = BITS_TO_U32(nmaskbits);
> @@ -1068,7 +1068,7 @@ EXPORT_SYMBOL(bitmap_remap);
> * returns 13.
> */
> int bitmap_bitremap(int oldbit, const unsigned long *old,
> - const unsigned long *new, int bits)
> + const unsigned long *new, unsigned int bits)
> {
> int w = bitmap_weight(new, bits);
> int n = bitmap_pos_to_ord(old, oldbit, bits);
> --
> 2.36.1
prev parent reply other threads:[~2022-07-17 3:57 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-07-14 16:42 [PATCH v3] lib/bitmap: Make count and length parameters unsigned Paul Menzel
2022-07-14 16:47 ` Andy Shevchenko
2022-07-14 18:12 ` Yury Norov
2022-07-17 3:57 ` Yury Norov [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=YtOIrI9WqZYN5a5k@yury-laptop \
--to=yury.norov@gmail.com \
--cc=andriy.shevchenko@linux.intel.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@rasmusvillemoes.dk \
--cc=pmenzel@molgen.mpg.de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.