Re: [Xen-devel] [PATCH 2/2] use return value of domain_adjust_tot_pages() where feasible

[George Dunlap <george.dunlap@xxxxxxxxxxxxx>]

On 11/27/2013 03:46 PM, Jan Beulich wrote:
> > > > On 27.11.13 at 15:44, George Dunlap <george.dunlap@xxxxxxxxxxxxx> wrote:
> > On 11/27/2013 08:26 AM, Jan Beulich wrote:
> > > This is generally cheaper than re-reading ->tot_pages.
> > >
> > > While doing so I also noticed an improper use (lacking error handling)
> > > of get_domain() as well as lacks of ->is_dying checks in the memory
> > > sharing code, which the patch fixes at once. In the course of doing
> > > this I further noticed other error paths there pointlessly calling
> > > put_page() et al with ->page_alloc_lock still held, which is also being
> > > reversed.
> > Thus hiding two very important changes underneath a summary that looks
> > completely unimportant.
> >
> > If this patch only did as the title suggests, I would question whether
> > it should be included for 4.4, since it seems to have little benefit.
> > Are the other two changes bug fixes?
> I'm sure the missing ->is_dying check is one. I'm not sure the
> put vs unlock ordering is just inefficient or could actively cause
> issues.
>
> > In any case, the summary should indicate to someone just browsing
> > through what important changes might be inside.
> The issue is that you can't really put all three things in the title.
> And hence I decided to keep the title what it was supposed to be
> when I started correcting this code.
I think I would call it something like, "Various fix-ups to mm-related 
code".  That would allow anyone scanning it to know that there were a 
number of fix-ups, and they were in the MM code; and would prompt them 
to look further if it seemed like something they might be looking for 
(either fixes to backport, or the source of a bug that was introduced).
> With - in my understanding - the memory sharing code still being
> experimental, it also didn't really seem worthwhile splitting these
> changes out into separate patches (I generally try to do so when
> spotting isolated issues, but tend to keep things together when
> in the course of doing other adjustments I recognize further small
> issues in code that's not production ready anyway, i.e. not
> needing to be backported, and hence the title not needing to hint
> at that).
>
> In any event, as to the freeze: The code was written and would
> have been submitted before the code freeze if I hadn't been
> required to hold it back until after XSA-74 went public (which
> goes back to our [unwritten?] policy of not publishing changes
> that were found necessary/desirable in the course of researching
> a specific security issue).
Unfortunately, the "unfairness" of having been held back for a security 
embargo (or in the dom0 PVH case, having been submitted a year ago) 
doesn't change the realities of the situation: that making changes risks 
introducing bugs which delay the release, or worse, are not found until 
after the release.  That may be a reason to consider it "having been 
submitted before the feature freeze", but it can't tilt the scales of a 
cost/benefits analysis.
Anyway, we're only half-way through the code freeze, and these do look 
like bug fixes; I'm inclined to say they're OK.
 -George

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel