linux-kernel - Re: [Xen-devel] [PATCH v4 2/2] xen_balloon: support memory auto onlining policy

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <87k2nd698k.fsf@vitty.brq.redhat.com>
Date:	Wed, 13 Jan 2016 11:53:47 +0100
From:	Vitaly Kuznetsov <vkuznets@...hat.com>
To:	David Vrabel <david.vrabel@...rix.com>
Cc:	<linux-mm@...ck.org>, Naoya Horiguchi <n-horiguchi@...jp.nec.com>,
	<linux-doc@...r.kernel.org>, Jonathan Corbet <corbet@....net>,
	Boris Ostrovsky <boris.ostrovsky@...cle.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Daniel Kiper <daniel.kiper@...cle.com>,
	"Kay Sievers" <kay@...y.org>, <linux-kernel@...r.kernel.org>,
	Tang Chen <tangchen@...fujitsu.com>,
	<xen-devel@...ts.xenproject.org>,
	Igor Mammedov <imammedo@...hat.com>,
	David Rientjes <rientjes@...gle.com>,
	Xishi Qiu <qiuxishi@...wei.com>,
	Dan Williams <dan.j.williams@...el.com>,
	"K. Y. Srinivasan" <kys@...rosoft.com>,
	"Mel Gorman" <mgorman@...hsingularity.net>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [Xen-devel] [PATCH v4 2/2] xen_balloon: support memory auto onlining policy

David Vrabel <david.vrabel@...rix.com> writes:

> On 12/01/16 16:56, Vitaly Kuznetsov wrote:
>> Add support for the newly added kernel memory auto onlining policy to Xen
>> ballon driver.
> [...]
>> --- a/drivers/xen/Kconfig
>> +++ b/drivers/xen/Kconfig
>> @@ -37,23 +37,29 @@ config XEN_BALLOON_MEMORY_HOTPLUG
>>  
>>  	  Memory could be hotplugged in following steps:
>>  
>> -	    1) dom0: xl mem-max <domU> <maxmem>
>> +	    1) domU: ensure that memory auto online policy is in effect by
>> +	       checking /sys/devices/system/memory/auto_online_blocks file
>> +	       (should be 'online').
>
> Step 1 applies to dom0 and domUs.
>

domU here (even before my patch) rather means 'the domain we're trying
to add memory to', not sure how to work it shorter. What about 'target
domain'?

>> --- a/drivers/xen/balloon.c
>> +++ b/drivers/xen/balloon.c
>> @@ -284,7 +284,7 @@ static void release_memory_resource(struct resource *resource)
>>  	kfree(resource);
>>  }
>>  
>> -static enum bp_state reserve_additional_memory(void)
>> +static enum bp_state reserve_additional_memory(bool online)
>>  {
>>  	long credit;
>>  	struct resource *resource;
>> @@ -338,7 +338,18 @@ static enum bp_state reserve_additional_memory(void)
>>  	}
>>  #endif
>>  
>> -	rc = add_memory_resource(nid, resource, false);
>> +	/*
>> +	 * add_memory_resource() will call online_pages() which in its turn
>> +	 * will call xen_online_page() callback causing deadlock if we don't
>> +	 * release balloon_mutex here. It is safe because there can only be
>> +	 * one balloon_process() running at a time and balloon_mutex is
>> +	 * internal to Xen driver, generic memory hotplug code doesn't mess
>> +	 * with it.
>
> There are multiple callers of reserve_additional_memory() and these are
> not all serialized via the balloon process.  Replace the "It is safe..."
> sentence with:
>
> "Unlocking here is safe because the callers drop the mutex before trying
> again."
>
>> +	 */
>> +	mutex_unlock(&balloon_mutex);
>> +	rc = add_memory_resource(nid, resource, online);
>
> This should always be memhp_auto_online, because...
>
>> @@ -562,14 +573,11 @@ static void balloon_process(struct work_struct *work)
>>  
>>  		credit = current_credit();
>>  
>> -		if (credit > 0) {
>> -			if (balloon_is_inflated())
>> -				state = increase_reservation(credit);
>> -			else
>> -				state = reserve_additional_memory();
>> -		}
>> -
>> -		if (credit < 0)
>> +		if (credit > 0 && balloon_is_inflated())
>> +			state = increase_reservation(credit);
>> +		else if (credit > 0)
>> +			state = reserve_additional_memory(memhp_auto_online);
>> +		else if (credit < 0)
>>  			state = decrease_reservation(-credit, GFP_BALLOON);
>
> I'd have preferred this refactored as:
>
> if (credit > 0) {
>     if (balloon_is_inflated())

That's what we had before and what caused the
'reserve_additional_memory' line to become > 80 chars after adding a
parameter. But as we'll be always calling add_memory_resource() with
'memhp_auto_online' the parameter is redundant and we can keep things as
they are.

>         ...
>     else
>         ...
> } else if (credit < 0) {
>     ...
> }

>>  
>>  		state = update_schedule(state);
>> @@ -599,7 +607,7 @@ static int add_ballooned_pages(int nr_pages)
>>  	enum bp_state st;
>>  
>>  	if (xen_hotplug_unpopulated) {
>> -		st = reserve_additional_memory();
>> +		st = reserve_additional_memory(false);
>
> ... we want to auto-online this memory as well.
>
>>  		if (st != BP_ECANCELED) {
>>  			mutex_unlock(&balloon_mutex);
>>  			wait_event(balloon_wq,
>> 

-- 
  Vitaly