Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <8caea951-5781-92f5-0d12-bd0e66452b77@molgen.mpg.de>
Date:   Sat, 9 Jul 2022 14:27:45 +0200
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.0.1
Subject: Re: [PATCH v2] lib/bitmap: Make parameter len unsigned
To:     Yury Norov <yury.norov@gmail.com>
References: <20220708075239.9057-1-pmenzel@molgen.mpg.de>
 <YshBa/87hSAZwIP3@yury-laptop>
Content-Language: en-US
Cc:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Rasmus Villemoes <linux@rasmusvillemoes.dk>,
        linux-kernel@vger.kernel.org
From:   Paul Menzel <pmenzel@molgen.mpg.de>
In-Reply-To: <YshBa/87hSAZwIP3@yury-laptop>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

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@molgen.mpg.de>

> 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