From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id 2D3A9158DD1 for ; Tue, 17 Dec 2024 19:24:24 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734463467; cv=none; b=NCMMak61t/HjYJr4+dGNRhrbuLUp45DuprDmx5EQ7G3ExGcsY6pLOo8iGM9DXnO+AmV7QywFAl+Gvg0kZyJdHkYEYdF2WcNEIOQkdyo/k8aCL6m+sMISj+tAOuqqdFZciq94AfnmQEq+ewwKGpm6ctJ9Dgo1qfmQ64Q87369j1k= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734463467; c=relaxed/simple; bh=rn4mqQdzpjoiqDj74ZC7JkwoIrErxsRjiFHOYrWYDeg=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=TgOfBjnVYwmb6jSXvn+3DLxnXVuGHKAXj+pe6O97BJuZzKpFcATVFJCI2hYs6BblI9Wgw5bQ1a32PEmcEg4pX+D+6owh0CuBC+Eq8o7pPeB3pa76wvGm58U8yopzrnATT75gHaRTjrytrzycS6tcCOx4CpDf28PPMx8IRsJhrxI= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 4FA5C339; Tue, 17 Dec 2024 11:24:52 -0800 (PST) Received: from [10.57.91.184] (unknown [10.57.91.184]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id EDD9E3F528; Tue, 17 Dec 2024 11:24:18 -0800 (PST) Message-ID: <097741c8-b2ea-4869-8792-f9663019137c@arm.com> Date: Tue, 17 Dec 2024 19:24:17 +0000 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [RFC PATCH 08/12] khugepaged: Abstract PMD-THP collapse Content-Language: en-GB To: Dev Jain , akpm@linux-foundation.org, david@redhat.com, willy@infradead.org, kirill.shutemov@linux.intel.com Cc: anshuman.khandual@arm.com, catalin.marinas@arm.com, cl@gentwo.org, vbabka@suse.cz, mhocko@suse.com, apopple@nvidia.com, dave.hansen@linux.intel.com, will@kernel.org, baohua@kernel.org, jack@suse.cz, srivatsa@csail.mit.edu, haowenchao22@gmail.com, hughd@google.com, aneesh.kumar@kernel.org, yang@os.amperecomputing.com, peterx@redhat.com, ioworker0@gmail.com, wangkefeng.wang@huawei.com, ziy@nvidia.com, jglisse@google.com, surenb@google.com, vishal.moola@gmail.com, zokeefe@google.com, zhengqi.arch@bytedance.com, jhubbard@nvidia.com, 21cnbao@gmail.com, linux-mm@kvack.org, linux-kernel@vger.kernel.org References: <20241216165105.56185-1-dev.jain@arm.com> <20241216165105.56185-9-dev.jain@arm.com> From: Ryan Roberts In-Reply-To: <20241216165105.56185-9-dev.jain@arm.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 16/12/2024 16:51, Dev Jain wrote: > Abstract away taking the mmap_lock exclusively, copying page contents, and > setting the PMD, into vma_collapse_anon_folio_pmd(). > > Signed-off-by: Dev Jain > --- > mm/khugepaged.c | 119 +++++++++++++++++++++++++++--------------------- > 1 file changed, 66 insertions(+), 53 deletions(-) > > diff --git a/mm/khugepaged.c b/mm/khugepaged.c > index 078794aa3335..88beebef773e 100644 > --- a/mm/khugepaged.c > +++ b/mm/khugepaged.c > @@ -1111,58 +1111,17 @@ static int alloc_charge_folio(struct folio **foliop, struct mm_struct *mm, > return SCAN_SUCCEED; > } > > -static int collapse_huge_page(struct mm_struct *mm, unsigned long address, > - int referenced, int unmapped, int order, > - struct collapse_control *cc) > +static int vma_collapse_anon_folio_pmd(struct mm_struct *mm, unsigned long address, > + struct vm_area_struct *vma, struct collapse_control *cc, pmd_t *pmd, > + struct folio *folio) > { > + struct mmu_notifier_range range; > + spinlock_t *pmd_ptl, *pte_ptl; > LIST_HEAD(compound_pagelist); > - pmd_t *pmd, _pmd; > - pte_t *pte; > pgtable_t pgtable; > - struct folio *folio; > - spinlock_t *pmd_ptl, *pte_ptl; > - int result = SCAN_FAIL; > - struct vm_area_struct *vma; > - struct mmu_notifier_range range; > - > - VM_BUG_ON(address & ~HPAGE_PMD_MASK); > - > - /* > - * Before allocating the hugepage, release the mmap_lock read lock. > - * The allocation can take potentially a long time if it involves > - * sync compaction, and we do not need to hold the mmap_lock during > - * that. We will recheck the vma after taking it again in write mode. > - */ > - mmap_read_unlock(mm); > - > - result = alloc_charge_folio(&folio, mm, order, cc); > - if (result != SCAN_SUCCEED) > - goto out_nolock; > - > - mmap_read_lock(mm); > - result = hugepage_vma_revalidate(mm, address, true, &vma, order, cc); > - if (result != SCAN_SUCCEED) { > - mmap_read_unlock(mm); > - goto out_nolock; > - } > - > - result = find_pmd_or_thp_or_none(mm, address, &pmd); > - if (result != SCAN_SUCCEED) { > - mmap_read_unlock(mm); > - goto out_nolock; > - } > - > - if (unmapped) { > - /* > - * __collapse_huge_page_swapin will return with mmap_lock > - * released when it fails. So we jump out_nolock directly in > - * that case. Continuing to collapse causes inconsistency. > - */ > - result = __collapse_huge_page_swapin(mm, vma, address, pmd, > - referenced, order); > - if (result != SCAN_SUCCEED) > - goto out_nolock; > - } > + int result; > + pmd_t _pmd; > + pte_t *pte; > > mmap_read_unlock(mm); > /* > @@ -1174,7 +1133,8 @@ static int collapse_huge_page(struct mm_struct *mm, unsigned long address, > * mmap_lock. > */ > mmap_write_lock(mm); > - result = hugepage_vma_revalidate(mm, address, true, &vma, order, cc); > + > + result = hugepage_vma_revalidate(mm, address, true, &vma, HPAGE_PMD_ORDER, cc); > if (result != SCAN_SUCCEED) > goto out_up_write; > /* check if the pmd is still valid */ > @@ -1206,7 +1166,7 @@ static int collapse_huge_page(struct mm_struct *mm, unsigned long address, > pte = pte_offset_map_lock(mm, &_pmd, address, &pte_ptl); > if (pte) { > result = __collapse_huge_page_isolate(vma, address, pte, cc, > - &compound_pagelist, order); > + &compound_pagelist, HPAGE_PMD_ORDER); > spin_unlock(pte_ptl); > } else { > result = SCAN_PMD_NULL; > @@ -1262,11 +1222,64 @@ static int collapse_huge_page(struct mm_struct *mm, unsigned long address, > deferred_split_folio(folio, false); > spin_unlock(pmd_ptl); > > - folio = NULL; > - > result = SCAN_SUCCEED; > out_up_write: > mmap_write_unlock(mm); > + return result; > +} > + > +static int collapse_huge_page(struct mm_struct *mm, unsigned long address, > + int referenced, int unmapped, int order, > + struct collapse_control *cc) > +{ > + struct vm_area_struct *vma; > + int result = SCAN_FAIL; > + struct folio *folio; > + pmd_t *pmd; > + > + /* > + * Before allocating the hugepage, release the mmap_lock read lock. > + * The allocation can take potentially a long time if it involves > + * sync compaction, and we do not need to hold the mmap_lock during > + * that. We will recheck the vma after taking it again in write mode. > + */ > + mmap_read_unlock(mm); > + > + result = alloc_charge_folio(&folio, mm, order, cc); > + if (result != SCAN_SUCCEED) > + goto out_nolock; > + > + mmap_read_lock(mm); > + result = hugepage_vma_revalidate(mm, address, true, &vma, order, cc); > + if (result != SCAN_SUCCEED) { > + mmap_read_unlock(mm); > + goto out_nolock; > + } > + > + result = find_pmd_or_thp_or_none(mm, address, &pmd); > + if (result != SCAN_SUCCEED) { > + mmap_read_unlock(mm); > + goto out_nolock; > + } > + > + if (unmapped) { > + /* > + * __collapse_huge_page_swapin will return with mmap_lock > + * released when it fails. So we jump out_nolock directly in > + * that case. Continuing to collapse causes inconsistency. > + */ > + result = __collapse_huge_page_swapin(mm, vma, address, pmd, > + referenced, order); > + if (result != SCAN_SUCCEED) > + goto out_nolock; > + } > + > + if (order == HPAGE_PMD_ORDER) > + result = vma_collapse_anon_folio_pmd(mm, address, vma, cc, pmd, folio); I think the locking is broken here? collapse_huge_page() used to enter with the mmamp read lock and exit without the lock held at all. After the change, this is only true for order == HPAGE_PMD_ORDER. For other orders, you exit with the mmap read lock still held. Perhaps: else mmap_read_unlock(mm); > + > + if (result == SCAN_SUCCEED) > + folio = NULL; > + > out_nolock: > if (folio) > folio_put(folio);