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

[Jan Beulich <jbeulich@xxxxxxxx>]

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

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

> Regarding a proper diff, you would like me to make git shows that it
> was copy from Arm and it is not newly created file. Am I understand you
> correctly?

Not quite, I think. Git has move detection (and we've seen that in
action in other patches of yours). So when truly moving a file, what
(if anything) is changed is easily visible.

Jan