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 26586C433E4 for ; Fri, 17 Jul 2020 16:10:10 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id B5CD32070E for ; Fri, 17 Jul 2020 16:10:09 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="rz3oI1vx" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org B5CD32070E 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 152CA6B00A4; Fri, 17 Jul 2020 12:10:09 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 0DBD76B00A5; Fri, 17 Jul 2020 12:10:09 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id F0E318D0003; Fri, 17 Jul 2020 12:10:08 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0052.hostedemail.com [216.40.44.52]) by kanga.kvack.org (Postfix) with ESMTP id D930B6B00A4 for ; Fri, 17 Jul 2020 12:10:08 -0400 (EDT) Received: from smtpin21.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 719AE1AAE6 for ; Fri, 17 Jul 2020 16:10:08 +0000 (UTC) X-FDA: 77048054496.21.waste46_001175f26f0c Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin21.hostedemail.com (Postfix) with ESMTP id 1245E18043DB5 for ; Fri, 17 Jul 2020 16:09:59 +0000 (UTC) X-HE-Tag: waste46_001175f26f0c X-Filterd-Recvd-Size: 6928 Received: from mail-io1-f68.google.com (mail-io1-f68.google.com [209.85.166.68]) by imf50.hostedemail.com (Postfix) with ESMTP for ; Fri, 17 Jul 2020 16:09:58 +0000 (UTC) Received: by mail-io1-f68.google.com with SMTP id k23so10950269iom.10 for ; 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=L3Yvyp1x5yB4Yd4V+jlR5ttvyjjmWuIqve4vmAIbZdDcVVrNyNH8kKYcx2hTgRLV1p 48CIrAXXKEm3j/4odNm5AySYeQXj4pfzAsJ0aAh0Q77O3jj68Z7GwxUrKDtP97Y4H8VS lOvsUEAlW1Rz/vF0lawRF9M/JQhfhcaxYloOqIeqt/BtX5F1fTnhp05CJAjvP7eCnJVY aIGE+v+wDB3ZhAFtbLqnPdCTFRY1nRr8RjiUrrsBm/VZfP4+n7WLPbmzqH5cGLSAt9/E eQJZ3uWAvxvXB67qBwyWFAUJYVfv6+1wXHe3ph55UEAYo5uUA+im3kxgCP0aACTw63hY D0IA== X-Gm-Message-State: AOAM530pCSvvCnOyUZK0RcpesL9gLl+cNGd7fqzs9ndOprne6LMuWUJh OpqVAO+XWaEjZ0m6HmjnswLahl3aqpJOXRriFLQ= 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" X-Rspamd-Queue-Id: 1245E18043DB5 X-Spamd-Result: default: False [0.00 / 100.00] X-Rspamd-Server: rspam05 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 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.