Subject: Re: [Xen-devel] [PATCH] xen/balloon: don't online new memory
 initially
To: Igor Druzhinin <igor.druzhinin@citrix.com>,
        linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org
Cc: boris.ostrovsky@oracle.com
References: <20170703154037.17851-1-jgross@suse.com>
 <1eca2051-1765-d373-c462-03fe4ddf1054@citrix.com>
From: Juergen Gross <jgross@suse.com>
Message-ID: <4db6b7e1-1868-a516-0836-50cd08e9d4ab@suse.com>
Date: Tue, 4 Jul 2017 07:42:23 +0200
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.2.1
MIME-Version: 1.0
In-Reply-To: <1eca2051-1765-d373-c462-03fe4ddf1054@citrix.com>
Content-Type: text/plain; charset=utf-8
Content-Language: de-DE
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 4680
Lines: 137

On 03/07/17 20:44, Igor Druzhinin wrote:
> On 03/07/17 16:40, 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.
>>
>> 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 | 20 ++++++++++++--------
>>  include/xen/balloon.h     |  8 ++++++++
>>  3 files changed, 23 insertions(+), 8 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..66ec519c825c 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 unsigned 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,13 +103,8 @@ static struct notifier_block xenstore_notifier = {
>>  	.notifier_call = balloon_init_watcher,
>>  };
>>  
>> -static int __init balloon_init(void)
>> +void __init xen_balloon_init(void)
>>  {
>> -	if (!xen_domain())
>> -		return -ENODEV;
>> -
>> -	pr_info("Initialising balloon driver\n");
>> -
>>  	register_balloon(&balloon_dev);
>>  
>>  	register_xen_selfballooning(&balloon_dev);
>> @@ -109,7 +113,7 @@ static int __init balloon_init(void)
>>  
>>  	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
>>
> 
> We came across the same issue just recently. The problem was that for
> some kernel versions DMA buffers for emulated devices are allocated in
> this recently hotplugged area. This area is not properly described for
> QEMU so when a DMA request comes in QEMU treats it as "unassigned" and
> skips by default. This eventually leads to cryptic failures of system
> loading.
> 
> Internally we developed a workaround for QEMU with which we try to
> satisfy all the "unassigned" requests. But it doesn't solves the problem
> in a proper way IMHO.
> 
> I haven't not completely understood your use-case but we might try come
> up with a general solution for both of the problems because they are
> obviously related.
> 
>> Onlining new memory will lead to more problems e.g. undesired conflicts
>> with NVMe devices meant to be operated as block devices.
> 
> Could you explain this in more detail?

Please see

https://lists.xen.org/archives/html/xen-devel/2017-03/msg03020.html

for a more detailed discussion.


Juergen