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

[Shakeel Butt]

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.

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

> +
>  /*
>   * 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);

>  }
>
>  /*
> @@ -437,7 +442,7 @@ static inline struct obj_cgroup **page_obj_cgroups_check(struct page *page)
>  {
>         unsigned long memcg_data = page->memcg_data;
>
> -       if (memcg_data && (memcg_data & 0x1UL))
> +       if (memcg_data && test_bit(PG_MEMCG_OBJ_CGROUPS, &memcg_data))
>                 return (struct obj_cgroup **)memcg_data;
>
>         return NULL;
> @@ -453,7 +458,11 @@ static inline struct obj_cgroup **page_obj_cgroups_check(struct page *page)
>  static inline bool set_page_obj_cgroups(struct page *page,
>                                         struct obj_cgroup **objcgs)
>  {
> -       return !cmpxchg(&page->memcg_data, 0, (unsigned long)objcgs | 0x1UL);
> +       unsigned long memcg_data = (unsigned long)objcgs;
> +
> +       __set_bit(PG_MEMCG_OBJ_CGROUPS, &memcg_data);
> +
> +       return !cmpxchg(&page->memcg_data, 0, memcg_data);
>  }
>
>  /*
> --
> 2.26.2
>