linux-kernel - Re: [PATCHv3, RFC 31/34] thp: initial implementation of do_huge_linear

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:	Mon, 08 Apr 2013 11:52:54 -0700
From:	Dave Hansen <dave@...1.net>
To:	"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>
CC:	Andrea Arcangeli <aarcange@...hat.com>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Al Viro <viro@...iv.linux.org.uk>,
	Hugh Dickins <hughd@...gle.com>,
	Wu Fengguang <fengguang.wu@...el.com>, Jan Kara <jack@...e.cz>,
	Mel Gorman <mgorman@...e.de>, linux-mm@...ck.org,
	Andi Kleen <ak@...ux.intel.com>,
	Matthew Wilcox <matthew.r.wilcox@...el.com>,
	"Kirill A. Shutemov" <kirill@...temov.name>,
	Hillf Danton <dhillf@...il.com>, linux-fsdevel@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCHv3, RFC 31/34] thp: initial implementation of do_huge_linear_fault()

On 04/05/2013 04:59 AM, Kirill A. Shutemov wrote:
> +int do_huge_linear_fault(struct mm_struct *mm, struct vm_area_struct *vma,
> +		unsigned long address, pmd_t *pmd, unsigned int flags)
> +{
> +	unsigned long haddr = address & HPAGE_PMD_MASK;
> +	struct page *cow_page, *page, *dirty_page = NULL;
> +	bool anon = false, fallback = false, page_mkwrite = false;
> +	pgtable_t pgtable = NULL;
> +	struct vm_fault vmf;
> +	int ret;
> +
> +	/* Fallback if vm_pgoff and vm_start are not suitable */
> +	if (((vma->vm_start >> PAGE_SHIFT) & HPAGE_CACHE_INDEX_MASK) !=
> +			(vma->vm_pgoff & HPAGE_CACHE_INDEX_MASK))
> +		return do_fallback(mm, vma, address, pmd, flags);
> +
> +	if (haddr < vma->vm_start || haddr + HPAGE_PMD_SIZE > vma->vm_end)
> +		return do_fallback(mm, vma, address, pmd, flags);
> +
> +	if (unlikely(khugepaged_enter(vma)))
> +		return VM_FAULT_OOM;
> +
> +	/*
> +	 * If we do COW later, allocate page before taking lock_page()
> +	 * on the file cache page. This will reduce lock holding time.
> +	 */
> +	if ((flags & FAULT_FLAG_WRITE) && !(vma->vm_flags & VM_SHARED)) {
> +		if (unlikely(anon_vma_prepare(vma)))
> +			return VM_FAULT_OOM;
> +
> +		cow_page = alloc_hugepage_vma(transparent_hugepage_defrag(vma),
> +				vma, haddr, numa_node_id(), 0);
> +		if (!cow_page) {
> +			count_vm_event(THP_FAULT_FALLBACK);
> +			return do_fallback(mm, vma, address, pmd, flags);
> +		}
> +		count_vm_event(THP_FAULT_ALLOC);
> +		if (mem_cgroup_newpage_charge(cow_page, mm, GFP_KERNEL)) {
> +			page_cache_release(cow_page);
> +			return do_fallback(mm, vma, address, pmd, flags);
> +		}

Ugh.  This is essentially a copy-n-paste of code in __do_fault(),
including the comments.  Is there no way to consolidate the code so that
there's less duplication here?

Part of the reason we have so many bugs in hugetlbfs is that it's really
a forked set of code that does things its own way.  I really hope we're
not going down the road of creating another feature in the same way.

When you copy *this* much code (or any, really), you really need to talk
about it in the patch description.  I was looking at other COW code, and
just happened to stumble on to the __do_fault() code.
--
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/