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=-18.2 required=3.0 tests=BAYES_00,DKIMWL_WL_MED, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_IN_DEF_DKIM_WL autolearn=ham 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 BE1A0C433DB for ; Fri, 5 Feb 2021 18:59:21 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 2277A64E30 for ; Fri, 5 Feb 2021 18:59:21 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 2277A64E30 Authentication-Results: mail.kernel.org; dmarc=fail (p=reject dis=none) header.from=google.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 863E96B0006; Fri, 5 Feb 2021 13:59:20 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 813046B006E; Fri, 5 Feb 2021 13:59:20 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 702EF6B0070; Fri, 5 Feb 2021 13:59:20 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0196.hostedemail.com [216.40.44.196]) by kanga.kvack.org (Postfix) with ESMTP id 5CB426B0006 for ; Fri, 5 Feb 2021 13:59:20 -0500 (EST) Received: from smtpin02.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 13FC8824999B for ; Fri, 5 Feb 2021 18:59:20 +0000 (UTC) X-FDA: 77785127280.02.alley89_1201632275e7 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin02.hostedemail.com (Postfix) with ESMTP id EE18310097AA1 for ; Fri, 5 Feb 2021 18:59:19 +0000 (UTC) X-HE-Tag: alley89_1201632275e7 X-Filterd-Recvd-Size: 7580 Received: from mail-il1-f172.google.com (mail-il1-f172.google.com [209.85.166.172]) by imf03.hostedemail.com (Postfix) with ESMTP for ; Fri, 5 Feb 2021 18:59:19 +0000 (UTC) Received: by mail-il1-f172.google.com with SMTP id o7so2253778ils.2 for ; Fri, 05 Feb 2021 10:59:19 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=umcUJjWlYyqhcB34L7jgiVYkOGucvAbPTMYmElui7W8=; b=sxvdKwLVtJjOYJactSKqj9AfcdPfdvYp/V6Mg0fIQpYbLA8SHxX0K6BJDTpBjkL4Qv lmem8SOCb37UshDi2wl22+MCq4w+kmqrM8+C7BxN9ng+4g5Jv+E0D470uPlk9606zyDH H6607b6EDARbFm/hZH4nW6Dllif4XUfsJJkaxtFsAX3TRZPkDxlF+Y7KwKp/p5QD8t+q 2l2eaEjfR86d42X3W8MESoXsYXk7D9Fsx+TGcBIGbhFOvK3Hequ+8MYg4B7JcH4CzBAu xbReADtXCiPXx9Zr4+nMdp9YumSadxv6wRiu7O7zAAMyGK/+w72fe4Ti+3OysAhLZSKn NTzg== 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=umcUJjWlYyqhcB34L7jgiVYkOGucvAbPTMYmElui7W8=; b=qlLloZzReIZfKbNylZnjam4pJHSEVWqENa68rBDKVD4FKvXQuyc1Mrk/fYzBSkK4n0 u5LPqezCDbz+YIU4IGtrIY2VR2b3baTd8dhg71FHQgdgsh77Pbmql5JW33bCsWSdO/8u ERntEAFpR2obHOhoW61DmI7FP63wVBLIHQ9AewYRr9RnCNJPTEoTQN2Vhd/vEgyyhG6T nvHpIr8xWHjCEuDsbet4WPtLMBsrgpSjnqmo1FBygxcBlRgAIjb4yGXW40H8XRj9k/S7 NzdABqZYyNh8uUtYPNghf3Urvj/8iCT7BVJxUs/sc4IeNZ5I8iwgTs0eonu01bge9oh7 h94w== X-Gm-Message-State: AOAM533IaF4qN88shTxKhpgiN3QLedFBfDaGs+CPTH+Jcl4RczqbcLsR C8Hs0664armMRe4twUUoiN0uH92EyJDyaZFZnVDh5Q== X-Google-Smtp-Source: ABdhPJzRjyWFdBr5HltCd4z7bm71JYdOrZsSRUuNoWy4DyikEu7SE2W8cMJ25NmgWLn8OTjw3wqbX2rnNlrxUEEX44I= X-Received: by 2002:a05:6e02:1be6:: with SMTP id y6mr4840036ilv.145.1612551558458; Fri, 05 Feb 2021 10:59:18 -0800 (PST) MIME-Version: 1.0 References: <20210205062719.74431-1-songmuchun@bytedance.com> In-Reply-To: From: Shakeel Butt Date: Fri, 5 Feb 2021 10:59:06 -0800 Message-ID: Subject: Re: [External] Re: [PATCH] mm: memcontrol: remove rcu_read_lock from get_mem_cgroup_from_page To: Johannes Weiner Cc: Michal Hocko , Muchun Song , Vladimir Davydov , Andrew Morton , Cgroups , Linux Memory Management List , LKML 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 Fri, Feb 5, 2021 at 10:31 AM Johannes Weiner wrote: > > On Fri, Feb 05, 2021 at 11:32:24AM +0100, Michal Hocko wrote: > > On Fri 05-02-21 17:14:30, Muchun Song wrote: > > > On Fri, Feb 5, 2021 at 4:36 PM Michal Hocko wrote: > > > > > > > > On Fri 05-02-21 14:27:19, Muchun Song wrote: > > > > > The get_mem_cgroup_from_page() is called under page lock, so the page > > > > > memcg cannot be changed under us. > > > > > > > > Where is the page lock enforced? > > > > > > Because it is called from alloc_page_buffers(). This path is under > > > page lock. > > > > I do not see any page lock enforecement there. There is not even a > > comment requiring that. Can we grow more users where this is not the > > case? There is no actual relation between alloc_page_buffers and > > get_mem_cgroup_from_page except that the former is the only _current_ > > existing user. I would be careful to dictate locking based solely on > > that. > > Since alloc_page_buffers() holds the page lock throughout the entire > time it uses the memcg, there is no actual reason for it to use RCU or > even acquire an additional reference on the css. We know it's pinned, > the charge pins it, and the page lock pins the charge. It can neither > move to a different cgroup nor be uncharged. > > So what do you say we switch alloc_page_buffers() to page_memcg()? > > And because that removes the last user of get_mem_cgroup_from_page(), > we can kill it off and worry about a good interface once a consumer > materializes for it. This seems like even better approach. > > diff --git a/fs/buffer.c b/fs/buffer.c > index 96c7604f69b3..12a10f461b81 100644 > --- a/fs/buffer.c > +++ b/fs/buffer.c > @@ -847,7 +847,7 @@ struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size, > if (retry) > gfp |= __GFP_NOFAIL; > > - memcg = get_mem_cgroup_from_page(page); > + memcg = page_memcg(page); A comment explaining why page_memcg(page) is safe here and then the patch looks good to me. > old_memcg = set_active_memcg(memcg); > > head = NULL; > @@ -868,7 +868,6 @@ struct buffer_head *alloc_page_buffers(struct page *page, unsigned long size, > } > out: > set_active_memcg(old_memcg); > - mem_cgroup_put(memcg); > return head; > /* > * In case anything failed, we just free everything we got. > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h > index a8c7a0ccc759..a44b2d51aecc 100644 > --- a/include/linux/memcontrol.h > +++ b/include/linux/memcontrol.h > @@ -687,8 +687,6 @@ struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p); > > struct mem_cgroup *get_mem_cgroup_from_mm(struct mm_struct *mm); > > -struct mem_cgroup *get_mem_cgroup_from_page(struct page *page); > - > struct lruvec *lock_page_lruvec(struct page *page); > struct lruvec *lock_page_lruvec_irq(struct page *page); > struct lruvec *lock_page_lruvec_irqsave(struct page *page, > @@ -1169,11 +1167,6 @@ static inline struct mem_cgroup *get_mem_cgroup_from_mm(struct mm_struct *mm) > return NULL; > } > > -static inline struct mem_cgroup *get_mem_cgroup_from_page(struct page *page) > -{ > - return NULL; > -} > - > static inline void mem_cgroup_put(struct mem_cgroup *memcg) > { > } > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 490357945f2c..ff52550d2f65 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -1048,29 +1048,6 @@ struct mem_cgroup *get_mem_cgroup_from_mm(struct mm_struct *mm) > } > EXPORT_SYMBOL(get_mem_cgroup_from_mm); > > -/** > - * get_mem_cgroup_from_page: Obtain a reference on given page's memcg. > - * @page: page from which memcg should be extracted. > - * > - * Obtain a reference on page->memcg and returns it if successful. Otherwise > - * root_mem_cgroup is returned. > - */ > -struct mem_cgroup *get_mem_cgroup_from_page(struct page *page) > -{ > - struct mem_cgroup *memcg = page_memcg(page); > - > - if (mem_cgroup_disabled()) > - return NULL; > - > - rcu_read_lock(); > - /* Page should not get uncharged and freed memcg under us. */ > - if (!memcg || WARN_ON_ONCE(!css_tryget(&memcg->css))) > - memcg = root_mem_cgroup; > - rcu_read_unlock(); > - return memcg; > -} > -EXPORT_SYMBOL(get_mem_cgroup_from_page); > - > static __always_inline struct mem_cgroup *active_memcg(void) > { > if (in_interrupt())