Re: [PATCH 2/4] [BUGFIX] dont set USED bit on tail pages

[Johannes Weiner <hannes@cmpxchg.org>]

On Fri, Jan 14, 2011 at 07:09:09PM +0900, KAMEZAWA Hiroyuki wrote:
> --- mmotm-0107.orig/mm/memcontrol.c
> +++ mmotm-0107/mm/memcontrol.c

> @@ -2154,6 +2139,23 @@ static void __mem_cgroup_commit_charge(s
>  	 */
>  	memcg_check_events(mem, pc->page);
>  }
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +/*
> + * Because tail pages are not mared as "used", set it. We're under

marked

> + * compund_lock and don't need to take care of races.
> + * Statistics are updated properly at charging. We just mark Used bits.
> + */
> +void mem_cgroup_split_huge_fixup(struct page *head, struct page *tail)
> +{
> +	struct page_cgroup *hpc = lookup_page_cgroup(head);
> +	struct page_cgroup *tpc = lookup_page_cgroup(tail);

I have trouble reading the code fluently with those names as they are
just very similar random letter sequences.  Could you rename them so
that they're better to discriminate?  headpc and tailpc perhaps?

> +	tpc->mem_cgroup = hpc->mem_cgroup;
> +	smp_wmb(); /* see __commit_charge() */
> +	SetPageCgroupUsed(tpc);
> +	VM_BUG_ON(PageCgroupCache(hpc));

Right now, this would be a bug due to other circumstances, but this
function does not require the page to be anon to function correctly,
does it?  I don't think we should encode a made up dependency here.

> @@ -2602,8 +2603,7 @@ __mem_cgroup_uncharge_common(struct page
>  		break;
>  	}
>  
> -	for (i = 0; i < count; i++)
> -		mem_cgroup_charge_statistics(mem, file, -1);
> +	mem_cgroup_charge_statistics(mem, file, -count);

Pass PageCgroupCache(pc) instead, ditch the `file' variable?

>  	ClearPageCgroupUsed(pc);
>  	/*
> Index: mmotm-0107/include/linux/memcontrol.h
> ===================================================================
> --- mmotm-0107.orig/include/linux/memcontrol.h
> +++ mmotm-0107/include/linux/memcontrol.h
> @@ -146,6 +146,10 @@ unsigned long mem_cgroup_soft_limit_recl
>  						gfp_t gfp_mask);
>  u64 mem_cgroup_get_limit(struct mem_cgroup *mem);
>  
> +#ifdef CONFIG_TRANSPARENT_HUGEPAGE
> +void mem_cgroup_split_huge_fixup(struct page *head, struct page *tail);
> +#endif
> +
>  #else /* CONFIG_CGROUP_MEM_RES_CTLR */
>  struct mem_cgroup;
>  
> Index: mmotm-0107/mm/huge_memory.c
> ===================================================================
> --- mmotm-0107.orig/mm/huge_memory.c
> +++ mmotm-0107/mm/huge_memory.c
> @@ -1203,6 +1203,8 @@ static void __split_huge_page_refcount(s
>  		BUG_ON(!PageDirty(page_tail));
>  		BUG_ON(!PageSwapBacked(page_tail));
>  
> +		mem_cgroup_split_huge_fixup(page, page_tail);

You need to provide a dummy for non-memcg configurations.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom policy in Canada: sign http://dissolvethecrtc.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>