Re: [PATCHv3, RFC 31/34] thp: initial implementation of do_huge_linear_fault()

[Dave Hansen]

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/