From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753244AbcHPIkQ (ORCPT ); Tue, 16 Aug 2016 04:40:16 -0400 Received: from mx1.redhat.com ([209.132.183.28]:52876 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753123AbcHPIkN (ORCPT ); Tue, 16 Aug 2016 04:40:13 -0400 From: Vitaly Kuznetsov To: "Alex Ng \(LIS\)" Cc: "devel\@linuxdriverproject.org" , "linux-kernel\@vger.kernel.org" , "KY Srinivasan" , Haiyang Zhang Subject: Re: [PATCH v2 RESEND 2/4] Drivers: hv: balloon: account for gaps in hot add regions References: <1470919670-23063-1-git-send-email-vkuznets@redhat.com> <1470919670-23063-3-git-send-email-vkuznets@redhat.com> Date: Tue, 16 Aug 2016 10:40:08 +0200 In-Reply-To: (Alex Ng's message of "Mon, 15 Aug 2016 22:17:34 +0000") Message-ID: <87bn0tayzb.fsf@vitty.brq.redhat.com> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.0.95 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.30]); Tue, 16 Aug 2016 08:40:11 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org "Alex Ng (LIS)" writes: >> @@ -676,35 +686,63 @@ static void hv_mem_hot_add(unsigned long start, >> unsigned long size, >> >> static void hv_online_page(struct page *pg) { >> - struct list_head *cur; >> struct hv_hotadd_state *has; >> + struct hv_hotadd_gap *gap; >> unsigned long cur_start_pgp; >> unsigned long cur_end_pgp; >> + bool is_gap = false; >> >> list_for_each(cur, &dm_device.ha_region_list) { >> has = list_entry(cur, struct hv_hotadd_state, list); >> cur_start_pgp = (unsigned long) >> + pfn_to_page(has->start_pfn); >> + cur_end_pgp = (unsigned long)pfn_to_page(has->end_pfn); >> + >> + /* The page belongs to a different HAS. */ >> + if (((unsigned long)pg < cur_start_pgp) || >> + ((unsigned long)pg >= cur_end_pgp)) >> + continue; >> + >> + cur_start_pgp = (unsigned long) >> pfn_to_page(has->covered_start_pfn); >> cur_end_pgp = (unsigned long)pfn_to_page(has- >> >covered_end_pfn); >> >> - if (((unsigned long)pg >= cur_start_pgp) && >> - ((unsigned long)pg < cur_end_pgp)) { >> - /* >> - * This frame is currently backed; online the >> - * page. >> - */ >> - __online_page_set_limits(pg); >> - __online_page_increment_counters(pg); >> - __online_page_free(pg); >> + /* The page is not backed. */ >> + if (((unsigned long)pg < cur_start_pgp) || >> + ((unsigned long)pg >= cur_end_pgp)) >> + continue; >> + >> + /* Check for gaps. */ >> + list_for_each_entry(gap, &has->gap_list, list) { >> + cur_start_pgp = (unsigned long) >> + pfn_to_page(gap->start_pfn); >> + cur_end_pgp = (unsigned long) >> + pfn_to_page(gap->end_pfn); >> + if (((unsigned long)pg >= cur_start_pgp) && >> + ((unsigned long)pg < cur_end_pgp)) { >> + is_gap = true; >> + break; >> + } >> } >> + >> + if (is_gap) >> + break; >> + >> + /* This frame is currently backed; online the page. */ >> + __online_page_set_limits(pg); >> + __online_page_increment_counters(pg); >> + __online_page_free(pg); >> + break; >> } >> } >> > > We may need to add similar logic to check for gaps in hv_bring_pgs_online(). > > [...] Yes, probably, I'll take a look and try to refactor the onlinig code in a separate function to avoid duplication. >> static unsigned long handle_pg_range(unsigned long pg_start, @@ -834,13 >> +881,19 @@ static unsigned long process_hot_add(unsigned long pg_start, >> unsigned long rg_size) >> { >> struct hv_hotadd_state *ha_region = NULL; >> + int covered; >> >> if (pfn_cnt == 0) >> return 0; >> >> - if (!dm_device.host_specified_ha_region) >> - if (pfn_covered(pg_start, pfn_cnt)) >> + if (!dm_device.host_specified_ha_region) { >> + covered = pfn_covered(pg_start, pfn_cnt); >> + if (covered < 0) >> + return 0; > > If the hot-add pages aren't covered by any region, then shouldn't it fall through instead of returning? > That way the new ha_region can be added to the list and we hot-add the > pages accordingly. I was under an impression this is impossible: hot_add_req()/process_hot_add() will create a new region in this case. 'covered < 0' was added to handle one particular error: failure to allocate memory to record gap (struct hv_hotadd_gap) and I don't have a better idea how to handle this: if we can't remember the gap we'll crash later on onlining... -- Vitaly