Re: [PATCH v3 13/18] mm/memcg: Add folio_memcg_lock() and folio_memcg_unlock()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Jul 07, 2021 at 08:28:39PM +0100, Matthew Wilcox wrote:
> On Wed, Jul 07, 2021 at 01:08:51PM -0400, Johannes Weiner wrote:
> > On Wed, Jun 30, 2021 at 05:00:29AM +0100, Matthew Wilcox (Oracle) wrote:
> > > -static void __unlock_page_memcg(struct mem_cgroup *memcg)
> > > +static void __memcg_unlock(struct mem_cgroup *memcg)
> > 
> > This is too generic a name. There are several locks in the memcg, and
> > this one only locks the page->memcg bindings in the group.
> 
> Fair.  __memcg_move_unlock looks like the right name to me?

Could you please elaborate what the problem with the current name is?

mem_cgroup_move_account() does this:

	lock_page_memcg(page);
	page->memcg = to;
	__unlock_page_memcg(from);

It locks and unlocks the page->memcg binding which can be done coming
from the page or the memcg. The current names are symmetrical to
reflect that it's the same lock.

We could switch them both to move_lock, but as per the other email,
lock_page_memcg() was chosen to resemble lock_page(). Because from a
memcg POV they're interchangeable - the former is just a more narrowly
scoped version for contexts that don't hold the page lock. It used to
be called something else and we had several contexts taking redundant
locks on accident because this hierarchy wasn't clear.

I don't mind fixing poorly chosen or misleading naming schemes, but I
think we need better explanations to overcome the reasoning behind the
existing names, not just the assumption that there weren't any.



[Index of Archives]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [eCos]     [Asterisk Internet PBX]     [Linux API]     [Monitors]

  Powered by Linux