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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

Hi Julien,

> > 
> > > 
> > > 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.
I don't know how it should work, but when I ran CI's cppcheck and
Eclair jobs nothing were broken:
https://gitlab.com/xen-project/people/olkur/xen/-/pipelines/1153360853

> 
> But if the other agrees with you, then this change ought to be
> explained 
> in the commit message.
> 
> 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.
I understand your point, I'll do my best next time. 

> > 
> > > 
> > > > -            "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.
Next time I'll add the explanation to the commit message to make it
more clear.

> 
> > 
> > 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.
GENERIC_FIND_FIRST_BIT config was removed in Linux kernel too as all
architectures were switched to use find_{first,last}_bit()
unconditionally:
https://lore.kernel.org/linux-arch/20211005054059.475634-5-yury.norov@xxxxxxxxx/

This is not the case of Xen, as some, at least, x86 has arch specific
implementation of find_{first,last}_bit(), but it can be another one
justification why this config can be removed in Xen.

~ Oleksii