Re: [PATCH v3 15/18] mm/memcg: Add mem_cgroup_folio_lruvec()

[Johannes Weiner <hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org>]

On Wed, Jun 30, 2021 at 08:18:33PM +0100, Matthew Wilcox wrote:
> On Wed, Jun 30, 2021 at 05:00:31AM +0100, Matthew Wilcox (Oracle) wrote:
> > This is the folio equivalent of mem_cgroup_page_lruvec().
> 
> I'm just going through and removing the wrappers.
> 
> Why is this function called this?  There's an odd mix of
> 
> lock_page_memcg()

This was chosen to match lock_page().

> page_memcg()

And this to match page_mapping(), page_zone() etc.

> count_memcg_page_event()

count_vm_event()

> split_page_memcg()

split_page(), split_page_owner()

> mem_cgroup_charge()
> mem_cgroup_swapin_charge_page()

These are larger, heavier subsystem API calls that modify all kinds of
state, not just the page. Hence the namespacing.

With the smaller getter/setter type functions on pages we have
traditionally used <verb>_<object> rather than page_<verb>, simply
because the page is such a low-level object and many functions do
sequences of page manipulations. Namespacing would turn them into:

	page_do_this(page);
	page_set_that(page);
	page_lock(page);
	if (page_is_blah(page))
		page_mark_foo(page);
	page_unlock(page);
	page_put(page);

which is hard on the reader because it obscures the salient part of
each line behind repetetive boiler plate.

> mem_cgroup_lruvec()

This is arguably not a namespace prefix, but rather an accessor
function to look up the memcg's lruvec.

> mem_cgroup_from_task()

This is a pattern to look up memcgs from various objects:

- mem_cgroup_from_css()
- mem_cgroup_from_counter()
- mem_cgroup_from_id()
- mem_cgroup_from_seq()
- mem_cgroup_from_obj()

and we used to have mem_cgroup_from_page() at some point...

> I'd really like to call this function folio_lruvec().

That would be a better name indeed.

However, pairing renames with conversion is worrisome because it means
having two unintuitively diverging names for the same operation in the
API during a time where everybody has to relearn the code base already.

Please either precede it with a rename to page_lruvec(), or keep the
existing naming pattern in the conversion.

> It happens to behave differently if the folio is part of a memcg,
> but conceptually, lruvecs aren't particularly tied to memcgs.

Conceptually, lruvecs are always tied to a memcg. On !CONFIG_MEMCG
kernels that just happens to be the root cgroup.

But because it's an accessor to a page attribute, dropping the
namespacing prefix is in line with things like page_memcg().