Re: [PATCH v1 3/4] mm: introduce page memcg flags

[Roman Gushchin]

On Thu, Sep 24, 2020 at 12:03:35AM -0700, Shakeel Butt wrote:
> On Tue, Sep 22, 2020 at 1:38 PM Roman Gushchin <guro@xxxxxx> wrote:
> >
> > The lowest bit in page->memcg_data is used to distinguish between
> > struct memory_cgroup pointer and a pointer to a objcgs array.
> > All checks and modifications of this bit are open-coded.
> >
> > Let's formalize it using page memcg flags, defined in page_memcg_flags
> > enum and replace all open-coded accesses with test_bit()/__set_bit().
> >
> > Few additional flags might be added later. Flags are intended to be
> > mutually exclusive.
> 
> Why mutually exclusive? I understand mutual exclusion between non-slab
> kernel memory and objcgs vector but future feature might not need to
> be mutually exclusive.
> 
> One use-case I am thinking of is actually using a couple of bits here
> to store more idle (or hot) age by future extension of DAMON. That
> would be for user memory (anon or file and not slab or kmem) but
> multiple bits can set.

Yeah, I agree. There are no reasons to require a mutual exclusion.
I'll drop it.

> 
> >
> > Signed-off-by: Roman Gushchin <guro@xxxxxx>
> > ---
> >  include/linux/memcontrol.h | 29 +++++++++++++++++++----------
> >  1 file changed, 19 insertions(+), 10 deletions(-)
> >
> > diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> > index ab3ea3e90583..9a49f1e1c0c7 100644
> > --- a/include/linux/memcontrol.h
> > +++ b/include/linux/memcontrol.h
> > @@ -343,6 +343,11 @@ struct mem_cgroup {
> >
> >  extern struct mem_cgroup *root_mem_cgroup;
> >
> > +enum page_memcg_flags {
> > +       /* page->memcg_data is a pointer to an objcgs vector */
> > +       PG_MEMCG_OBJ_CGROUPS,
> > +};
> 
> If you agree with my next comment then I think PG_MEMCG_LAST_FLAG and
> MEMCG_FLAGS_MASK should be introduced in this patch instead of the
> next one.

Ok, agree.

> 
> > +
> >  /*
> >   * page_mem_cgroup - get the memory cgroup associated with a page
> >   * @page: a pointer to the page struct
> > @@ -371,13 +376,7 @@ static inline struct mem_cgroup *page_mem_cgroup_check(struct page *page)
> >  {
> >         unsigned long memcg_data = page->memcg_data;
> >
> > -       /*
> > -        * The lowest bit set means that memcg isn't a valid
> > -        * memcg pointer, but a obj_cgroups pointer.
> > -        * In this case the page is shared and doesn't belong
> > -        * to any specific memory cgroup.
> > -        */
> > -       if (memcg_data & 0x1UL)
> > +       if (test_bit(PG_MEMCG_OBJ_CGROUPS, &memcg_data))
> >                 return NULL;
> >
> >         return (struct mem_cgroup *)memcg_data;
> > @@ -422,7 +421,13 @@ static inline void clear_page_mem_cgroup(struct page *page)
> >   */
> >  static inline struct obj_cgroup **page_obj_cgroups(struct page *page)
> >  {
> > -       return (struct obj_cgroup **)(page->memcg_data & ~0x1UL);
> > +       unsigned long memcg_data = page->memcg_data;
> > +
> > +       VM_BUG_ON_PAGE(memcg_data && !test_bit(PG_MEMCG_OBJ_CGROUPS,
> > +                                              &memcg_data), page);
> > +       __clear_bit(PG_MEMCG_OBJ_CGROUPS, &memcg_data);
> > +
> > +       return (struct obj_cgroup **)memcg_data;
> 
> Wouldn't the following be more future proof?
> 
> return (struct obj_cgroup **)(memcg_data & ~MEMCG_FLAGS_MASK);

Agree. I'll send an updated version soon.

Thank you for looking into the patchset!

Roman