Re: [PATCH 2/3] mm: Simplify split_page_memcg()

[David Hildenbrand <david@redhat.com>]

On 04.11.24 22:05, Matthew Wilcox (Oracle) wrote:
> The last argument to split_page_memcg() is now always 0, so remove it,
> effectively reverting commit b8791381d7ed.
> 
> Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org>
> ---
>   include/linux/memcontrol.h |  4 ++--
>   mm/memcontrol.c            | 26 ++++++++++++++------------
>   mm/page_alloc.c            |  4 ++--
>   3 files changed, 18 insertions(+), 16 deletions(-)
> 
> diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
> index 5502aa8e138e..a787080f814f 100644
> --- a/include/linux/memcontrol.h
> +++ b/include/linux/memcontrol.h
> @@ -1044,7 +1044,7 @@ static inline void memcg_memory_event_mm(struct mm_struct *mm,
>   	rcu_read_unlock();
>   }
>   
> -void split_page_memcg(struct page *head, int old_order, int new_order);
> +void split_page_memcg(struct page *first, int order);
>   
>   #else /* CONFIG_MEMCG */
>   
> @@ -1463,7 +1463,7 @@ void count_memcg_event_mm(struct mm_struct *mm, enum vm_event_item idx)
>   {
>   }
>   
> -static inline void split_page_memcg(struct page *head, int old_order, int new_order)
> +static inline void split_page_memcg(struct page *first, int order)
>   {
>   }
>   #endif /* CONFIG_MEMCG */
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 5e44d6e7591e..506439a5dcfe 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -3034,25 +3034,27 @@ void __memcg_slab_free_hook(struct kmem_cache *s, struct slab *slab,
>   }
>   
>   /*
> - * Because folio_memcg(head) is not set on tails, set it now.
> + * The memcg data is only set on the first page, now transfer it to all the
> + * other pages.
>    */
> -void split_page_memcg(struct page *head, int old_order, int new_order)
> +void split_page_memcg(struct page *first, int order)
>   {
> -	struct folio *folio = page_folio(head);
> +	unsigned long memcg_data = first->memcg_data;
> +	struct obj_cgroup *objcg;
>   	int i;
> -	unsigned int old_nr = 1 << old_order;
> -	unsigned int new_nr = 1 << new_order;
> +	unsigned int nr = 1 << order;
>   
> -	if (mem_cgroup_disabled() || !folio_memcg_charged(folio))
> +	if (!memcg_data)
>   		return;
 >   > -	for (i = new_nr; i < old_nr; i += new_nr)
> -		folio_page(folio, i)->memcg_data = folio->memcg_data;
> +	VM_BUG_ON_PAGE((memcg_data & OBJEXTS_FLAGS_MASK) != MEMCG_DATA_KMEM,
> +			first);
> +	objcg = (void *)(memcg_data & ~OBJEXTS_FLAGS_MASK);

I'm not sure if dropping the

	if (mem_cgroup_disabled() || !folio_memcg_charged(folio))

end adding that OBJEXTS_FLAGS_MASK magic here is the right thing to do.

It also isn't really part of a revert of b8791381d7ed.


-- 
Cheers,

David / dhildenb