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

[George Dunlap <george.dunlap@eu.citrix.com>]

On 11/27/2013 03:46 PM, Jan Beulich wrote:
>>>> On 27.11.13 at 15:44, George Dunlap <george.dunlap@eu.citrix.com> 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