| Message ID | 20210324172608.302316-1-george.dunlap@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | CHANGELOG.md: Make PV shim smaller by factoring out HVM-specific shadow code | expand |
On 24.03.2021 18:26, George Dunlap wrote: > Signed-off-by: George Dunlap <george.dunlap@citrix.com> > --- > Missed one from my list when creating the other series > > CC: Ian Jackson <ian.jackson@citrix.com> > CC: Jan Beulich <jbeulich@suse.com> > CC: Roger Pau Monne <roger.pau@citrix.com> > --- > CHANGELOG.md | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/CHANGELOG.md b/CHANGELOG.md > index 15a22d6bde..49832ae017 100644 > --- a/CHANGELOG.md > +++ b/CHANGELOG.md > @@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) > - x86_emulate: Expanded testing for several instruction classes > - CI loop: Add Alpine Linux, Ubuntu Focal targets; drop CentOS 6 > - CI loop: Add dom0less aarch64 smoke test > + - Factored out HVM-specific shadow code, allowing PV shim to be slimmer But shadow code doesn't get included by default in shim-exclusive builds (and others are unlikely to disable HVM). Also, just to mention it - some of the patches in the direction of !HVM builds getting smaller are still pending. They've been acked (for the most part at least), but couldn't be posted in time (sitting behind the PV guest accessors changes, which were waiting for a decision security-wise). Jan
> On Mar 25, 2021, at 7:08 AM, Jan Beulich <JBeulich@suse.com> wrote: > > On 24.03.2021 18:26, George Dunlap wrote: >> Signed-off-by: George Dunlap <george.dunlap@citrix.com> >> --- >> Missed one from my list when creating the other series >> >> CC: Ian Jackson <ian.jackson@citrix.com> >> CC: Jan Beulich <jbeulich@suse.com> >> CC: Roger Pau Monne <roger.pau@citrix.com> >> --- >> CHANGELOG.md | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/CHANGELOG.md b/CHANGELOG.md >> index 15a22d6bde..49832ae017 100644 >> --- a/CHANGELOG.md >> +++ b/CHANGELOG.md >> @@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) >> - x86_emulate: Expanded testing for several instruction classes >> - CI loop: Add Alpine Linux, Ubuntu Focal targets; drop CentOS 6 >> - CI loop: Add dom0less aarch64 smoke test >> + - Factored out HVM-specific shadow code, allowing PV shim to be slimmer > > But shadow code doesn't get included by default in shim-exclusive > builds (and others are unlikely to disable HVM). Can you propose some better text please? -George
On 29.03.2021 18:12, George Dunlap wrote: >> On Mar 25, 2021, at 7:08 AM, Jan Beulich <JBeulich@suse.com> wrote: >> On 24.03.2021 18:26, George Dunlap wrote: >>> Signed-off-by: George Dunlap <george.dunlap@citrix.com> >>> --- >>> Missed one from my list when creating the other series >>> >>> CC: Ian Jackson <ian.jackson@citrix.com> >>> CC: Jan Beulich <jbeulich@suse.com> >>> CC: Roger Pau Monne <roger.pau@citrix.com> >>> --- >>> CHANGELOG.md | 1 + >>> 1 file changed, 1 insertion(+) >>> >>> diff --git a/CHANGELOG.md b/CHANGELOG.md >>> index 15a22d6bde..49832ae017 100644 >>> --- a/CHANGELOG.md >>> +++ b/CHANGELOG.md >>> @@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) >>> - x86_emulate: Expanded testing for several instruction classes >>> - CI loop: Add Alpine Linux, Ubuntu Focal targets; drop CentOS 6 >>> - CI loop: Add dom0less aarch64 smoke test >>> + - Factored out HVM-specific shadow code, allowing PV shim to be slimmer >> >> But shadow code doesn't get included by default in shim-exclusive >> builds (and others are unlikely to disable HVM). > > Can you propose some better text please? Does this need mentioning here in the first place? Jan
On 29/03/2021 17:23, Jan Beulich wrote: > On 29.03.2021 18:12, George Dunlap wrote: >>> On Mar 25, 2021, at 7:08 AM, Jan Beulich <JBeulich@suse.com> wrote: >>> On 24.03.2021 18:26, George Dunlap wrote: >>>> Signed-off-by: George Dunlap <george.dunlap@citrix.com> >>>> --- >>>> Missed one from my list when creating the other series >>>> >>>> CC: Ian Jackson <ian.jackson@citrix.com> >>>> CC: Jan Beulich <jbeulich@suse.com> >>>> CC: Roger Pau Monne <roger.pau@citrix.com> >>>> --- >>>> CHANGELOG.md | 1 + >>>> 1 file changed, 1 insertion(+) >>>> >>>> diff --git a/CHANGELOG.md b/CHANGELOG.md >>>> index 15a22d6bde..49832ae017 100644 >>>> --- a/CHANGELOG.md >>>> +++ b/CHANGELOG.md >>>> @@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) >>>> - x86_emulate: Expanded testing for several instruction classes >>>> - CI loop: Add Alpine Linux, Ubuntu Focal targets; drop CentOS 6 >>>> - CI loop: Add dom0less aarch64 smoke test >>>> + - Factored out HVM-specific shadow code, allowing PV shim to be slimmer >>> But shadow code doesn't get included by default in shim-exclusive >>> builds (and others are unlikely to disable HVM). >> Can you propose some better text please? > Does this need mentioning here in the first place? I would recommend not. We've been doing incremental improvements for the shim for several releases now, and in this case, we're literally talking a few kb of code. As we already align to 2M boundaries for superpage reasons, there almost certainly isn't actually a reduction in runtime size. ~Andrew
> On Mar 29, 2021, at 6:26 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: > > On 29/03/2021 17:23, Jan Beulich wrote: >> On 29.03.2021 18:12, George Dunlap wrote: >>>> On Mar 25, 2021, at 7:08 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>> On 24.03.2021 18:26, George Dunlap wrote: >>>>> Signed-off-by: George Dunlap <george.dunlap@citrix.com> >>>>> --- >>>>> Missed one from my list when creating the other series >>>>> >>>>> CC: Ian Jackson <ian.jackson@citrix.com> >>>>> CC: Jan Beulich <jbeulich@suse.com> >>>>> CC: Roger Pau Monne <roger.pau@citrix.com> >>>>> --- >>>>> CHANGELOG.md | 1 + >>>>> 1 file changed, 1 insertion(+) >>>>> >>>>> diff --git a/CHANGELOG.md b/CHANGELOG.md >>>>> index 15a22d6bde..49832ae017 100644 >>>>> --- a/CHANGELOG.md >>>>> +++ b/CHANGELOG.md >>>>> @@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) >>>>> - x86_emulate: Expanded testing for several instruction classes >>>>> - CI loop: Add Alpine Linux, Ubuntu Focal targets; drop CentOS 6 >>>>> - CI loop: Add dom0less aarch64 smoke test >>>>> + - Factored out HVM-specific shadow code, allowing PV shim to be slimmer >>>> But shadow code doesn't get included by default in shim-exclusive >>>> builds (and others are unlikely to disable HVM). >>> Can you propose some better text please? >> Does this need mentioning here in the first place? > > I would recommend not. > > We've been doing incremental improvements for the shim for several > releases now, and in this case, we're literally talking a few kb of > code. As we already align to 2M boundaries for superpage reasons, there > almost certainly isn't actually a reduction in runtime size. I don’t understand why the two of you are downplaying your work so much. Yes, these are all only incremental improvements; but they are improvements; and the cumulative effect of loads of incremental improvements can be significant. Communicating to people just what the nature of all these incremental improvements are is important. I mean, look at the release notes for Go 1.15 [1]. It includes things like this: "JSEscape now consistently uses Unicode escapes (\u00XX), which are compatible with JSON." "go test -v now groups output by test name, rather than printing the test name on each line." Those sound far more trivial than “Even more shadow code has been moved to an HVM-specific file”. If the approach is going to be “SUPER IMPORTANT SPECIAL STUFF ONLY", I’d recommend removing CHANGELOG.md. Having an official list that says, “Well, really, we only did 2 things this release” is going to be actively harmful. -George [1] https://golang.org/doc/go1.15
On 30.03.2021 11:56, George Dunlap wrote: > > >> On Mar 29, 2021, at 6:26 PM, Andrew Cooper <andrew.cooper3@citrix.com> wrote: >> >> On 29/03/2021 17:23, Jan Beulich wrote: >>> On 29.03.2021 18:12, George Dunlap wrote: >>>>> On Mar 25, 2021, at 7:08 AM, Jan Beulich <JBeulich@suse.com> wrote: >>>>> On 24.03.2021 18:26, George Dunlap wrote: >>>>>> Signed-off-by: George Dunlap <george.dunlap@citrix.com> >>>>>> --- >>>>>> Missed one from my list when creating the other series >>>>>> >>>>>> CC: Ian Jackson <ian.jackson@citrix.com> >>>>>> CC: Jan Beulich <jbeulich@suse.com> >>>>>> CC: Roger Pau Monne <roger.pau@citrix.com> >>>>>> --- >>>>>> CHANGELOG.md | 1 + >>>>>> 1 file changed, 1 insertion(+) >>>>>> >>>>>> diff --git a/CHANGELOG.md b/CHANGELOG.md >>>>>> index 15a22d6bde..49832ae017 100644 >>>>>> --- a/CHANGELOG.md >>>>>> +++ b/CHANGELOG.md >>>>>> @@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) >>>>>> - x86_emulate: Expanded testing for several instruction classes >>>>>> - CI loop: Add Alpine Linux, Ubuntu Focal targets; drop CentOS 6 >>>>>> - CI loop: Add dom0less aarch64 smoke test >>>>>> + - Factored out HVM-specific shadow code, allowing PV shim to be slimmer >>>>> But shadow code doesn't get included by default in shim-exclusive >>>>> builds (and others are unlikely to disable HVM). >>>> Can you propose some better text please? >>> Does this need mentioning here in the first place? >> >> I would recommend not. >> >> We've been doing incremental improvements for the shim for several >> releases now, and in this case, we're literally talking a few kb of >> code. As we already align to 2M boundaries for superpage reasons, there >> almost certainly isn't actually a reduction in runtime size. > > I don’t understand why the two of you are downplaying your work so much. Yes, these are all only incremental improvements; but they are improvements; and the cumulative effect of loads of incremental improvements can be significant. Communicating to people just what the nature of all these incremental improvements are is important. > > I mean, look at the release notes for Go 1.15 [1]. It includes things like this: > > "JSEscape now consistently uses Unicode escapes (\u00XX), which are compatible with JSON." > > "go test -v now groups output by test name, rather than printing the test name on each line." > > Those sound far more trivial than “Even more shadow code has been moved to an HVM-specific file”. > > If the approach is going to be “SUPER IMPORTANT SPECIAL STUFF ONLY", I’d recommend removing CHANGELOG.md. Having an official list that says, “Well, really, we only did 2 things this release” is going to be actively harmful. I don't think it needs to be "super important" only, but it ought to be at least user visible / user recognizable imo. Jan
George Dunlap writes ("Re: [PATCH] CHANGELOG.md: Make PV shim smaller by factoring out HVM-specific shadow code"):
> I don’t understand why the two of you are downplaying your work so much. Yes, these are all only incremental improvements; but they are improvements; and the cumulative effect of loads of incremental improvements can be significant. Communicating to people just what the nature of all these incremental improvements are is important.
I agree with George here.
There ae a number of reasons why behind-the-scenes work with little
(intentional) user-visible impact are useful to note in the
CHANGELOG.md. With my Release Manager hat on I would like to see, for
example,
>> + - Factored out HVM-specific shadow code, allowing PV shim to be slimmer
something about htis work in the CHANGELOG.md.
IDK precisely, and I don't think George does either, what a good and
accurate statement is. But I guess we will go with the text above if
we don't get something better.
George, were there other changelog items that were subject to a
a similar question ? I don't find them in my email with a quick look
but I suspect I have missed one or two ?
Thanks,
Ian,
On 31.03.2021 15:52, Ian Jackson wrote: > George Dunlap writes ("Re: [PATCH] CHANGELOG.md: Make PV shim smaller by factoring out HVM-specific shadow code"): >> I don’t understand why the two of you are downplaying your work so much. Yes, these are all only incremental improvements; but they are improvements; and the cumulative effect of loads of incremental improvements can be significant. Communicating to people just what the nature of all these incremental improvements are is important. > > I agree with George here. > > There ae a number of reasons why behind-the-scenes work with little > (intentional) user-visible impact are useful to note in the > CHANGELOG.md. With my Release Manager hat on I would like to see, for > example, > >>> + - Factored out HVM-specific shadow code, allowing PV shim to be slimmer > > something about htis work in the CHANGELOG.md. > > IDK precisely, and I don't think George does either, what a good and > accurate statement is. But I guess we will go with the text above if > we don't get something better. At the very least the part after the comma ought to be deleted. As said in an earlier reply, at least the shim default config disables shadow code anyway, so the factoring out has no effect there. Jan
> On Mar 31, 2021, at 2:54 PM, Jan Beulich <jbeulich@suse.com> wrote: > > On 31.03.2021 15:52, Ian Jackson wrote: >> George Dunlap writes ("Re: [PATCH] CHANGELOG.md: Make PV shim smaller by factoring out HVM-specific shadow code"): >>> I don’t understand why the two of you are downplaying your work so much. Yes, these are all only incremental improvements; but they are improvements; and the cumulative effect of loads of incremental improvements can be significant. Communicating to people just what the nature of all these incremental improvements are is important. >> >> I agree with George here. >> >> There ae a number of reasons why behind-the-scenes work with little >> (intentional) user-visible impact are useful to note in the >> CHANGELOG.md. With my Release Manager hat on I would like to see, for >> example, >> >>>> + - Factored out HVM-specific shadow code, allowing PV shim to be slimmer >> >> something about htis work in the CHANGELOG.md. >> >> IDK precisely, and I don't think George does either, what a good and >> accurate statement is. But I guess we will go with the text above if >> we don't get something better. > > At the very least the part after the comma ought to be deleted. As > said in an earlier reply, at least the shim default config disables > shadow code anyway, so the factoring out has no effect there. Thanks. So when you wrote the series, what was your motivation? Did you have a particular technical outcome in mind? Or did it just bother you that there was HVM-only code in a PV-only build? :-) -George
On 31.03.2021 16:00, George Dunlap wrote: > > >> On Mar 31, 2021, at 2:54 PM, Jan Beulich <jbeulich@suse.com> wrote: >> >> On 31.03.2021 15:52, Ian Jackson wrote: >>> George Dunlap writes ("Re: [PATCH] CHANGELOG.md: Make PV shim smaller by factoring out HVM-specific shadow code"): >>>> I don’t understand why the two of you are downplaying your work so much. Yes, these are all only incremental improvements; but they are improvements; and the cumulative effect of loads of incremental improvements can be significant. Communicating to people just what the nature of all these incremental improvements are is important. >>> >>> I agree with George here. >>> >>> There ae a number of reasons why behind-the-scenes work with little >>> (intentional) user-visible impact are useful to note in the >>> CHANGELOG.md. With my Release Manager hat on I would like to see, for >>> example, >>> >>>>> + - Factored out HVM-specific shadow code, allowing PV shim to be slimmer >>> >>> something about htis work in the CHANGELOG.md. >>> >>> IDK precisely, and I don't think George does either, what a good and >>> accurate statement is. But I guess we will go with the text above if >>> we don't get something better. >> >> At the very least the part after the comma ought to be deleted. As >> said in an earlier reply, at least the shim default config disables >> shadow code anyway, so the factoring out has no effect there. > > Thanks. So when you wrote the series, what was your motivation? Did you have a particular technical outcome in mind? Or did it just bother you that there was HVM-only code in a PV-only build? :-) What bothers me are more the implications - it being rather hard in many cases, and in particular in shadow code, to be able to tell what paths are involved in the handling of what kind(s) of guests. This has made more time consuming investigation of (suspected) misbehavior in more than one case. Jan
> On Mar 31, 2021, at 3:06 PM, Jan Beulich <jbeulich@suse.com> wrote: > > On 31.03.2021 16:00, George Dunlap wrote: >> >> >>> On Mar 31, 2021, at 2:54 PM, Jan Beulich <jbeulich@suse.com> wrote: >>> >>> On 31.03.2021 15:52, Ian Jackson wrote: >>>> George Dunlap writes ("Re: [PATCH] CHANGELOG.md: Make PV shim smaller by factoring out HVM-specific shadow code"): >>>>> I don’t understand why the two of you are downplaying your work so much. Yes, these are all only incremental improvements; but they are improvements; and the cumulative effect of loads of incremental improvements can be significant. Communicating to people just what the nature of all these incremental improvements are is important. >>>> >>>> I agree with George here. >>>> >>>> There ae a number of reasons why behind-the-scenes work with little >>>> (intentional) user-visible impact are useful to note in the >>>> CHANGELOG.md. With my Release Manager hat on I would like to see, for >>>> example, >>>> >>>>>> + - Factored out HVM-specific shadow code, allowing PV shim to be slimmer >>>> >>>> something about htis work in the CHANGELOG.md. >>>> >>>> IDK precisely, and I don't think George does either, what a good and >>>> accurate statement is. But I guess we will go with the text above if >>>> we don't get something better. >>> >>> At the very least the part after the comma ought to be deleted. As >>> said in an earlier reply, at least the shim default config disables >>> shadow code anyway, so the factoring out has no effect there. >> >> Thanks. So when you wrote the series, what was your motivation? Did you have a particular technical outcome in mind? Or did it just bother you that there was HVM-only code in a PV-only build? :-) > > What bothers me are more the implications - it being rather hard in > many cases, and in particular in shadow code, to be able to tell what > paths are involved in the handling of what kind(s) of guests. This > has made more time consuming investigation of (suspected) misbehavior > in more than one case. OK, so how about: - Factored out HVM-specific shadow code, improving code clarity and reducing the size of PV-only hypervisor builds -George
On 31.03.2021 16:30, George Dunlap wrote: > > >> On Mar 31, 2021, at 3:06 PM, Jan Beulich <jbeulich@suse.com> wrote: >> >> On 31.03.2021 16:00, George Dunlap wrote: >>> >>> >>>> On Mar 31, 2021, at 2:54 PM, Jan Beulich <jbeulich@suse.com> wrote: >>>> >>>> On 31.03.2021 15:52, Ian Jackson wrote: >>>>> George Dunlap writes ("Re: [PATCH] CHANGELOG.md: Make PV shim smaller by factoring out HVM-specific shadow code"): >>>>>> I don’t understand why the two of you are downplaying your work so much. Yes, these are all only incremental improvements; but they are improvements; and the cumulative effect of loads of incremental improvements can be significant. Communicating to people just what the nature of all these incremental improvements are is important. >>>>> >>>>> I agree with George here. >>>>> >>>>> There ae a number of reasons why behind-the-scenes work with little >>>>> (intentional) user-visible impact are useful to note in the >>>>> CHANGELOG.md. With my Release Manager hat on I would like to see, for >>>>> example, >>>>> >>>>>>> + - Factored out HVM-specific shadow code, allowing PV shim to be slimmer >>>>> >>>>> something about htis work in the CHANGELOG.md. >>>>> >>>>> IDK precisely, and I don't think George does either, what a good and >>>>> accurate statement is. But I guess we will go with the text above if >>>>> we don't get something better. >>>> >>>> At the very least the part after the comma ought to be deleted. As >>>> said in an earlier reply, at least the shim default config disables >>>> shadow code anyway, so the factoring out has no effect there. >>> >>> Thanks. So when you wrote the series, what was your motivation? Did you have a particular technical outcome in mind? Or did it just bother you that there was HVM-only code in a PV-only build? :-) >> >> What bothers me are more the implications - it being rather hard in >> many cases, and in particular in shadow code, to be able to tell what >> paths are involved in the handling of what kind(s) of guests. This >> has made more time consuming investigation of (suspected) misbehavior >> in more than one case. > > OK, so how about: > > - Factored out HVM-specific shadow code, improving code clarity and reducing the size of PV-only hypervisor builds This sounds okay to me. Thanks, Jan
diff --git a/CHANGELOG.md b/CHANGELOG.md index 15a22d6bde..49832ae017 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/) - x86_emulate: Expanded testing for several instruction classes - CI loop: Add Alpine Linux, Ubuntu Focal targets; drop CentOS 6 - CI loop: Add dom0less aarch64 smoke test + - Factored out HVM-specific shadow code, allowing PV shim to be slimmer ## Removed / support downgraded
Signed-off-by: George Dunlap <george.dunlap@citrix.com> --- Missed one from my list when creating the other series CC: Ian Jackson <ian.jackson@citrix.com> CC: Jan Beulich <jbeulich@suse.com> CC: Roger Pau Monne <roger.pau@citrix.com> --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+)