Re: [patch 13/13] mm: memcontrol: rewrite uncharge API

[Naoya Horiguchi]

On Tue, Jul 15, 2014 at 05:48:43PM -0400, Johannes Weiner wrote:
> On Tue, Jul 15, 2014 at 04:49:53PM -0400, Naoya Horiguchi wrote:
> > I feel that these 2 messages have the same cause (just appear differently).
> > __add_to_page_cache_locked() (and mem_cgroup_try_charge()) can be called
> > for hugetlb, while we avoid calling mem_cgroup_migrate()/mem_cgroup_uncharge()
> > for hugetlb. This seems to make page_cgroup of the hugepage inconsistent,
> > and results in the bad page bug ("page dumped because: cgroup check failed").
> > So maybe some more PageHuge check is necessary around the charging code.
> 
> This struck me as odd because I don't remember removing a PageHuge()
> call in the charge path and wondered how it worked before my changes:
> apparently it just checked PageCompound() in mem_cgroup_charge_file().
> 
> So it's not fallout of the new uncharge batching code, but was already
> broken during the rewrite of the charge API because then hugetlb pages
> entered the charging code.
> 
> Anyway, we don't have file-specific charging code anymore, and the
> PageCompound() check would have required changing anyway for THP
> cache.  So I guess the solution is checking PageHuge() in charge,
> uncharge, and migrate for now.  Oh well.
> 
> How about this?

With tweaking a bit, this patch solved the problem, thanks!

> diff --git a/mm/filemap.c b/mm/filemap.c
> index 9c99d6868a5e..b61194273b56 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -564,9 +564,12 @@ static int __add_to_page_cache_locked(struct page *page,
>  	VM_BUG_ON_PAGE(!PageLocked(page), page);
>  	VM_BUG_ON_PAGE(PageSwapBacked(page), page);
>  
> -	error = mem_cgroup_try_charge(page, current->mm, gfp_mask, &memcg);
> -	if (error)
> -		return error;
> +	if (!PageHuge(page)) {
> +		error = mem_cgroup_try_charge(page, current->mm,
> +					      gfp_mask, &memcg);
> +		if (error)
> +			return error;
> +	}
>  
>  	error = radix_tree_maybe_preload(gfp_mask & ~__GFP_HIGHMEM);
>  	if (error) {

We have mem_cgroup_commit_charge() later in __add_to_page_cache_locked(),
so adding "if (!PageHuge(page))" for it is necessary too.

> diff --git a/mm/migrate.c b/mm/migrate.c
> index 7f5a42403fae..dabed2f08609 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -781,7 +781,8 @@ static int move_to_new_page(struct page *newpage, struct page *page,
>  		if (!PageAnon(newpage))
>  			newpage->mapping = NULL;
>  	} else {
> -		mem_cgroup_migrate(page, newpage, false);
> +		if (!PageHuge(page))
> +			mem_cgroup_migrate(page, newpage, false);
>  		if (remap_swapcache)
>  			remove_migration_ptes(page, newpage);
>  		if (!PageAnon(page))
> diff --git a/mm/swap.c b/mm/swap.c
> index 3461f2f5be20..97b6ec132398 100644
> --- a/mm/swap.c
> +++ b/mm/swap.c
> @@ -62,12 +62,12 @@ static void __page_cache_release(struct page *page)
>  		del_page_from_lru_list(page, lruvec, page_off_lru(page));
>  		spin_unlock_irqrestore(&zone->lru_lock, flags);
>  	}
> -	mem_cgroup_uncharge(page);
>  }
>  
>  static void __put_single_page(struct page *page)
>  {
>  	__page_cache_release(page);
> +	mem_cgroup_uncharge_page(page);

My kernel is based on mmotm-2014-07-09-17-08, where mem_cgroup_uncharge_page()
does not exist any more. Maybe mem_cgroup_uncharge(page) seems correct.

>  	free_hot_cold_page(page, false);
>  }
>  
> @@ -75,7 +75,10 @@ static void __put_compound_page(struct page *page)
>  {
>  	compound_page_dtor *dtor;
>  
> -	__page_cache_release(page);
> +	if (!PageHuge(page)) {
> +		__page_cache_release(page);
> +		mem_cgroup_uncharge_page(page);

ditto.

Thanks,
Naoya Horiguchi
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/