Re: [PATCH] xen/lib: introduce generic find next bit operations

[Jan Beulich <jbeulich@xxxxxxxx>]

On 26.01.2024 17:08, Julien Grall wrote:
> On 26/01/2024 15:58, Oleksii wrote:
>> On Fri, 2024-01-26 at 13:20 +0000, Julien Grall wrote:
>>> On 26/01/2024 12:20, Oleksii Kurochko wrote:
>>>> find-next-bit.c is common for Arm64, PPC and RISCV64,
>>>> so it is moved to xen/lib.
>>>>
>>>> Suggested-by: Jan Beulich <jbeulich@xxxxxxxx>
>>>> Signed-off-by: Oleksii Kurochko <oleksii.kurochko@xxxxxxxxx>
>>>> ---
>>>>    docs/misra/exclude-list.json                  |   4 -
>>>>    xen/arch/arm/arm64/lib/Makefile               |   2 +-
>>>>    xen/arch/arm/include/asm/arm64/bitops.h       |  48 --------
>>>>    xen/arch/ppc/include/asm/bitops.h             | 115 -------------
>>>> -----
>>>>    xen/include/xen/bitops.h                      |  48 ++++++++
>>>>    xen/lib/Makefile                              |   1 +
>>>>    .../find_next_bit.c => lib/find-next-bit.c}   |   0
>>>>    7 files changed, 50 insertions(+), 168 deletions(-)
>>>>    rename xen/{arch/arm/arm64/lib/find_next_bit.c => lib/find-next-
>>>> bit.c} (100%)
>>>>
>>>> diff --git a/docs/misra/exclude-list.json b/docs/misra/exclude-
>>>> list.json
>>>> index 7971d0e70f..7fe02b059d 100644
>>>> --- a/docs/misra/exclude-list.json
>>>> +++ b/docs/misra/exclude-list.json
>>>> @@ -13,10 +13,6 @@
>>>>                "rel_path": "arch/arm/arm64/insn.c",
>>>>                "comment": "Imported on Linux, ignore for now"
>>>>            },
>>>> -        {
>>>> -            "rel_path": "arch/arm/arm64/lib/find_next_bit.c",
>>>
>>> Rather than removing the section, I was expecting the rel_path to be
>>> updated. Can you explain why you think the exclusion is not
>>> necessary?
>> I considered simply updating the path to xen/lib/find-next-bit.c, but
>> ultimately opted to remove it. This decision was based on the fact that
>> the line in question checks for a file that no longer exists. 
> 
> That's not quite correct. The file still exists but with a different name.
> 
>> If it's
>> preferable to update the rel_path with xen/lib/find-next-bit.c, I'm
>> more than willing to make that adjustment.
> 
> The problem is by removing the file, you effectively tell Eclair to 
> report any MISRA error. I don't believe this is what we want here.
> 
> But if the other agrees with you, then this change ought to be explained 
> in the commit message.

Imo it can only be removed (rather than updated) if the reason for adding
has meanwhile gone away. I don't think that's the case, though.

> To me the key is the commit message should contain enough information 
> for the reviewer to understand what/why you are doing. All the changes I 
> pointed out are definitely not just a normal code movement.
>>
>>>
>>>> -            "comment": "Imported from Linux, ignore for now"
>>>> -        },
>>>>            {
>>>>                "rel_path": "arch/x86/acpi/boot.c",
>>>>                "comment": "Imported from Linux, ignore for now"
>>>> diff --git a/xen/arch/arm/arm64/lib/Makefile
>>>> b/xen/arch/arm/arm64/lib/Makefile
>>>> index 1b9c7a95e6..66cfac435a 100644
>>>> --- a/xen/arch/arm/arm64/lib/Makefile
>>>> +++ b/xen/arch/arm/arm64/lib/Makefile
>>>> @@ -1,4 +1,4 @@
>>>>    obj-y += memcpy.o memcmp.o memmove.o memset.o memchr.o
>>>>    obj-y += clear_page.o
>>>> -obj-y += bitops.o find_next_bit.o
>>>> +obj-y += bitops.o
>>>>    obj-y += strchr.o strcmp.o strlen.o strncmp.o strnlen.o strrchr.o
>>>> diff --git a/xen/arch/arm/include/asm/arm64/bitops.h
>>>> b/xen/arch/arm/include/asm/arm64/bitops.h
>>>> index d85a49bca4..f9dd066237 100644
>>>> --- a/xen/arch/arm/include/asm/arm64/bitops.h
>>>> +++ b/xen/arch/arm/include/asm/arm64/bitops.h
>>>> @@ -36,57 +36,9 @@ static inline int flsl(unsigned long x)
>>>>    
>>>>    /* Based on linux/include/asm-generic/bitops/find.h */
>>>>    
>>>> -#ifndef find_next_bit
>>>> -/**
>>>> - * find_next_bit - find the next set bit in a memory region
>>>> - * @addr: The address to base the search on
>>>> - * @offset: The bitnumber to start searching at
>>>> - * @size: The bitmap size in bits
>>>> - */
>>>> -extern unsigned long find_next_bit(const unsigned long *addr,
>>>> unsigned long
>>>> -          size, unsigned long offset);
>>>> -#endif
>>>> -
>>>> -#ifndef find_next_zero_bit
>>>> -/**
>>>> - * find_next_zero_bit - find the next cleared bit in a memory
>>>> region
>>>> - * @addr: The address to base the search on
>>>> - * @offset: The bitnumber to start searching at
>>>> - * @size: The bitmap size in bits
>>>> - */
>>>> -extern unsigned long find_next_zero_bit(const unsigned long *addr,
>>>> unsigned
>>>> -          long size, unsigned long offset);
>>>> -#endif
>>>> -
>>>> -#ifdef CONFIG_GENERIC_FIND_FIRST_BIT
>>>> -
>>>> -/**
>>>> - * find_first_bit - find the first set bit in a memory region
>>>> - * @addr: The address to start the search at
>>>> - * @size: The maximum size to search
>>>> - *
>>>> - * Returns the bit number of the first set bit.
>>>> - */
>>>> -extern unsigned long find_first_bit(const unsigned long *addr,
>>>> -                              unsigned long size);
>>>> -
>>>> -/**
>>>> - * find_first_zero_bit - find the first cleared bit in a memory
>>>> region
>>>> - * @addr: The address to start the search at
>>>> - * @size: The maximum size to search
>>>> - *
>>>> - * Returns the bit number of the first cleared bit.
>>>> - */
>>>> -extern unsigned long find_first_zero_bit(const unsigned long
>>>> *addr,
>>>> -                                   unsigned long size);
>>>> -#else /* CONFIG_GENERIC_FIND_FIRST_BIT */
>>>> -
>>>>    #define find_first_bit(addr, size) find_next_bit((addr), (size),
>>>> 0)
>>>>    #define find_first_zero_bit(addr, size)
>>>> find_next_zero_bit((addr), (size), 0)
>>>>    
>>>> -#endif /* CONFIG_GENERIC_FIND_FIRST_BIT */
>>>
>>> AFAICT, you are changing the behavior for Arm64 without explaining
>>> why.
>>> Before, it was possible to set CONFIG_GENERIC_FIND_FIRST_BIT so the
>>> generic version of find_first_*_bit are used. This is not possible
>>> anymore with your change.
>>>
>>> Looking at Linux, I see that arm64 is now selecting
>>> GENERIC_FIND_FIRST_BIT (see [1]). So I would argue, we should not
>>> define
>>> find_first_bit(). That said, that's probably a separate patch.
>>>
>>> For now, you want to explain why GENERIC_FIND_FIRST_BIT is dropped.
>> I chose to remove it because I couldn't find any usage or configuration
>> setting for this in Xen (Arm).
> 
> Right. My point is the commit message can lead to think this is a simple 
> code movement and there are no change of behavior.
> 
> It wasn't clear to me whether this was done on purpose or not. It looks 
> like it was and therefore should really be explained in the commit message.
> 
>>
>> I can add "#ifdef GENERIC_FIND_FIRST_BIT" around find_first_zero_bit()
>> and find_first_bit() in xen/bitops.h, and according to the link [1], it
>> should be wrapped with ifdef. Perhaps it would be better to use "#if
>> defined(GENERIC_FIND_FIRST_BIT) && defined(CONFIG_ARM_64)".
>>
>> My only concern is that it might seem somewhat inconsistent with the
>> other find_*_bit() functions added in this patch. Should we be care
>> about that? I mean that do we need similar config or it would be enough
>> to add a comment why it is necessary to have ifdef
>> GENERIC_FIND_FIRST_BIT.
> 
> Note that I didn't ask to keep GENERIC_FIND_FIRST_BIT. I actually 
> pointed out that it can be removed but it should be explained in the 
> commit message.

+1

And, Oleksii, this isn't the first time that it is being pointed out to
you that commit messages need to contain enough information to justify
changes made.

Jan