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

[Oleksii <oleksii.kurochko@xxxxxxxxx>]

On Wed, 2024-01-24 at 12:24 +0100, Jan Beulich wrote:
> On 24.01.2024 10:34, Oleksii wrote:
> > On Tue, 2024-01-23 at 14:37 +0100, Jan Beulich wrote:
> > > On 23.01.2024 13:34, Oleksii wrote:
> > > > On Tue, 2024-01-23 at 12:14 +0100, Jan Beulich wrote:
> > > > > On 22.12.2023 16:13, Oleksii Kurochko wrote:
> > > > > > --- a/xen/common/Kconfig
> > > > > > +++ b/xen/common/Kconfig
> > > > > > @@ -47,6 +47,9 @@ config ARCH_MAP_DOMAIN_PAGE
> > > > > >  config GENERIC_BUG_FRAME
> > > > > >     bool
> > > > > >  
> > > > > > +config GENERIC_FIND_NEXT_BIT
> > > > > > +   bool
> > > > > 
> > > > > There's no need for this, as ...
> > > > > 
> > > > > > --- a/xen/lib/Makefile
> > > > > > +++ b/xen/lib/Makefile
> > > > > > @@ -3,6 +3,7 @@ obj-$(CONFIG_X86) += x86/
> > > > > >  lib-y += bsearch.o
> > > > > >  lib-y += ctors.o
> > > > > >  lib-y += ctype.o
> > > > > > +lib-$(CONFIG_GENERIC_FIND_NEXT_BIT) += find-next-bit.o
> > > > > 
> > > > > ... you're moving this to lib/. Or have you encountered any
> > > > > issue
> > > > > with building this uniformly, and you forgot to mention this
> > > > > in
> > > > > the description?
> > > > I didn't check. My intention was to provide opportunity to
> > > > check if
> > > > an
> > > > architecture want to use generic version or not. Otherwise, I
> > > > expected
> > > > that we will have multiple definiotion of the funcion.
> > > > 
> > > > But considering that they are all defined under #ifdef...#endif
> > > > we
> > > > can
> > > > remove the declaration of the config GENERIC_FIND_NEXT_BIT.
> > > 
> > > What #ifdef / #endif would matter here? Whats in lib/ is intended
> > > to
> > > be
> > > generic anyway. And what is in the resulting lib.a won't be used
> > > by
> > > an
> > > arch if it has an arch-specific implementation. 
> > If what is implemented in lib.a won't be used by an arch if it has
> > an
> > arch-specific implementation then, for sure, I have to drop
> > CONFIG_GENERIC_FIND_NEXT_BIT.
> > But I am not really understand if lib.a is linked with Xen, then it
> > should be an issue then if some arch implement find-next-bit
> > function
> > we will have to multiple definitions ( one in lib.a and one arch
> > specific ). Probably, I have to look at how it is done.
> 
> You're aware how linking works? Objects are pulled out of archives
> only
> if there's no other definition for a to-be-resolved symbol provided
> by
> a particular object in the archive.
I wasn't aware about the case of the archive. Thanks for the
explanation.

> 
> > > Problems could arise if
> > > an arch had an inline function colliding with the out-of-line
> > > one.
> > > But
> > > that's about the old case where I could see a need to make the
> > > building
> > > of one of the objects conditional. And you'll note that withing
> > > this
> > > Makefile there are pretty few conditionals.
> > Could you please clarify What does it mean "out-of-line" ?
> 
> "not inline"
> 
> > > > > > --- /dev/null
> > > > > > +++ b/xen/lib/find-next-bit.c
> > > > > > [...]
> > > > > 
> > > > > I was going to ask that you convince git to actually present
> > > > > a
> > > > > proper
> > > > > diff, to make visible what changes. But other than the
> > > > > description
> > > > > says
> > > > > you don't really move the file, you copy it. Judging from
> > > > > further
> > > > > titles
> > > > > there's also nowhere you'd make Arm actually use this now
> > > > > generic
> > > > > code.
> > > > I wanted to do it separately, outside this patch series to
> > > > simplify
> > > > review and not have Arm specific changes in RISC-V patch
> > > > series.
> > > 
> > > Then do it the other way around: Make a separate _prereq_ change
> > > truly
> > > moving the file.
> > So this one patch should be separated by 2? One which moves find-
> > next-
> > bit.c from Arm to xen/lib, and second where xen/lib/Makefile is
> > updated.
> 
> No, that would break the Arm build. I suggested breaking out this
> patch from the series, and then doing what the description says:
> Actually move the file. I don't think I suggested splitting the
> patch. Even the breaking out of the series was only because you
> said "I wanted to do it separately, outside this patch series".
What I meant was that I would like to have a patch which introduces
generic version of find-next-bit in the current patch series and
provide a separate patch outside of the current patch series which
switches Arm to use generic version.

~ Oleksii