From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757308AbcAMKx6 (ORCPT ); Wed, 13 Jan 2016 05:53:58 -0500 Received: from mx1.redhat.com ([209.132.183.28]:60573 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1757029AbcAMKxy (ORCPT ); Wed, 13 Jan 2016 05:53:54 -0500 From: Vitaly Kuznetsov To: David Vrabel Cc: , Naoya Horiguchi , , Jonathan Corbet , Boris Ostrovsky , Greg Kroah-Hartman , Daniel Kiper , "Kay Sievers" , , Tang Chen , , Igor Mammedov , David Rientjes , Xishi Qiu , Dan Williams , "K. Y. Srinivasan" , "Mel Gorman" , Andrew Morton Subject: Re: [Xen-devel] [PATCH v4 2/2] xen_balloon: support memory auto onlining policy References: <1452617777-10598-1-git-send-email-vkuznets@redhat.com> <1452617777-10598-3-git-send-email-vkuznets@redhat.com> <56953A18.2070407@citrix.com> Date: Wed, 13 Jan 2016 11:53:47 +0100 In-Reply-To: <56953A18.2070407@citrix.com> (David Vrabel's message of "Tue, 12 Jan 2016 17:38:32 +0000") Message-ID: <87k2nd698k.fsf@vitty.brq.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.5 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org David Vrabel 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 >> + 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