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