From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Aneesh Kumar K.V" Subject: Re: [PATCH -V9 04/15] hugetlb: use mmu_gather instead of a temporary linked list for accumulating pages Date: Wed, 13 Jun 2012 22:13:00 +0530 Message-ID: <87y5nrmacr.fsf@skywalker.in.ibm.com> References: <1339583254-895-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com> <1339583254-895-5-git-send-email-aneesh.kumar@linux.vnet.ibm.com> <20120613145923.GA14777@tiehlicka.suse.cz> <20120613150338.GB14777@tiehlicka.suse.cz> Mime-Version: 1.0 Return-path: In-Reply-To: <20120613150338.GB14777@tiehlicka.suse.cz> Sender: owner-linux-mm@kvack.org List-ID: Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Michal Hocko Cc: linux-mm@kvack.org, kamezawa.hiroyu@jp.fujitsu.com, dhillf@gmail.com, rientjes@google.com, akpm@linux-foundation.org, hannes@cmpxchg.org, linux-kernel@vger.kernel.org, cgroups@vger.kernel.org Michal Hocko writes: > On Wed 13-06-12 16:59:23, Michal Hocko wrote: >> On Wed 13-06-12 15:57:23, Aneesh Kumar K.V wrote: >> > From: "Aneesh Kumar K.V" >> > >> > Use a mmu_gather instead of a temporary linked list for accumulating >> > pages when we unmap a hugepage range >> >> Sorry for coming up with the comment that late but you owe us an >> explanation _why_ you are doing this. >> >> I assume that this fixes a real problem when we take i_mmap_mutex >> already up in >> unmap_mapping_range >> mutex_lock(&mapping->i_mmap_mutex); >> unmap_mapping_range_tree | unmap_mapping_range_list >> unmap_mapping_range_vma >> zap_page_range_single >> unmap_single_vma >> unmap_hugepage_range >> mutex_lock(&vma->vm_file->f_mapping->i_mmap_mutex); >> >> And that this should have been marked for stable as well (I haven't >> checked when this has been introduced). >> >> But then I do not see how this help when you still do this: >> [...] >> > diff --git a/mm/memory.c b/mm/memory.c >> > index 1b7dc66..545e18a 100644 >> > --- a/mm/memory.c >> > +++ b/mm/memory.c >> > @@ -1326,8 +1326,11 @@ static void unmap_single_vma(struct mmu_gather *tlb, >> > * Since no pte has actually been setup, it is >> > * safe to do nothing in this case. >> > */ >> > - if (vma->vm_file) >> > - unmap_hugepage_range(vma, start, end, NULL); >> > + if (vma->vm_file) { >> > + mutex_lock(&vma->vm_file->f_mapping->i_mmap_mutex); >> > + __unmap_hugepage_range(tlb, vma, start, end, NULL); >> > + mutex_unlock(&vma->vm_file->f_mapping->i_mmap_mutex); >> > + } >> > } else >> > unmap_page_range(tlb, vma, start, end, details); >> > } > > Ahhh, you are removing the lock in the next patch. Really confusing and > not nice for the stable backport. > Could you merge those two patches and add Cc: stable? > Then you can add my > Reviewed-by: Michal Hocko > In the last review cycle I was asked to see if we can get a lockdep report for the above and what I found was we don't really cause the above deadlock with the current codebase because for hugetlb we don't directly call unmap_mapping_range. But still it is good to remove the i_mmap_mutex, because we don't need that protection now. I didn't mark it for stable because of the above reason. -aneesh -- 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 S1754372Ab2FMQni (ORCPT ); Wed, 13 Jun 2012 12:43:38 -0400 Received: from e28smtp09.in.ibm.com ([122.248.162.9]:35939 "EHLO e28smtp09.in.ibm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752917Ab2FMQng (ORCPT ); Wed, 13 Jun 2012 12:43:36 -0400 From: "Aneesh Kumar K.V" To: Michal Hocko Cc: linux-mm@kvack.org, kamezawa.hiroyu@jp.fujitsu.com, dhillf@gmail.com, rientjes@google.com, akpm@linux-foundation.org, hannes@cmpxchg.org, linux-kernel@vger.kernel.org, cgroups@vger.kernel.org Subject: Re: [PATCH -V9 04/15] hugetlb: use mmu_gather instead of a temporary linked list for accumulating pages In-Reply-To: <20120613150338.GB14777@tiehlicka.suse.cz> References: <1339583254-895-1-git-send-email-aneesh.kumar@linux.vnet.ibm.com> <1339583254-895-5-git-send-email-aneesh.kumar@linux.vnet.ibm.com> <20120613145923.GA14777@tiehlicka.suse.cz> <20120613150338.GB14777@tiehlicka.suse.cz> User-Agent: Notmuch/0.13.2+35~g0ff57e7 (http://notmuchmail.org) Emacs/24.1.50.1 (x86_64-unknown-linux-gnu) Date: Wed, 13 Jun 2012 22:13:00 +0530 Message-ID: <87y5nrmacr.fsf@skywalker.in.ibm.com> MIME-Version: 1.0 Content-Type: text/plain x-cbid: 12061316-2674-0000-0000-000004E368C3 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Michal Hocko writes: > On Wed 13-06-12 16:59:23, Michal Hocko wrote: >> On Wed 13-06-12 15:57:23, Aneesh Kumar K.V wrote: >> > From: "Aneesh Kumar K.V" >> > >> > Use a mmu_gather instead of a temporary linked list for accumulating >> > pages when we unmap a hugepage range >> >> Sorry for coming up with the comment that late but you owe us an >> explanation _why_ you are doing this. >> >> I assume that this fixes a real problem when we take i_mmap_mutex >> already up in >> unmap_mapping_range >> mutex_lock(&mapping->i_mmap_mutex); >> unmap_mapping_range_tree | unmap_mapping_range_list >> unmap_mapping_range_vma >> zap_page_range_single >> unmap_single_vma >> unmap_hugepage_range >> mutex_lock(&vma->vm_file->f_mapping->i_mmap_mutex); >> >> And that this should have been marked for stable as well (I haven't >> checked when this has been introduced). >> >> But then I do not see how this help when you still do this: >> [...] >> > diff --git a/mm/memory.c b/mm/memory.c >> > index 1b7dc66..545e18a 100644 >> > --- a/mm/memory.c >> > +++ b/mm/memory.c >> > @@ -1326,8 +1326,11 @@ static void unmap_single_vma(struct mmu_gather *tlb, >> > * Since no pte has actually been setup, it is >> > * safe to do nothing in this case. >> > */ >> > - if (vma->vm_file) >> > - unmap_hugepage_range(vma, start, end, NULL); >> > + if (vma->vm_file) { >> > + mutex_lock(&vma->vm_file->f_mapping->i_mmap_mutex); >> > + __unmap_hugepage_range(tlb, vma, start, end, NULL); >> > + mutex_unlock(&vma->vm_file->f_mapping->i_mmap_mutex); >> > + } >> > } else >> > unmap_page_range(tlb, vma, start, end, details); >> > } > > Ahhh, you are removing the lock in the next patch. Really confusing and > not nice for the stable backport. > Could you merge those two patches and add Cc: stable? > Then you can add my > Reviewed-by: Michal Hocko > In the last review cycle I was asked to see if we can get a lockdep report for the above and what I found was we don't really cause the above deadlock with the current codebase because for hugetlb we don't directly call unmap_mapping_range. But still it is good to remove the i_mmap_mutex, because we don't need that protection now. I didn't mark it for stable because of the above reason. -aneesh