On Wed, Jul 07, 2021 at 04:41:05PM -0400, Johannes Weiner wrote:

On Wed, Jul 07, 2021 at 08:28:39PM +0100, Matthew Wilcox wrote:

quoted

On Wed, Jul 07, 2021 at 01:08:51PM -0400, Johannes Weiner wrote:

quoted

On Wed, Jun 30, 2021 at 05:00:29AM +0100, Matthew Wilcox (Oracle) wrote:

quoted

-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.

OK, so in the prerequisite series to this patch, lock_page() becomes
folio_lock().  This series turns lock_page_memcg() into
folio_memcg_lock().  As a minimum, then, this needs to turn into
__folio_memcg_unlock().

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.

Unfortunately, it's still not clear.  I've answered questions from
people who think that they have the page locked because they called
lock_page_memcg() ;-(