[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

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
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

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.


Xen-devel mailing list



Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.