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
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
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
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
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
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
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
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
© 2016 - 2024 Red Hat, Inc.