Subject: Re: [PATCH v3] xen/balloon: don't online new memory initially
To: Juergen Gross <jgross@suse.com>, linux-kernel@vger.kernel.org,
        xen-devel@lists.xenproject.org
References: <20170710081045.874-1-jgross@suse.com>
 <3d195aea-55b7-c412-afbf-534fd143e578@oracle.com>
 <4e4bcc27-f0c2-7aa6-ea4c-bcc1176b6052@suse.com>
From: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Message-ID: <3c80810e-adef-e8f3-ed09-c02f5d9a0324@oracle.com>
Date: Tue, 18 Jul 2017 12:33:45 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.8.0
MIME-Version: 1.0
In-Reply-To: <4e4bcc27-f0c2-7aa6-ea4c-bcc1176b6052@suse.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4282
Lines: 129

On 07/18/2017 12:12 PM, Juergen Gross wrote:
> On 18/07/17 18:08, Boris Ostrovsky wrote:
>> On 07/10/2017 04:10 AM, Juergen Gross wrote:
>>> When setting up the Xenstore watch for the memory target size the new
>>> watch will fire at once. Don't try to reach the configured target size
>>> by onlining new memory in this case, as the current memory size will
>>> be smaller in almost all cases due to e.g. BIOS reserved pages.
>>>
>>> Onlining new memory will lead to more problems e.g. undesired conflicts
>>> with NVMe devices meant to be operated as block devices.
>>>
>>> Instead remember the difference between target size and current size
>>> when the watch fires for the first time and apply it to any further
>>> size changes, too.
>> I don't think I understand how deferring setting the target until next
>> watch firing will help with the original problem. Could you explain?
> The difference of target and current size will be evaluated at the
> time the watch fires for the first time. This difference is subtracted
> from the target on later watch events.

OK, I clearly completely missed the point here.

Reviewed-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>


>
>
> Juergen
>
>> Thanks.
>> -boris
>>
>>
>>> In order to avoid races between balloon.c and xen-balloon.c init calls
>>> do the xen-balloon.c initialization from balloon.c.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>>  drivers/xen/balloon.c     |  3 +++
>>>  drivers/xen/xen-balloon.c | 22 ++++++++++++----------
>>>  include/xen/balloon.h     |  8 ++++++++
>>>  3 files changed, 23 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/xen/balloon.c b/drivers/xen/balloon.c
>>> index 50dcb68d8070..ab609255a0f3 100644
>>> --- a/drivers/xen/balloon.c
>>> +++ b/drivers/xen/balloon.c
>>> @@ -780,6 +780,9 @@ static int __init balloon_init(void)
>>>  	}
>>>  #endif
>>>  
>>> +	/* Init the xen-balloon driver. */
>>> +	xen_balloon_init();
>>> +
>>>  	return 0;
>>>  }
>>>  subsys_initcall(balloon_init);
>>> diff --git a/drivers/xen/xen-balloon.c b/drivers/xen/xen-balloon.c
>>> index e7715cb62eef..e89136ab851e 100644
>>> --- a/drivers/xen/xen-balloon.c
>>> +++ b/drivers/xen/xen-balloon.c
>>> @@ -59,6 +59,8 @@ static void watch_target(struct xenbus_watch *watch,
>>>  {
>>>  	unsigned long long new_target;
>>>  	int err;
>>> +	static bool watch_fired;
>>> +	static long target_diff;
>>>  
>>>  	err = xenbus_scanf(XBT_NIL, "memory", "target", "%llu", &new_target);
>>>  	if (err != 1) {
>>> @@ -69,7 +71,14 @@ static void watch_target(struct xenbus_watch *watch,
>>>  	/* The given memory/target value is in KiB, so it needs converting to
>>>  	 * pages. PAGE_SHIFT converts bytes to pages, hence PAGE_SHIFT - 10.
>>>  	 */
>>> -	balloon_set_new_target(new_target >> (PAGE_SHIFT - 10));
>>> +	new_target >>= PAGE_SHIFT - 10;
>>> +	if (watch_fired) {
>>> +		balloon_set_new_target(new_target - target_diff);
>>> +		return;
>>> +	}
>>> +
>>> +	watch_fired = true;
>>> +	target_diff = new_target - balloon_stats.target_pages;
>>>  }
>>>  static struct xenbus_watch target_watch = {
>>>  	.node = "memory/target",
>>> @@ -94,22 +103,15 @@ static struct notifier_block xenstore_notifier = {
>>>  	.notifier_call = balloon_init_watcher,
>>>  };
>>>  
>>> -static int __init balloon_init(void)
>>> +void xen_balloon_init(void)
>>>  {
>>> -	if (!xen_domain())
>>> -		return -ENODEV;
>>> -
>>> -	pr_info("Initialising balloon driver\n");
>>> -
>>>  	register_balloon(&balloon_dev);
>>>  
>>>  	register_xen_selfballooning(&balloon_dev);
>>>  
>>>  	register_xenstore_notifier(&xenstore_notifier);
>>> -
>>> -	return 0;
>>>  }
>>> -subsys_initcall(balloon_init);
>>> +EXPORT_SYMBOL_GPL(xen_balloon_init);
>>>  
>>>  #define BALLOON_SHOW(name, format, args...)				\
>>>  	static ssize_t show_##name(struct device *dev,			\
>>> diff --git a/include/xen/balloon.h b/include/xen/balloon.h
>>> index d1767dfb0d95..8906361bb50c 100644
>>> --- a/include/xen/balloon.h
>>> +++ b/include/xen/balloon.h
>>> @@ -35,3 +35,11 @@ static inline int register_xen_selfballooning(struct device *dev)
>>>  	return -ENOSYS;
>>>  }
>>>  #endif
>>> +
>>> +#ifdef CONFIG_XEN_BALLOON
>>> +void xen_balloon_init(void);
>>> +#else
>>> +static inline void xen_balloon_init(void)
>>> +{
>>> +}
>>> +#endif
>>