[PATCH 00/23] further population of xen/lib/

Jan Beulich posted 23 patches 2 years, 12 months ago
Test gitlab-ci failed
Patches applied successfully (tree, apply log)
git fetch https://gitlab.com/xen-project/patchew/xen tags/patchew/c53a6802-8bae-1dc6-5ac4-6238e122aaa4@suse.com
There is a newer version of this series
[PATCH 00/23] further population of xen/lib/
Posted by Jan Beulich 2 years, 12 months ago
This is to dissolve / move xen/common/lib.c and xen/common/string.c.
One benefit of moving these functions into an archive is that we can
drop some of the related __HAVE_ARCH_* #define-s: By living in an
archive, the per-arch functions will preempt any loading of the
respective functions (objects) from the archive. (Down the road we
may want to move the per-arch functions into archives as well, at
which point the per-arch archive(s) would need to be specified ahead
of the common one(s) to the linker.)

01: lib: move muldiv64()
02: lib: move 64-bit div/mod compiler helpers
03: string: drop redundant declarations
04: lib: move memset()
05: lib: move memcpy()
06: lib: move memmove()
07: lib: move memcmp()
08: lib: move memchr()
09: lib: move memchr_inv()
10: lib: move strlen()
11: lib: move strnlen()
12: lib: move strcmp()
13: lib: move strncmp()
14: lib: move strlcpy()
15: lib: move strlcat()
16: lib: move strchr()
17: lib: move strrchr()
18: lib: move strstr()
19: lib: move strcasecmp()
20: lib: move/rename strnicmp() to strncasecmp()
21: lib: move strspn()
22: lib: move strpbrk()
23: lib: move strsep()

Jan

Re: [PATCH 00/23] further population of xen/lib/
Posted by Julien Grall 2 years, 12 months ago
Hi Jan,

On 01/04/2021 11:14, Jan Beulich wrote:
> This is to dissolve / move xen/common/lib.c and xen/common/string.c.
> One benefit of moving these functions into an archive is that we can
> drop some of the related __HAVE_ARCH_* #define-s: By living in an
> archive, the per-arch functions will preempt any loading of the
> respective functions (objects) from the archive. (Down the road we
> may want to move the per-arch functions into archives as well, at
> which point the per-arch archive(s) would need to be specified ahead
> of the common one(s) to the linker.)

While I think it is a good idea to move code in xen/lib, I am not 
convinced that having a single function per file is that beneficial.

Do you have numbers showing how much Xen will shrink after this series?

Cheers,

-- 
Julien Grall

Re: [PATCH 00/23] further population of xen/lib/
Posted by Jan Beulich 2 years, 12 months ago
On 01.04.2021 13:54, Julien Grall wrote:
> On 01/04/2021 11:14, Jan Beulich wrote:
>> This is to dissolve / move xen/common/lib.c and xen/common/string.c.
>> One benefit of moving these functions into an archive is that we can
>> drop some of the related __HAVE_ARCH_* #define-s: By living in an
>> archive, the per-arch functions will preempt any loading of the
>> respective functions (objects) from the archive. (Down the road we
>> may want to move the per-arch functions into archives as well, at
>> which point the per-arch archive(s) would need to be specified ahead
>> of the common one(s) to the linker.)
> 
> While I think it is a good idea to move code in xen/lib, I am not 
> convinced that having a single function per file is that beneficial.
> 
> Do you have numbers showing how much Xen will shrink after this series?

In the default build, from all I was able to tell, there's one function
that's unused (strspn(), as mentioned in the respective patch description).
I don't think I've been claiming any space savings here, though, so I
wonder why you make this a criteria at all. The functions being one per
CU is such that they can be individually overridden by an arch, without
pulling in dead code.

Jan

Re: [PATCH 00/23] further population of xen/lib/
Posted by Julien Grall 2 years, 12 months ago
Hi Jan,

On 01/04/2021 14:43, Jan Beulich wrote:
> On 01.04.2021 13:54, Julien Grall wrote:
>> On 01/04/2021 11:14, Jan Beulich wrote:
>>> This is to dissolve / move xen/common/lib.c and xen/common/string.c.
>>> One benefit of moving these functions into an archive is that we can
>>> drop some of the related __HAVE_ARCH_* #define-s: By living in an
>>> archive, the per-arch functions will preempt any loading of the
>>> respective functions (objects) from the archive. (Down the road we
>>> may want to move the per-arch functions into archives as well, at
>>> which point the per-arch archive(s) would need to be specified ahead
>>> of the common one(s) to the linker.)
>>
>> While I think it is a good idea to move code in xen/lib, I am not
>> convinced that having a single function per file is that beneficial.
>>
>> Do you have numbers showing how much Xen will shrink after this series?
> 
> In the default build, from all I was able to tell, there's one function
> that's unused (strspn(), as mentioned in the respective patch description).
> I don't think I've been claiming any space savings here, though, so I

