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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Fri, 14 Jan 2011 13:09:31 +0100
From:	Johannes Weiner <hannes@...xchg.org>
To:	KAMEZAWA Hiroyuki <kamezawa.hiroyu@...fujitsu.com>
Cc:	"linux-mm@...ck.org" <linux-mm@...ck.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"nishimura@....nes.nec.co.jp" <nishimura@....nes.nec.co.jp>,
	"balbir@...ux.vnet.ibm.com" <balbir@...ux.vnet.ibm.com>,
	Greg Thelen <gthelen@...gle.com>, aarcange@...hat.com,
	"akpm@...ux-foundation.org" <akpm@...ux-foundation.org>
Subject: Re: [PATCH 2/4] [BUGFIX] dont set USED bit on tail pages

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 from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/