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=-4.1 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,SPF_HELO_NONE,SPF_PASS 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 D29E2C433E3 for ; Fri, 17 Jul 2020 16:10:00 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id B063C2070E for ; Fri, 17 Jul 2020 16:10:00 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="rz3oI1vx" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726832AbgGQQJ7 (ORCPT ); Fri, 17 Jul 2020 12:09:59 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59566 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726256AbgGQQJ6 (ORCPT ); Fri, 17 Jul 2020 12:09:58 -0400 Received: from mail-io1-xd42.google.com (mail-io1-xd42.google.com [IPv6:2607:f8b0:4864:20::d42]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9FA9DC0619D2; Fri, 17 Jul 2020 09:09:58 -0700 (PDT) Received: by mail-io1-xd42.google.com with SMTP id l1so10962841ioh.5; Fri, 17 Jul 2020 09:09:58 -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=80duo2UCKGgh3D4vpF1VXWLmX4mSAl0+jky26IF2Kfc=; b=rz3oI1vxyD4wEFspA2IXpcN6RZgCG8ugcDQv4pjD++gEX0xWJzrl7/IvE0B5L4OiMN n3lSLCuMdLZg9h1a9XUA8isyooRlZIp0Oe7D9CgRkEQ0pNf8NqPD1dhRLxBVIonbLTuH rJLeVM/JLCYk8BU7XiOVFLi2U+GNwhp8V6yNnhOXc3KRHH8RRVm7Km3qi4oWqaNlreFc Nf4q/lt8EZ1JBoxQcRv4D75VlGItFhNde8Wi0JR7SvZ8DetJSH5t09MP7u3EKVvDFuVD 6jcccRbVO1SvOYnx0WLDxbbiA2hiNAhErX2ZWtnmy4PR+B/1O3fLfgwwJ/2Fk88FyQJ8 +Zrw== 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=80duo2UCKGgh3D4vpF1VXWLmX4mSAl0+jky26IF2Kfc=; b=iCvbYfnlqB3EmS6i6jUibmQuf0U/FkCOzi7rVLGik9BcIwWfOpq7D8oKftt1QZ9Coj bdsx9jd62rIK1tTngQDxo6foDJ464bZY9hMoRDr9M2TZBVg48v8K5pr1jh9LYdgpkMxS 9Y9eXwoqZSTKehBIkBgMsZJqh1oqckh18om4FbyjiV0C+chGKtFT5yZJwQkh5MMpwbGF TUaS3q/clhRSQRofJ5rHYkRVz/PidpFPW4J/QPlQqiXAV5f8pn4GlgqFVrBL7SbCcDMx t7UkYaI+GbhL+OW+BqcguyikWO4r2/VW+1z45t80Akaxe5zxPaHgEzajWylibvHyLPgr idMA== X-Gm-Message-State: AOAM533xWUFBbTxaCaARvtBd9xCnPQPRbJMZVD54pBoHTAOiCvPggToS UbsSh7g9Hd2XGkizO5FIdxxHtUNP+pyd4oK9g/s= X-Google-Smtp-Source: ABdhPJxAS5zKXGaRqMgoDBx8B5vS6pgJi7+88Y8nU6nWwsBaz2StwBp/+VMGafhOk/5PifpRQsVX2g3wWOuV+jYMgq8= X-Received: by 2002:a02:ce9a:: with SMTP id y26mr11930673jaq.121.1595002197726; Fri, 17 Jul 2020 09:09:57 -0700 (PDT) MIME-Version: 1.0 References: <1594429136-20002-1-git-send-email-alex.shi@linux.alibaba.com> <1594429136-20002-16-git-send-email-alex.shi@linux.alibaba.com> In-Reply-To: From: Alexander Duyck Date: Fri, 17 Jul 2020 09:09:46 -0700 Message-ID: Subject: Re: [PATCH v16 15/22] mm/compaction: do page isolation first in compaction To: Alex Shi Cc: Andrew Morton , Mel Gorman , Tejun Heo , Hugh Dickins , Konstantin Khlebnikov , Daniel Jordan , Yang Shi , Matthew Wilcox , Johannes Weiner , kbuild test robot , linux-mm , LKML , cgroups@vger.kernel.org, Shakeel Butt , Joonsoo Kim , Wei Yang , "Kirill A. Shutemov" Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Jul 16, 2020 at 10:10 PM Alex Shi wrote: > > > >> @@ -950,6 +951,21 @@ static bool too_many_isolated(pg_data_t *pgdat) > >> if (!(cc->gfp_mask & __GFP_FS) && page_mapping(page)) > >> goto isolate_fail; > >> > >> + /* > >> + * Be careful not to clear PageLRU until after we're > >> + * sure the page is not being freed elsewhere -- the > >> + * page release code relies on it. > >> + */ > >> + if (unlikely(!get_page_unless_zero(page))) > >> + goto isolate_fail; > >> + > >> + if (__isolate_lru_page_prepare(page, isolate_mode) != 0) > >> + goto isolate_fail_put; > >> + > >> + /* Try isolate the page */ > >> + if (!TestClearPageLRU(page)) > >> + goto isolate_fail_put; > >> + > >> /* If we already hold the lock, we can skip some rechecking */ > >> if (!locked) { > >> locked = compact_lock_irqsave(&pgdat->lru_lock, > > > > Why not do the __isolate_lru_page_prepare before getting the page? > > That way you can avoid performing an extra atomic operation on non-LRU > > pages. > > > > This change come from Hugh Dickins as mentioned from commit log: > >> trylock_page() is not safe to use at this time: its setting PG_locked > >> can race with the page being freed or allocated ("Bad page"), and can > >> also erase flags being set by one of those "sole owners" of a freshly > >> allocated page who use non-atomic __SetPageFlag(). > > Hi Hugh, > > would you like to show more details of the bug? > > ... > > >> + * sure the page is not being freed elsewhere -- the > >> + * page release code relies on it. > >> + */ > >> + if (unlikely(!get_page_unless_zero(page))) > >> + goto busy; > >> + > >> + if (!TestClearPageLRU(page)) { > >> + /* > >> + * This page may in other isolation path, > >> + * but we still hold lru_lock. > >> + */ > >> + put_page(page); > >> + goto busy; > >> + } > >> + > > > > I wonder if it wouldn't make sense to combine these two atomic ops > > with tests and the put_page into a single inline function? Then it > > could be possible to just do one check and if succeeds you do the > > block of code below, otherwise you just fall-through into the -EBUSY > > case. > > > > Uh, since get_page changes page->_refcount, TestClearPageLRU changes page->flags, > So I don't know how to combine them, could you make it more clear with code? Actually it is pretty straight forward. Something like this: static inline bool get_page_unless_zero_or_nonlru(struct page *page) { if (get_page_unless_zero(page)) { if (TestClearPageLRU(page)) return true; put_page(page); } return false; } You can then add comments as necessary. The general idea is you are having to do this in two different spots anyway so why not combine the logic? Although it does assume you can change the ordering of the other test above.