Re: [PATCH v2] lib/bitmap: Make parameter len unsigned

[Paul Menzel]

Dear Yuri,


Thank you for your reply, and sorry for all the failure messages the
first version created.

Am 08.07.22 um 16:38 schrieb Yury Norov:
> On Fri, Jul 08, 2022 at 09:52:40AM +0200, Paul Menzel wrote:
> > The length is non-negative, so make it unsigned.
> >
> > Signed-off-by: Paul Menzel <pmenzel@xxxxxxxxxxxxx>

> Can you please tell more about your motivation for fixing 
> __bitmap_set?

It’s just about semantics(?) that a count can’t be negative, and only 
seems to generate tiny smaller code (less instructions):

```
__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(-)

> The following __bitmap_clear has the same problem, and
> bitmap_parse{,_user}, and bitmap_print_to_pagebuf, and 
> bitmap_parselist...

Indeed.

> Is there a particular problem that is resolved after fixing 
> __bitmap_set()?
>
> I'm OK if this is a single patch, but for a cleanup work it would be
> more logical to clean everything in a single patch/series...
If you agree, I can change the other places too.


Kind regards,

Paul