From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from psmtp.com (na3sys010amx129.postini.com [74.125.245.129]) by kanga.kvack.org (Postfix) with SMTP id BFE7E6B013C for ; Thu, 13 Sep 2012 05:17:03 -0400 (EDT) Received: from /spool/local by e23smtp01.au.ibm.com with IBM ESMTP SMTP Gateway: Authorized Use Only! Violators will be prosecuted for from ; Thu, 13 Sep 2012 19:15:27 +1000 Received: from d23av02.au.ibm.com (d23av02.au.ibm.com [9.190.235.138]) by d23relay04.au.ibm.com (8.13.8/8.13.8/NCO v10.0) with ESMTP id q8D97i3726083534 for ; Thu, 13 Sep 2012 19:07:44 +1000 Received: from d23av02.au.ibm.com (loopback [127.0.0.1]) by d23av02.au.ibm.com (8.14.4/8.13.1/NCO v10.0 AVout) with ESMTP id q8D9GpsW026009 for ; Thu, 13 Sep 2012 19:16:52 +1000 Message-ID: <5051A481.3090901@linux.vnet.ibm.com> Date: Thu, 13 Sep 2012 17:16:49 +0800 From: Xiao Guangrong MIME-Version: 1.0 Subject: Re: [PATCH 2/3] thp: move release mmap_sem lock out of khugepaged_alloc_page References: <50508632.9090003@linux.vnet.ibm.com> <50508689.50904@linux.vnet.ibm.com> <20120912151844.a2f17f98.akpm@linux-foundation.org> In-Reply-To: <20120912151844.a2f17f98.akpm@linux-foundation.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Sender: owner-linux-mm@kvack.org List-ID: To: Andrew Morton Cc: Hugh Dickins , Linux Memory Management List , LKML On 09/13/2012 06:18 AM, Andrew Morton wrote: > On Wed, 12 Sep 2012 20:56:41 +0800 > Xiao Guangrong wrote: > >> To make the code more clear, move release the lock out of khugepaged_alloc_page >> >> ... >> >> --- a/mm/huge_memory.c >> +++ b/mm/huge_memory.c >> @@ -1854,11 +1854,6 @@ static struct page >> *hpage = alloc_hugepage_vma(khugepaged_defrag(), vma, address, >> node, __GFP_OTHER_NODE); >> >> - /* >> - * After allocating the hugepage, release the mmap_sem read lock in >> - * preparation for taking it in write mode. >> - */ >> - up_read(&mm->mmap_sem); >> if (unlikely(!*hpage)) { >> count_vm_event(THP_COLLAPSE_ALLOC_FAILED); >> *hpage = ERR_PTR(-ENOMEM); >> @@ -1905,7 +1900,6 @@ static struct page >> struct vm_area_struct *vma, unsigned long address, >> int node) >> { >> - up_read(&mm->mmap_sem); >> VM_BUG_ON(!*hpage); >> return *hpage; >> } >> @@ -1931,8 +1925,14 @@ static void collapse_huge_page(struct mm_struct *mm, >> >> VM_BUG_ON(address & ~HPAGE_PMD_MASK); >> >> - /* release the mmap_sem read lock. */ >> new_page = khugepaged_alloc_page(hpage, mm, vma, address, node); >> + >> + /* >> + * After allocating the hugepage, release the mmap_sem read lock in >> + * preparation for taking it in write mode. >> + */ >> + up_read(&mm->mmap_sem); >> + >> if (!new_page) >> return; > > Well that's a pretty minor improvement: one still has to go off on a > big hunt to locate the matching down_read(). > > And the patch will increase mmap_sem hold times by a teeny amount. Do > we really want to do this? Andrew, This is why i did in the previous patch (the lock is released in alloc function), but as you noticed, this is really teeny overload after this patch - only increase the load of count_vm_event() which operates a cpu-local variable. And, before i posted this patch, i did kerbench test, no regression was found. The another approach is, let the function name indicate the lock will be released, how about just change the function name to khugepaged_alloc_page_release_lock? -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@kvack.org. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: email@kvack.org From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754418Ab2IMJSI (ORCPT ); Thu, 13 Sep 2012 05:18:08 -0400 Received: from e23smtp02.au.ibm.com ([202.81.31.144]:40771 "EHLO e23smtp02.au.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752805Ab2IMJRB (ORCPT ); Thu, 13 Sep 2012 05:17:01 -0400 Message-ID: <5051A481.3090901@linux.vnet.ibm.com> Date: Thu, 13 Sep 2012 17:16:49 +0800 From: Xiao Guangrong User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:15.0) Gecko/20120827 Thunderbird/15.0 MIME-Version: 1.0 To: Andrew Morton CC: Hugh Dickins , Linux Memory Management List , LKML Subject: Re: [PATCH 2/3] thp: move release mmap_sem lock out of khugepaged_alloc_page References: <50508632.9090003@linux.vnet.ibm.com> <50508689.50904@linux.vnet.ibm.com> <20120912151844.a2f17f98.akpm@linux-foundation.org> In-Reply-To: <20120912151844.a2f17f98.akpm@linux-foundation.org> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit x-cbid: 12091309-5490-0000-0000-00000220F12B Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 09/13/2012 06:18 AM, Andrew Morton wrote: > On Wed, 12 Sep 2012 20:56:41 +0800 > Xiao Guangrong wrote: > >> To make the code more clear, move release the lock out of khugepaged_alloc_page >> >> ... >> >> --- a/mm/huge_memory.c >> +++ b/mm/huge_memory.c >> @@ -1854,11 +1854,6 @@ static struct page >> *hpage = alloc_hugepage_vma(khugepaged_defrag(), vma, address, >> node, __GFP_OTHER_NODE); >> >> - /* >> - * After allocating the hugepage, release the mmap_sem read lock in >> - * preparation for taking it in write mode. >> - */ >> - up_read(&mm->mmap_sem); >> if (unlikely(!*hpage)) { >> count_vm_event(THP_COLLAPSE_ALLOC_FAILED); >> *hpage = ERR_PTR(-ENOMEM); >> @@ -1905,7 +1900,6 @@ static struct page >> struct vm_area_struct *vma, unsigned long address, >> int node) >> { >> - up_read(&mm->mmap_sem); >> VM_BUG_ON(!*hpage); >> return *hpage; >> } >> @@ -1931,8 +1925,14 @@ static void collapse_huge_page(struct mm_struct *mm, >> >> VM_BUG_ON(address & ~HPAGE_PMD_MASK); >> >> - /* release the mmap_sem read lock. */ >> new_page = khugepaged_alloc_page(hpage, mm, vma, address, node); >> + >> + /* >> + * After allocating the hugepage, release the mmap_sem read lock in >> + * preparation for taking it in write mode. >> + */ >> + up_read(&mm->mmap_sem); >> + >> if (!new_page) >> return; > > Well that's a pretty minor improvement: one still has to go off on a > big hunt to locate the matching down_read(). > > And the patch will increase mmap_sem hold times by a teeny amount. Do > we really want to do this? Andrew, This is why i did in the previous patch (the lock is released in alloc function), but as you noticed, this is really teeny overload after this patch - only increase the load of count_vm_event() which operates a cpu-local variable. And, before i posted this patch, i did kerbench test, no regression was found. The another approach is, let the function name indicate the lock will be released, how about just change the function name to khugepaged_alloc_page_release_lock?