Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S966658Ab3DQOgm (ORCPT ); Wed, 17 Apr 2013 10:36:42 -0400 Received: from mga01.intel.com ([192.55.52.88]:20868 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S966457Ab3DQOgl (ORCPT ); Wed, 17 Apr 2013 10:36:41 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.87,492,1363158000"; d="scan'208";a="320462978" From: "Kirill A. Shutemov" To: Dave Hansen Cc: "Kirill A. Shutemov" , Andrea Arcangeli , Andrew Morton , Al Viro , Hugh Dickins , Wu Fengguang , Jan Kara , Mel Gorman , linux-mm@kvack.org, Andi Kleen , Matthew Wilcox , "Kirill A. Shutemov" , Hillf Danton , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org In-Reply-To: <51631206.3060605@sr71.net> References: <1365163198-29726-1-git-send-email-kirill.shutemov@linux.intel.com> <1365163198-29726-32-git-send-email-kirill.shutemov@linux.intel.com> <51631206.3060605@sr71.net> Subject: Re: [PATCHv3, RFC 31/34] thp: initial implementation of do_huge_linear_fault() Content-Transfer-Encoding: 7bit Message-Id: <20130417143842.1A76CE0085@blue.fi.intel.com> Date: Wed, 17 Apr 2013 17:38:42 +0300 (EEST) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2948 Lines: 70 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/