From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.6 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SIGNED_OFF_BY,SPF_HELO_NONE, SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 78C4EC388F9 for ; Tue, 27 Oct 2020 16:54:06 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id E882E20727 for ; Tue, 27 Oct 2020 16:54:05 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="k2HZ+kc9" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E882E20727 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 5EBF06B0070; Tue, 27 Oct 2020 12:54:05 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 59A8E6B0071; Tue, 27 Oct 2020 12:54:05 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 4625F6B0072; Tue, 27 Oct 2020 12:54:05 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0128.hostedemail.com [216.40.44.128]) by kanga.kvack.org (Postfix) with ESMTP id 159AA6B0070 for ; Tue, 27 Oct 2020 12:54:05 -0400 (EDT) Received: from smtpin26.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay01.hostedemail.com (Postfix) with ESMTP id 99477180AD801 for ; Tue, 27 Oct 2020 16:54:04 +0000 (UTC) X-FDA: 77418302808.26.unit30_0a043e72727d Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin26.hostedemail.com (Postfix) with ESMTP id 75E541804B65A for ; Tue, 27 Oct 2020 16:54:04 +0000 (UTC) X-HE-Tag: unit30_0a043e72727d X-Filterd-Recvd-Size: 7802 Received: from mail-ed1-f68.google.com (mail-ed1-f68.google.com [209.85.208.68]) by imf38.hostedemail.com (Postfix) with ESMTP for ; Tue, 27 Oct 2020 16:54:04 +0000 (UTC) Received: by mail-ed1-f68.google.com with SMTP id k9so59292edo.5 for ; Tue, 27 Oct 2020 09:54:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=kxOgqqlVfNE/0giJq5nhZZ7jCo399QUJ06fkJloHJeU=; b=k2HZ+kc9PzKK6W3Q8QLsthL3YlV+AV9D8RENdY4AZChhhm+yJvNIQzFS0WhpKvDA+x IdWOym9pXG4m+38XI2TpJg03kjaEdd5rKYCQa9vUzuegLUa7CjVhgnlbhFDgZSmrt/3d H112xEky60IFF5zf0hjWmdp8Khw54v7p0y5UJ0q9daG4LUeekjWrYnldkhxQX78v/8nI DiPC6gI25gBa8Cu+81oAtWIDaM9NrT2ayTpOoxBAd/BbNjwADzJ+fWfujAvvW3PM6zcR 3N3godD2t9yjxnieR79nHyKt8SY1+rQKvK+Vr2oCEwKMY1CA7PJG9V3mmFOTA50Zignd i/jA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=kxOgqqlVfNE/0giJq5nhZZ7jCo399QUJ06fkJloHJeU=; b=f8Im6Z20Iy78+g27eDttcfaF/C+f2aLCEXJe42gYaytKwbfp9dlWRkngcTRXiQqVac hIWcb4ZePqSYsSg0y47cha+nAXj+XYYM9uPDGX5HXXTKJyZDPzyh/yIcVaO3jacluZ8I axzdAsh+TvIs3CIcP6/VhOSLHl6UTjF2n5sXwdtNGI0nz5Pt2jr07l/NZUID71vUKeze CqVtpSSJJeevp27mg1ca8JzNF82ixuMiqYXbWTsOd+EN52vWZPK2anURuH52uhPmnGkG 5S2VJ1zFyoCNXv6YYg0KtVC4XLwwqPrIoieYViP1qkcmTt71aUVEmKFAEPOhlwgE7qXX HFXQ== X-Gm-Message-State: AOAM5338GeG3tKEylLnP7PBHlRtu5WNNiMxEoBoovvNcPWqcl2TxcMYX bS1EGkGNWAZb1hGeb69TMtufrZKJqcQ6o9aMX0MRqTEIH5vBEQ== X-Google-Smtp-Source: ABdhPJzrxrMLAjgdC89YGfmF6JnIRNQrbMVaEXJ+3LUAvmaMnd6q2mfmEAZvbtsaMw5/IwHsNX7mn95NFYkX0ebdkhk= X-Received: by 2002:a05:6402:6d8:: with SMTP id n24mr3251195edy.168.1603817642883; Tue, 27 Oct 2020 09:54:02 -0700 (PDT) MIME-Version: 1.0 References: <20201007161736.ACC6E387@viggo.jf.intel.com> <20201007161745.26B1D789@viggo.jf.intel.com> <20201027152858.GA11135@linux> In-Reply-To: <20201027152858.GA11135@linux> From: Yang Shi Date: Tue, 27 Oct 2020 09:53:50 -0700 Message-ID: Subject: Re: [RFC][PATCH 5/9] mm/migrate: demote pages during reclaim To: Oscar Salvador Cc: Dave Hansen , Linux Kernel Mailing List , Linux MM , Yang Shi , David Rientjes , Huang Ying , Dan Williams Content-Type: text/plain; charset="UTF-8" X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Tue, Oct 27, 2020 at 8:29 AM Oscar Salvador wrote: > > On Wed, Oct 07, 2020 at 09:17:45AM -0700, Dave Hansen wrote: > > Signed-off-by: Dave Hansen > > Cc: Yang Shi > > Cc: David Rientjes > > Cc: Huang Ying > > Cc: Dan Williams > > I am still going through all the details, but just my thoughts on things > that caught my eye: > > > --- a/include/linux/migrate.h~demote-with-migrate_pages 2020-10-07 09:15:31.028642442 -0700 > > +++ b/include/linux/migrate.h 2020-10-07 09:15:31.034642442 -0700 > > @@ -27,6 +27,7 @@ enum migrate_reason { > > MR_MEMPOLICY_MBIND, > > MR_NUMA_MISPLACED, > > MR_CONTIG_RANGE, > > + MR_DEMOTION, > > MR_TYPES > > I think you also need to add it under include/trace/events/migrate.h, so > mm_migrate_pages event can know about it. Agree. > > > +bool migrate_demote_page_ok(struct page *page, struct scan_control *sc) > > Make it static? > Also, scan_control seems to be unused here. > > > +{ > > + int next_nid = next_demotion_node(page_to_nid(page)); > > + > > + VM_BUG_ON_PAGE(!PageLocked(page), page); > > Right after the call to migrate_demote_page_ok, we call unlock_page > which already has this check in place. > I know that this is only to be on the safe side and we do not loss anything, > but just my thoughts. > > > +static struct page *alloc_demote_page(struct page *page, unsigned long node) > > +{ > > + /* > > + * Try to fail quickly if memory on the target node is not > > + * available. Leaving out __GFP_IO and __GFP_FS helps with > > + * this. If the desintation node is full, we want kswapd to > > + * run there so that its pages will get reclaimed and future > > + * migration attempts may succeed. > > + */ > > + gfp_t flags = (__GFP_HIGHMEM | __GFP_MOVABLE | __GFP_NORETRY | > > + __GFP_NOMEMALLOC | __GFP_NOWARN | __GFP_THISNODE | > > + __GFP_KSWAPD_RECLAIM); > > I think it would be nicer to have this as a real GFP_ thingy defined. > e.g: GFP_DEMOTION > > > + /* HugeTLB pages should not be on the LRU */ > > + WARN_ON_ONCE(PageHuge(page)); > > I am not sure about this one. > This could only happen if the page, which now it is in another list, ends up in > the buddy system. That is quite unlikely bth. > And nevertheless, this is only a warning, which means that if this scenario gets > to happen, we will be allocating a single page to satisfy a higher-order page, and > I am not sure about the situation we will end up with. IMHO, we should use BUG_ON instead of WARN_ON or we should just back off if we see hugetlb page in this path and print out some warning. > > > + > > + if (PageTransHuge(page)) { > > + struct page *thp; > > + > > + flags |= __GFP_COMP; > > + > > + thp = alloc_pages_node(node, flags, HPAGE_PMD_ORDER); > > + if (!thp) > > + return NULL; > > + prep_transhuge_page(thp); > > + return thp; > > + } > > + > > + return __alloc_pages_node(node, flags, 0); > > Would make sense to transform this in some sort of new_demotion_page, > which actually calls alloc_migration_target with the right stuff in place? > And then pass a struct migration_target_control so alloc_migration_target > does the right thing. > alloc_migration_target also takes care of calling prep_transhuge_page > when needed. > e.g: > > static struct page *new_demotion_node(struct page *page, unsigned long private) > { > struct migration_target_control mtc = { > .nid = private, > .gfp_mask = GFP_DEMOTION, > }; > > if (PageTransHuge(page)) > mtc.gfp_mask |= __GFP_COMP; > > return alloc_migration_target(page, (unsigned long)&mtc); > } > > The only thing I see is that alloc_migration_target seems to "override" > the gfp_mask and does ORs GFP_TRANSHUGE for THP pages, which includes > __GFP_DIRECT_RECLAIM (not appreciated in this case). > But maybe this can be worked around by checking if gfp_mask == GFP_DEMOTION, > and if so, just keep the mask as it is. Makes sense to me. > > > + > > + if (list_empty(demote_pages)) > > + return 0; > > + > > + /* Demotion ignores all cpuset and mempolicy settings */ > > + err = migrate_pages(demote_pages, alloc_demote_page, NULL, > > + target_nid, MIGRATE_ASYNC, MR_DEMOTION, > > + &nr_succeeded); > > As I said, instead of alloc_demote_page, use a new_demote_page and make > alloc_migration_target handle the allocations and prep thp pages. > > > -- > Oscar Salvador > SUSE L3 >