From mboxrd@z Thu Jan 1 00:00:00 1970 From: George Dunlap Subject: Re: [PATCH 2/2] use return value of domain_adjust_tot_pages() where feasible Date: Wed, 27 Nov 2013 16:51:40 +0000 Message-ID: <5296231C.2000408@eu.citrix.com> References: <5295B64E0200007800107636@nat28.tlf.novell.com> <5295BAD0020000780010765E@nat28.tlf.novell.com> <52960555.2040803@eu.citrix.com> <529621F00200007800107AF7@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta4.messagelabs.com ([85.158.143.247]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1VliL6-0003JV-7a for xen-devel@lists.xenproject.org; Wed, 27 Nov 2013 16:51:48 +0000 In-Reply-To: <529621F00200007800107AF7@nat28.tlf.novell.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Jan Beulich Cc: Andrew Cooper , TimDeegan , Keir Fraser , xen-devel List-Id: xen-devel@lists.xenproject.org On 11/27/2013 03:46 PM, Jan Beulich wrote: >>>> On 27.11.13 at 15:44, George Dunlap 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