| Message ID | c53a6802-8bae-1dc6-5ac4-6238e122aaa4@suse.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | further population of xen/lib/ | expand |
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,
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,
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,
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 >