On Fri 05-02-21 13:15:40, 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 <mhocko@xxxxxxxx> 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. Yes, this makes much more sense than impose a weird locking rules to a more general purpose helper. Thanks! -- Michal Hocko SUSE Labs
