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=-2.0 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, USER_AGENT_SANE_1 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 A8EAFC43331 for ; Wed, 13 Nov 2019 13:45:07 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 7F0CE22459 for ; Wed, 13 Nov 2019 13:45:07 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=infradead.org header.i=@infradead.org header.b="eLUpbLpu" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727486AbfKMNpG (ORCPT ); Wed, 13 Nov 2019 08:45:06 -0500 Received: from bombadil.infradead.org ([198.137.202.133]:51770 "EHLO bombadil.infradead.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726010AbfKMNpG (ORCPT ); Wed, 13 Nov 2019 08:45:06 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=infradead.org; s=bombadil.20170209; h=In-Reply-To:Content-Transfer-Encoding :Content-Type:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date: Sender:Reply-To:Content-ID:Content-Description:Resent-Date:Resent-From: Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Id:List-Help: List-Unsubscribe:List-Subscribe:List-Post:List-Owner:List-Archive; bh=zRi6DLbBkEdEozkjZm98oVK/x7aADkM+iJVk1EnTFWk=; b=eLUpbLpu+7Mqv1XqZbOAstW1Xr s3S9M4qabuNrKeHjr+aMtQQTUqyJI+s3ZAcWxGGHoOLFIGkzrOpj7FV2i53sWElgDVbC8aR6ahCw4 RBdV5TnpKLElNuiyfEOEHdptZ1Paw4v6/YHhRda6W3KlU4ay0toajZfXkccooGszCUXu/yLxgAaC/ pFK9jKjJHT0Yqv57ZjYInT11Yq0q2yNowES80QxjYTVAK0ETrYGnpDUU1t5IkbFEWy6JC/g55VRZ8 Y+YCQRbkdtBuG2DTLd8Fj+26ap7yNVh1HvvGzY2S79eh+sc42JHFGRS0kewhOnmDsZICEEYfZx3N+ TuByAZvw==; Received: from willy by bombadil.infradead.org with local (Exim 4.92.3 #3 (Red Hat Linux)) id 1iUsxC-0003wY-TT; Wed, 13 Nov 2019 13:45:02 +0000 Date: Wed, 13 Nov 2019 05:45:02 -0800 From: Matthew Wilcox To: Alex Shi Cc: cgroups@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, akpm@linux-foundation.org, mgorman@techsingularity.net, tj@kernel.org, hughd@google.com, khlebnikov@yandex-team.ru, daniel.m.jordan@oracle.com, yang.shi@linux.alibaba.com, Johannes Weiner , Michal Hocko , Vladimir Davydov , Roman Gushchin , Shakeel Butt , Chris Down , Thomas Gleixner , Vlastimil Babka , Andrey Ryabinin , swkhack , "Potyra, Stefan" , Jason Gunthorpe , Mauro Carvalho Chehab , Peng Fan , Nikolay Borisov , Ira Weiny , Kirill Tkhai , Yafang Shao Subject: Re: [PATCH v2 4/8] mm/lru: only change the lru_lock iff page's lruvec is different Message-ID: <20191113134502.GD7934@bombadil.infradead.org> References: <1573567588-47048-1-git-send-email-alex.shi@linux.alibaba.com> <1573567588-47048-5-git-send-email-alex.shi@linux.alibaba.com> <20191112143624.GA7934@bombadil.infradead.org> <297ad71c-081c-f7e1-d640-8720a0eeeeba@linux.alibaba.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <297ad71c-081c-f7e1-d640-8720a0eeeeba@linux.alibaba.com> User-Agent: Mutt/1.12.1 (2019-06-15) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, Nov 13, 2019 at 10:26:24AM +0800, Alex Shi wrote: > 在 2019/11/12 下午10:36, Matthew Wilcox 写道: > > On Tue, Nov 12, 2019 at 10:06:24PM +0800, Alex Shi wrote: > >> +/* Don't lock again iff page's lruvec locked */ > >> +static inline struct lruvec *relock_page_lruvec_irq(struct page *page, > >> + struct lruvec *locked_lruvec) > >> +{ > >> + struct pglist_data *pgdat = page_pgdat(page); > >> + struct lruvec *lruvec; > >> + > >> + rcu_read_lock(); > >> + lruvec = mem_cgroup_page_lruvec(page, pgdat); > >> + > >> + if (locked_lruvec == lruvec) { > >> + rcu_read_unlock(); > >> + return lruvec; > >> + } > >> + rcu_read_unlock(); > > > > Why not simply: > > > > rcu_read_lock(); > > lruvec = mem_cgroup_page_lruvec(page, pgdat); > > rcu_read_unlock(); > > > > if (locked_lruvec == lruvec) > > The rcu_read_unlock here is for guarding the locked_lruvec/lruvec comparsion. > Otherwise memcg/lruvec maybe changed, like, from memcg migration etc. I guess. How does holding the RCU lock guard the comparison? You're comparing two pointers for equality. Nothing any other CPU can do at this point will change the results of that comparison. > > Also, why are you bothering to re-enable interrupts here? Surely if > > you're holding lock A with interrupts disabled , you can just drop lock A, > > acquire lock B and leave the interrupts alone. That way you only need > > one of this variety of function, and not the separate irq/irqsave variants. > > > > Thanks for the suggestion! Yes, if only do re-lock, it's better to leave the irq unchanging. but, when the locked_lruvec is NULL, it become a first time lock which irq or irqsave are different. Thus, in combined function we need a nother parameter to indicate if it do irqsaving. So comparing to a extra/indistinct parameter, I guess 2 inline functions would be a bit more simple/cleary? Ah, right, I missed the "if it's not held" case.