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

[Kirill A. Shutemov]

Dave Hansen wrote:
> 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?

I've looked into it once again and it seems there's not much space for
consolidation. Code structure looks very similar, but there are many
special cases for thp: fallback path, pte vs. pmd, etc. I don't see how we
can consolidate them in them in sane way.
I think copy is more maintainable :(

> 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.

I will document it in commit message and in comments for both functions.

-- 
 Kirill A. Shutemov
--
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/