You didn't. I was trying to guess why you wrote this series given that 
your cover letter doesn't provide a lot of benefits (other than dropping 
__HAVE_ARCH_*).

> wonder why you make this a criteria at all.

Because this is the main reason I would be willing to ack this series. 
This outweight the increase number of files with just a single function 
implemented.

> The functions being one per
> CU is such that they can be individually overridden by an arch, without
> pulling in dead code.

I would agree with functions like memcpy/memset() because you can gain a 
lot to outweight the implementation in assembly. I am not convinced this 
would be true for functions such as strlen().

So overall, the number of functions requiring overriding will likely be 
pretty limited and #ifdef would be IMHO tolerable.

Although, I would be OK with creating a file per function that are 
already overrided. For all the others, I think this is just pointless.

Cheers,

-- 
Julien Grall

Re: [PATCH 00/23] further population of xen/lib/
Posted by Jan Beulich 2 years, 12 months ago
On 01.04.2021 16:04, Julien Grall wrote:
> Hi Jan,
> 
> On 01/04/2021 14:43, Jan Beulich wrote:
>> On 01.04.2021 13:54, Julien Grall wrote:
>>> On 01/04/2021 11:14, Jan Beulich wrote:
>>>> This is to dissolve / move xen/common/lib.c and xen/common/string.c.
>>>> One benefit of moving these functions into an archive is that we can
>>>> drop some of the related __HAVE_ARCH_* #define-s: By living in an
>>>> archive, the per-arch functions will preempt any loading of the
>>>> respective functions (objects) from the archive. (Down the road we
>>>> may want to move the per-arch functions into archives as well, at
>>>> which point the per-arch archive(s) would need to be specified ahead
>>>> of the common one(s) to the linker.)
>>>
>>> While I think it is a good idea to move code in xen/lib, I am not
>>> convinced that having a single function per file is that beneficial.
>>>
>>> Do you have numbers showing how much Xen will shrink after this series?
>>
>> In the default build, from all I was able to tell, there's one function
>> that's unused (strspn(), as mentioned in the respective patch description).
>> I don't think I've been claiming any space savings here, though, so I
> 
> You didn't. I was trying to guess why you wrote this series given that 
> your cover letter doesn't provide a lot of benefits (other than dropping 
> __HAVE_ARCH_*).
> 
>> wonder why you make this a criteria at all.
> 
> Because this is the main reason I would be willing to ack this series. 
> This outweight the increase number of files with just a single function 
> implemented.
> 
>> The functions being one per
>> CU is such that they can be individually overridden by an arch, without
>> pulling in dead code.
> 
> I would agree with functions like memcpy/memset() because you can gain a 
> lot to outweight the implementation in assembly. I am not convinced this 
> would be true for functions such as strlen().

strlen() is actually a pretty good candidate for overriding, and
we may even want to on x86 with newer hardware's "Fast Short REP
CMPSB/SCASB".

> So overall, the number of functions requiring overriding will likely be 
> pretty limited and #ifdef would be IMHO tolerable.
> 
> Although, I would be OK with creating a file per function that are 
> already overrided. For all the others, I think this is just pointless.

Well, I don't see a reason to special case individual functions.
Plus any reasonable static library should imo have one (global)
function per object file in the normal case; there may be very
few exceptions. Drawing an ad hoc boundary at what currently has
an override somewhere doesn't look very attractive to me. Plus
to be honest while I would find it unfair to ask to further
split things if I did just a partial conversion (i.e. invest yet
more time), I find it rather odd to be asked to undo some of the
splitting when I've already taken the extra time to make things
consistent.

Jan

Re: [PATCH 00/23] further population of xen/lib/
Posted by Julien Grall 2 years, 12 months ago

On 01/04/2021 15:27, Jan Beulich wrote:
> On 01.04.2021 16:04, Julien Grall wrote: >> So overall, the number of functions requiring overriding will likely be
>> pretty limited and #ifdef would be IMHO tolerable.
>>
>> Although, I would be OK with creating a file per function that are
>> already overrided. For all the others, I think this is just pointless.
> 
> Well, I don't see a reason to special case individual functions.
> Plus any reasonable static library should imo have one (global)
> function per object file in the normal case; there may be very
> few exceptions. Drawing an ad hoc boundary at what currently has
> an override somewhere doesn't look very attractive to me. Plus
> to be honest while I would find it unfair to ask to further
> split things if I did just a partial conversion (i.e. invest yet
> more time), I find it rather odd to be asked to undo some of the
> splitting when I've already taken the extra time to make things
> consistent.

