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=-8.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_HELO_NONE,URIBL_BLOCKED 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 F3455C433E0 for ; Fri, 5 Feb 2021 18:15:44 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 4322964E4D for ; Fri, 5 Feb 2021 18:15:44 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4322964E4D Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=cmpxchg.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id 9BFE36B0070; Fri, 5 Feb 2021 13:15:43 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 971C56B0071; Fri, 5 Feb 2021 13:15:43 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 861296B0073; Fri, 5 Feb 2021 13:15:43 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0074.hostedemail.com [216.40.44.74]) by kanga.kvack.org (Postfix) with ESMTP id 71F286B0070 for ; Fri, 5 Feb 2021 13:15:43 -0500 (EST) Received: from smtpin27.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay02.hostedemail.com (Postfix) with ESMTP id 1151F3636 for ; Fri, 5 Feb 2021 18:15:43 +0000 (UTC) X-FDA: 77785017366.27.egg41_1b15398275e6 Received: from filter.hostedemail.com (10.5.16.251.rfc1918.com [10.5.16.251]) by smtpin27.hostedemail.com (Postfix) with ESMTP id E1C903D663 for ; Fri, 5 Feb 2021 18:15:42 +0000 (UTC) X-HE-Tag: egg41_1b15398275e6 X-Filterd-Recvd-Size: 7281 Received: from mail-qt1-f173.google.com (mail-qt1-f173.google.com [209.85.160.173]) by imf36.hostedemail.com (Postfix) with ESMTP for ; Fri, 5 Feb 2021 18:15:42 +0000 (UTC) Received: by mail-qt1-f173.google.com with SMTP id c1so5675441qtc.1 for ; Fri, 05 Feb 2021 10:15:42 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=cmpxchg-org.20150623.gappssmtp.com; s=20150623; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=XIgSkeXWHW1iLdxhmfSy4vGDVypEJJKaTvz/+TUywfw=; b=h3S3h+C3rXQ+iT7q44q8ThSjMNuxNz+xsM6CGSL6Hj4noskqYapqnJM+rdc1eRECvH YkiJQTBjKNdep/452yExfB/n2Ii3ypd4q5ygcUoCf6F9MqWu+QhyTOLVwBaD0wn9Tgmr RAl69uYMB36ucKfSVfiJ9xaRs8GnZ08lcc/Dt+lxnxmsHDKs2qNvtekBQiFaNA1arh/+ d8Vi0Y9FC0tl7fLu7RewHBz+sKCM4lmOo79AayMW2O46Bv+Jp9OsX+yHzqITi+ATTrTG IxVYgTLTp6yp60AJUZ3IQwBltqRHOtugETh+kS/2HwuFjfXHCPsxmasmaoTlXfIZXn6c P0Xw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=XIgSkeXWHW1iLdxhmfSy4vGDVypEJJKaTvz/+TUywfw=; b=W4yEiw/SUvRLdQqOL8Gc/PocCU85+57f5RZVxnufAeTj7fRIoy77FsEMhi2Werrwml h3lgapEGha1Tb+UHzwo5XrbRldb9HeLFt6NMUjE8I2gHvY10Sg2OzBmhfe0vE+OEM6d/ wPfAJ/vrjA6xw+LIiZPtayeOQsJlZoeeNjS6TKLV/SDic04dUpf7tyWtIyg+H2+Zb0Pb Wpkmfk2brtgunCrzEjHZuVexb0inc6aJL7raesrwQte5pqzmh817ZfuqnvW3dxEn7C9W 3XKT/uBOPiX/agWFTwmD2hODPcJ5qKAxH+SxxsSJ0AmJVh5pO/Vm97TPeGR0AA5OQf4v YNCw== X-Gm-Message-State: AOAM533LNVH+rNkm8ltuQqgaCbnJnkiD2Nndaq27M455EFMqpGPfukez l4as+0ru+qLrZiHgwfBJpooY3w== X-Google-Smtp-Source: ABdhPJynku4WsNlWNkLqPPMEO+1WL/Aj8gCdIhU0G1o/UHEQQuwkMIOX7PU1QvPU2jXKenGmJv5YYA== X-Received: by 2002:ac8:480b:: with SMTP id g11mr5306250qtq.290.1612548941572; Fri, 05 Feb 2021 10:15:41 -0800 (PST) Received: from localhost (70.44.39.90.res-cmts.bus.ptd.net. [70.44.39.90]) by smtp.gmail.com with ESMTPSA id x62sm5248278qkd.1.2021.02.05.10.15.40 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 05 Feb 2021 10:15:40 -0800 (PST) Date: Fri, 5 Feb 2021 13:15:40 -0500 From: Johannes Weiner To: Michal Hocko Cc: Muchun Song , Vladimir Davydov , Andrew Morton , Cgroups , Linux Memory Management List , LKML Subject: Re: [External] Re: [PATCH] mm: memcontrol: remove rcu_read_lock from get_mem_cgroup_from_page Message-ID: References: <20210205062719.74431-1-songmuchun@bytedance.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: 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 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. 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); 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())