From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Kiper Subject: Re: [PATCHv2 06/10] xen/balloon: only hotplug additional memory if required Date: Wed, 29 Jul 2015 17:55:35 +0200 Message-ID: <20150729155535.GL3492__48089.1154766597$1438185499$gmane$org@olila.local.net-space.pl> References: <1437738468-24110-1-git-send-email-david.vrabel@citrix.com> <1437738468-24110-7-git-send-email-david.vrabel@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1ZKTil-0001gp-9k for xen-devel@lists.xenproject.org; Wed, 29 Jul 2015 15:56:43 +0000 Content-Disposition: inline In-Reply-To: <1437738468-24110-7-git-send-email-david.vrabel@citrix.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: David Vrabel Cc: linux-mm@kvack.org, xen-devel@lists.xenproject.org, Boris Ostrovsky , linux-kernel@vger.kernel.org List-Id: xen-devel@lists.xenproject.org On Fri, Jul 24, 2015 at 12:47:44PM +0100, David Vrabel wrote: > Now that we track the total number of pages (included hotplugged > regions), it is easy to determine if more memory needs to be > hotplugged. > > Add a new BP_WAIT state to signal that the balloon process needs to > wait until kicked by the memory add notifier (when the new section is > onlined by userspace). > > Signed-off-by: David Vrabel > --- > v2: > - New BP_WAIT status after adding new memory sections. > --- > drivers/xen/balloon.c | 23 +++++++++++++++++++---- > 1 file changed, 19 insertions(+), 4 deletions(-) > > diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c > index b5037b1..ced34cd 100644 > --- a/drivers/xen/balloon.c > +++ b/drivers/xen/balloon.c > @@ -75,12 +75,14 @@ > * balloon_process() state: > * > * BP_DONE: done or nothing to do, > + * BP_WAIT: wait to be rescheduled, BP_SLEEP? BP_WAIT suggests that balloon process waits for something in a loop. > * BP_EAGAIN: error, go to sleep, > * BP_ECANCELED: error, balloon operation canceled. > */ > > enum bp_state { > BP_DONE, > + BP_WAIT, > BP_EAGAIN, > BP_ECANCELED > }; > @@ -167,6 +169,9 @@ static struct page *balloon_next_page(struct page *page) > > static enum bp_state update_schedule(enum bp_state state) > { > + if (state == BP_WAIT) > + return BP_WAIT; > + > if (state == BP_ECANCELED) > return BP_ECANCELED; > > @@ -242,12 +247,22 @@ static void release_memory_resource(struct resource *resource) > * bit set). Real size of added memory is established at page onlining stage. > */ Please align above, partially visible, comment to current reality. > -static enum bp_state reserve_additional_memory(long credit) > +static enum bp_state reserve_additional_memory(void) > { > + long credit; > struct resource *resource; > int nid, rc; > unsigned long balloon_hotplug; > > + credit = balloon_stats.target_pages - balloon_stats.total_pages; > + > + /* > + * Already hotplugged enough pages? Wait for them to be > + * onlined. > + */ Please change this comment for something like that: Already hotplugged enough pages? If yes then go to sleep. > + if (credit <= 0) > + return BP_EAGAIN; No, this should be BP_WAIT (BP_SLEEP). Otherwise when somebody touches balloon_stats.target_pages balloon process will be rescheduled unnecessarily until pages are onlined up to balloon_stats.total_pages. We do not want that. > + > balloon_hotplug = round_up(credit, PAGES_PER_SECTION); > > resource = additional_memory_resource(balloon_hotplug * PAGE_SIZE); > @@ -287,7 +302,7 @@ static enum bp_state reserve_additional_memory(long credit) > > balloon_stats.total_pages += balloon_hotplug; > > - return BP_DONE; > + return BP_WAIT; > err: Please add one empty line before err label. Daniel