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

[Jan Beulich <jbeulich@xxxxxxxx>]

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.

>> 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".

Jan