Date: Wed, 29 Jul 2015 17:55:35 +0200
From: Daniel Kiper <daniel.kiper@oracle.com>
To: David Vrabel <david.vrabel@citrix.com>
Cc: xen-devel@lists.xenproject.org,
        Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>,
        Boris Ostrovsky <boris.ostrovsky@oracle.com>,
        linux-kernel@vger.kernel.org, linux-mm@kvack.org
Subject: Re: [PATCHv2 06/10] xen/balloon: only hotplug additional memory if
 required
Message-ID: <20150729155535.GL3492@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-Disposition: inline
In-Reply-To: <1437738468-24110-7-git-send-email-david.vrabel@citrix.com>
User-Agent: Mutt/1.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3068
Lines: 102

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 <david.vrabel@citrix.com>
> ---
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/