Re: [PATCH v4 1/4] mm: memcontrol: use helpers to access page's memcg data

[Johannes Weiner]

On Thu, Oct 01, 2020 at 01:51:43PM -0700, Roman Gushchin wrote:
> On Thu, Oct 01, 2020 at 02:59:50PM -0400, Johannes Weiner wrote:
> > On Thu, Oct 01, 2020 at 11:27:39AM -0700, Roman Gushchin wrote:
> > > On Thu, Oct 01, 2020 at 09:46:38AM -0400, Johannes Weiner wrote:
> > > > On Wed, Sep 30, 2020 at 05:27:07PM -0700, Roman Gushchin wrote:
> > > > > +/*
> > > > > + * set_page_memcg - associate a page with a memory cgroup
> > > > > + * @page: a pointer to the page struct
> > > > > + * @memcg: a pointer to the memory cgroup
> > > > > + *
> > > > > + * Associates a page with a memory cgroup.
> > > > > + */
> > > > > +static inline void set_page_memcg(struct page *page, struct mem_cgroup *memcg)
> > > > > +{
> > > > > +	VM_BUG_ON_PAGE(PageSlab(page), page);
> > > > > +
> > > > > +	/*
> > > > > +	 * Please, refer to page_memcg()'s description for the page and memcg
> > > > > +	 * binding stability requirements.
> > > > > +	 */
> > > > > +	page->memcg_data = (unsigned long)memcg;
> > > > > +}
> > > > 
> > > > Please delete and inline this as per previous feedback, thanks.
> > > 
> > > Why it's better?
> > > It's ok for set_page_memcg(), but obviously worse for set_page_objcgs():
> > > it was nice to have all bit magic in one place, in few helper functions.
> > > And now it spills into several places. What's the win?
> > 
> > set_page_objcgs() is a worthwhile abstraction because it includes the
> > synchronization primitives that make it safe to use wrt
> > page_objcgs(). They encapsulate the cmpxchg and the READ_ONCE().
> > 
> > set_page_memcg() doesn't do any synchronization and relies fully on
> > the contextual locking. The name implies that it includes things to
> > make it safe wrt page_memcg(), which isn't true at all. It's a long
> > and misleading name for '='.
> > 
> > Btw, I really don't mind having this discussion, but please don't send
> > revisions that silently ignore feedback you don't agree with.
> 
> I'm not ignoring: I thought you was looking to remove clear_page_* functions,
> but it wasn't clear you want eliminate set_page_memcg() function. Please, when
> you asking for some "style" changes, please, provide some rationale, it's way
> less obvious than you think, what exactly you don't like in the proposed version.

https://lore.kernel.org/lkml/20200930210124.GB469663@xxxxxxxxxxx/
mentions and provides a rationale for inlining the setting and
clearing (unnecessary abstraction/encapsulation).

This is a code base shared by many different developers. It's routine
to be asked for style changes that make the new code blend in with its
surroundings, follow precedents and use idiomatic constructs, or avoid
anti-patterns that are known to cause maintenance burden or make it
harder to hack or debug the code.

If I'm being too terse and something isn't clear, please just ask for
clarification.