I am sure each of us spent time working on a solution that some 
reviewers were not necessary convinced of the usefulness and they had to 
undo the series...

In this case, you sent a large series with close to 0 commit message + a 
small cover letter. So I think it is fair for a reviewer to be 
unconvinced and ask for more input.

You provided that now, I think we want a short summary (or a link to the 
conversation) in each commit message.

This will be helpful to understand why the move was made without having 
to spend ages finding the original discussion.

Cheers,

-- 
Julien Grall

Re: [PATCH 00/23] further population of xen/lib/
Posted by Jan Beulich 2 years, 12 months ago
On 01.04.2021 16:55, Julien Grall wrote:
> 
> 
> On 01/04/2021 15:27, Jan Beulich wrote:
>> On 01.04.2021 16:04, Julien Grall wrote: >> So overall, the number of functions requiring overriding will likely be
>>> pretty limited and #ifdef would be IMHO tolerable.
>>>
>>> Although, I would be OK with creating a file per function that are
>>> already overrided. For all the others, I think this is just pointless.
>>
>> Well, I don't see a reason to special case individual functions.
>> Plus any reasonable static library should imo have one (global)
>> function per object file in the normal case; there may be very
>> few exceptions. Drawing an ad hoc boundary at what currently has
>> an override somewhere doesn't look very attractive to me. Plus
>> to be honest while I would find it unfair to ask to further
>> split things if I did just a partial conversion (i.e. invest yet
>> more time), I find it rather odd to be asked to undo some of the
>> splitting when I've already taken the extra time to make things
>> consistent.
> 
> I am sure each of us spent time working on a solution that some 
> reviewers were not necessary convinced of the usefulness and they had to 
> undo the series...
> 
> In this case, you sent a large series with close to 0 commit message + a 
> small cover letter. So I think it is fair for a reviewer to be 
> unconvinced and ask for more input.
> 
> You provided that now, I think we want a short summary (or a link to the 
> conversation) in each commit message.
> 
> This will be helpful to understand why the move was made without having 
> to spend ages finding the original discussion.

I'll add "Allow the function to be individually linkable, discardable,
and overridable." to all the str*() and mem*() patch descriptions. Is
that going to be good enough?

Jan

Re: [PATCH 00/23] further population of xen/lib/
Posted by Julien Grall 2 years, 12 months ago

On 01/04/2021 16:25, Jan Beulich wrote:
> On 01.04.2021 16:55, Julien Grall wrote:
>>
>>
>> On 01/04/2021 15:27, Jan Beulich wrote:
>>> On 01.04.2021 16:04, Julien Grall wrote: >> So overall, the number of functions requiring overriding will likely be
>>>> pretty limited and #ifdef would be IMHO tolerable.
>>>>
>>>> Although, I would be OK with creating a file per function that are
>>>> already overrided. For all the others, I think this is just pointless.
>>>
>>> Well, I don't see a reason to special case individual functions.
>>> Plus any reasonable static library should imo have one (global)
>>> function per object file in the normal case; there may be very
>>> few exceptions. Drawing an ad hoc boundary at what currently has
>>> an override somewhere doesn't look very attractive to me. Plus
>>> to be honest while I would find it unfair to ask to further
>>> split things if I did just a partial conversion (i.e. invest yet
>>> more time), I find it rather odd to be asked to undo some of the
>>> splitting when I've already taken the extra time to make things
>>> consistent.
>>
>> I am sure each of us spent time working on a solution that some
>> reviewers were not necessary convinced of the usefulness and they had to
>> undo the series...
>>
>> In this case, you sent a large series with close to 0 commit message + a
>> small cover letter. So I think it is fair for a reviewer to be
>> unconvinced and ask for more input.
>>
>> You provided that now, I think we want a short summary (or a link to the
>> conversation) in each commit message.
>>
>> This will be helpful to understand why the move was made without having
>> to spend ages finding the original discussion.
> 
> I'll add "Allow the function to be individually linkable, discardable,
> and overridable." to all the str*() and mem*() patch descriptions. Is
> that going to be good enough?
It will be good for me.

Cheers,

> 
> Jan
> 

-- 
Julien Grall