Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <cbdd91ba-bfd4-2f6d-9f9f-9cdcc8ff81a4@nutanix.com>
Date:   Mon, 27 Jun 2022 17:56:51 +0530
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.10.0
Subject: Re: [RFC PATCH 04/26] hugetlb: make huge_pte_lockptr take an explicit
 shift argument.
To:     James Houghton <jthoughton@google.com>,
        Mike Kravetz <mike.kravetz@oracle.com>,
        Muchun Song <songmuchun@bytedance.com>,
        Peter Xu <peterx@redhat.com>
Cc:     David Hildenbrand <david@redhat.com>,
        David Rientjes <rientjes@google.com>,
        Axel Rasmussen <axelrasmussen@google.com>,
        Mina Almasry <almasrymina@google.com>,
        Jue Wang <juew@google.com>,
        "Dr . David Alan Gilbert" <dgilbert@redhat.com>,
        linux-mm@kvack.org, linux-kernel@vger.kernel.org
References: <20220624173656.2033256-1-jthoughton@google.com>
 <20220624173656.2033256-5-jthoughton@google.com>
From:   "manish.mishra" <manish.mishra@nutanix.com>
In-Reply-To: <20220624173656.2033256-5-jthoughton@google.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?aFRMeEZWWG5hRk13eGJGMFVFZHFLeXhVK016Nk9kR2tTZitPeWlJN2NHalVZ?=
 =?utf-8?B?M0RBOHpKL24rYXFRZS9xd21DOE5BMG1NUmdhUHFLZCtPcHdMK1MwODY0eDlm?=
 =?utf-8?B?cGc1dGFFbFNmYVFRZUM2aDFYb2RQRkhSSkliaUVSeUFJblNkT1M1YmREUEFQ?=
 =?utf-8?B?Uk5aSjQ5NDhuQXNqc2tOQ1NnTFphcGJrVi9oT09rdzhrZXVjKzBsWXdpTkxN?=
 =?utf-8?B?eCszWkNuRlF0WG5BNmU1T2R6THFaZmpZaTdCU09GUlREb3hZc1VVYWpOdVdx?=
 =?utf-8?B?c0FJNUxVc1FXWk9yZTlQNndaSlcwQVJzSURpbnd4a09pSW5jZ1pKdDdua0ZD?=
 =?utf-8?B?cVJDYk1XY3hrWU9LbmliUWZLU3pJeUsvWDdVMWdINjRVdHdRTFJhSnRlMVpv?=
 =?utf-8?B?K2JobFNDelBzNXFheitpV3UvdGVEWHZCWTFXOVlGWTg5eVpyTVl3UmdwTzE1?=
 =?utf-8?B?T1FyNVNnYUhhSlJnWko0VkhncEVLL0lkTWg5MW1UYmVJTlorYUdhOVhaVlA1?=
 =?utf-8?B?Y1pyYXkzVTBRQ1VMbHhVNDNzQkg4bUxzbGJQNmpjcFh2UG4rMU9kOUZJb3BQ?=
 =?utf-8?B?Tktjb05wYmtBMU5iVnh5bFF4aGl0cVJVTHpZWHRtUVRSbGsxcnZ3YlVPRkMy?=
 =?utf-8?B?MGNEeU5FSHllL3NndjkybjFuQ3lmNVYrQkYzTldSTm13RHVXdGpnWEJ4czdt?=
 =?utf-8?B?OHZBNEVoOHNWZjFKMGlCYWxsYjJ2QVRybzU2UnNBWDZVdFo2VWY5cVRxRmdi?=
 =?utf-8?B?SGZRWmszVjdpdDRxTFFKaHQwc3hzRHZoRGNOYU1oU1FONUVGL0JFeXE1V083?=
 =?utf-8?B?YzVFbkt5b3ROWExPcWNaRFNQNjQ4ODB1NVBSZDYwQW1zQTNBQU92NnIvUjZH?=
 =?utf-8?B?cG8yWnNJUUNvU0J4SGpSZEE4V25HMGZVN3dPZU4rbjBncjFRcmRrMmJSUmdC?=
 =?utf-8?B?T2FlUEFvdnhZMTJyNmZjZUhabktsY044QTdMeUhWMldWckdaQ202dnl3d3Y3?=
 =?utf-8?B?OEgyVTJac29CZVo2OVNFSERDd01hYUVza25uMnhWMjU4L0Z4aXp5cVBvYXdv?=
 =?utf-8?B?MlF1UWxjeWo3eEsvRFZFVnhwSmgvZm1uR1Z6N0pnbmtEQUVsTlUvOEE0NXVh?=
 =?utf-8?B?Kzc2aXhVOGkxM3Y2ZkdhbjltTGw3azM4bjFnZVY3dUpEbTlOQ3JuNnZSbm1C?=
 =?utf-8?B?anFGeGRDeDFmWFdJNVJ3VWV6Szc5WnlIMDFmWGxlcVZ0S3FNWGlFVTZXbkwv?=
 =?utf-8?B?TlYxSTdCVVlvYVMwWGpBVSs3Y1I3WWY2Nm9BUHk2ajhYaUN3QWdSUWNPcnRq?=
 =?utf-8?B?ZTNDZ1F2dVF2T0REYi95anJYM2JPelc5V2ZyMnp3T3ZhbTgwdzRFSHNKMlM1?=
 =?utf-8?B?VXRDb2RucmhWb0xVQUowVmFQNUtRSUFBQ2hkRWYyY3ByRVdmK25pSnJmYWs2?=
 =?utf-8?B?U3MxZHFrTkRuMHFRR29US3M5dEJyZVV0REtxbC9KRFkyV3MvdUNWcFlwQVdE?=
 =?utf-8?B?THo1REtTZ0JZSlcyVEMwNWJyanM0em9KWEhSKzI2aFhpbTU4d1I1L092QnhB?=
 =?utf-8?B?OEZNc0RwM0VVQjVQNGo1Q2s4Ykowa0VObXg3c3lVV1NsaHoycjJJYkZZMFUz?=
 =?utf-8?B?K1hhZE0xV0pWVmtoU1hIUlZRS1pMcUQ2Z1YzYm5KQzd5S1RWcDkrT3FNakp2?=
 =?utf-8?B?N1pKaVljbTNkYkEzQlpjNkpzQlZ5WGFZRGxXY0lOU004ZWUySkNucWdBbEM0?=
 =?utf-8?B?em0vTjhkbzFoUVBNOUdPUzFvS0Vyb1hCR01HVjd5VTRpOXA2RElTK0p6ZE5B?=
 =?utf-8?B?bWIvSnk2dGRDb3ZBTlp4Q3BYWFAzVzd1Nzh6amFUNzZKQjEvUXZQUk5hVldk?=
 =?utf-8?B?cXBQb2tVZDVpUWxVYzZSQ1NIWkJjTXdtUVZuS3Jrd3FvaXNTbVhFbDNFckQ2?=
 =?utf-8?B?NmIyS084ZUdheFU2QzZnTUFkNU16dW1Nc28zdng2bjNrNzZwMUFDNUpOVTdU?=
 =?utf-8?B?bWwwbUFrd2Z4WktyZWNlTDNic1hqSnF6Ulp5dURRa0R0RmtoTDRmaEY1VWFQ?=
 =?utf-8?B?TkN6YktxUFAxOVhtOTJGZWZ1VVNvci8vNDlyOVNUTEs2MmxuZDhvNmpHSVgr?=
 =?utf-8?B?Z0RxM2tPM0J0dFVSRWJySDBaWllBZ1JlbVh2S3JFUk5UWkFURWdISzhFNXZQ?=
 =?utf-8?B?a2c9PQ==?=
X-OriginatorOrg: nutanix.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d9ea0cbf-75d8-4f81-15c8-08da583857d7
X-MS-Exchange-CrossTenant-AuthSource: PH0PR02MB7384.namprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Jun 2022 12:27:04.4285
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: bb047546-786f-4de1-bd75-24e5b6f79043
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: 05QwoIYg4GytSk/8BoFl4Xtc/gqFhfTFEN5yeCHC/u5lcseB5uImJ1cgDmFAD/F9BEUKzamkq91C2UGUsGINe9M/Q3hlxgX987+DgzSU2jM=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR02MB5296
X-Proofpoint-ORIG-GUID: lAt0UyXhOLXEoGEcRvSBMRC_gV_PF2Ac
X-Proofpoint-GUID: lAt0UyXhOLXEoGEcRvSBMRC_gV_PF2Ac
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.205,Aquarius:18.0.883,Hydra:6.0.517,FMLib:17.11.122.1
 definitions=2022-06-27_06,2022-06-24_01,2022-06-22_01
X-Proofpoint-Spam-Reason: safe
X-Spam-Status: No, score=-2.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 24/06/22 11:06 pm, James Houghton wrote:
> This is needed to handle PTL locking with high-granularity mapping. We
> won't always be using the PMD-level PTL even if we're using the 2M
> hugepage hstate. It's possible that we're dealing with 4K PTEs, in which
> case, we need to lock the PTL for the 4K PTE.
>
> Signed-off-by: James Houghton <jthoughton@google.com>
> ---
>   arch/powerpc/mm/pgtable.c |  3 ++-
>   include/linux/hugetlb.h   | 19 ++++++++++++++-----
>   mm/hugetlb.c              |  9 +++++----
>   mm/migrate.c              |  3 ++-
>   mm/page_vma_mapped.c      |  3 ++-
>   5 files changed, 25 insertions(+), 12 deletions(-)
>
> diff --git a/arch/powerpc/mm/pgtable.c b/arch/powerpc/mm/pgtable.c
> index e6166b71d36d..663d591a8f08 100644
> --- a/arch/powerpc/mm/pgtable.c
> +++ b/arch/powerpc/mm/pgtable.c
> @@ -261,7 +261,8 @@ int huge_ptep_set_access_flags(struct vm_area_struct *vma,
>   
>   		psize = hstate_get_psize(h);
>   #ifdef CONFIG_DEBUG_VM
> -		assert_spin_locked(huge_pte_lockptr(h, vma->vm_mm, ptep));
> +		assert_spin_locked(huge_pte_lockptr(huge_page_shift(h),
> +						    vma->vm_mm, ptep));
>   #endif
>   
>   #else
> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h
> index 498a4ae3d462..5fe1db46d8c9 100644
> --- a/include/linux/hugetlb.h
> +++ b/include/linux/hugetlb.h
> @@ -868,12 +868,11 @@ static inline gfp_t htlb_modify_alloc_mask(struct hstate *h, gfp_t gfp_mask)
>   	return modified_mask;
>   }
>   
> -static inline spinlock_t *huge_pte_lockptr(struct hstate *h,
> +static inline spinlock_t *huge_pte_lockptr(unsigned int shift,
>   					   struct mm_struct *mm, pte_t *pte)
>   {
> -	if (huge_page_size(h) == PMD_SIZE)
> +	if (shift == PMD_SHIFT)
>   		return pmd_lockptr(mm, (pmd_t *) pte);
> -	VM_BUG_ON(huge_page_size(h) == PAGE_SIZE);

I may have wrong understanding here, is per pmd lock for reducing

contention, if that is the case should be take per pmd lock of

PAGE_SIZE too and page_table_lock for anything higer than PMD.

>   	return &mm->page_table_lock;
>   }
>   
> @@ -1076,7 +1075,7 @@ static inline gfp_t htlb_modify_alloc_mask(struct hstate *h, gfp_t gfp_mask)
>   	return 0;
>   }
>   
> -static inline spinlock_t *huge_pte_lockptr(struct hstate *h,
> +static inline spinlock_t *huge_pte_lockptr(unsigned int shift,
>   					   struct mm_struct *mm, pte_t *pte)
>   {
>   	return &mm->page_table_lock;
> @@ -1116,7 +1115,17 @@ static inline spinlock_t *huge_pte_lock(struct hstate *h,
>   {
>   	spinlock_t *ptl;
>   
> -	ptl = huge_pte_lockptr(h, mm, pte);
> +	ptl = huge_pte_lockptr(huge_page_shift(h), mm, pte);
> +	spin_lock(ptl);
> +	return ptl;
> +}
> +
> +static inline spinlock_t *huge_pte_lock_shift(unsigned int shift,
> +					      struct mm_struct *mm, pte_t *pte)
> +{
> +	spinlock_t *ptl;
> +
> +	ptl = huge_pte_lockptr(shift, mm, pte);
>   	spin_lock(ptl);
>   	return ptl;
>   }
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 0eec34edf3b2..d6d0d4c03def 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -4817,7 +4817,7 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
>   			continue;
>   
>   		dst_ptl = huge_pte_lock(h, dst, dst_pte);
> -		src_ptl = huge_pte_lockptr(h, src, src_pte);
> +		src_ptl = huge_pte_lockptr(huge_page_shift(h), src, src_pte);
>   		spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
>   		entry = huge_ptep_get(src_pte);
>   		dst_entry = huge_ptep_get(dst_pte);
> @@ -4894,7 +4894,8 @@ int copy_hugetlb_page_range(struct mm_struct *dst, struct mm_struct *src,
>   
>   				/* Install the new huge page if src pte stable */
>   				dst_ptl = huge_pte_lock(h, dst, dst_pte);
> -				src_ptl = huge_pte_lockptr(h, src, src_pte);
> +				src_ptl = huge_pte_lockptr(huge_page_shift(h),
> +							   src, src_pte);
>   				spin_lock_nested(src_ptl, SINGLE_DEPTH_NESTING);
>   				entry = huge_ptep_get(src_pte);
>   				if (!pte_same(src_pte_old, entry)) {
> @@ -4948,7 +4949,7 @@ static void move_huge_pte(struct vm_area_struct *vma, unsigned long old_addr,
>   	pte_t pte;
>   
>   	dst_ptl = huge_pte_lock(h, mm, dst_pte);
> -	src_ptl = huge_pte_lockptr(h, mm, src_pte);
> +	src_ptl = huge_pte_lockptr(huge_page_shift(h), mm, src_pte);
>   
>   	/*
>   	 * We don't have to worry about the ordering of src and dst ptlocks
> @@ -6024,7 +6025,7 @@ int hugetlb_mcopy_atomic_pte(struct mm_struct *dst_mm,
>   		page_in_pagecache = true;
>   	}
>   
> -	ptl = huge_pte_lockptr(h, dst_mm, dst_pte);
> +	ptl = huge_pte_lockptr(huge_page_shift(h), dst_mm, dst_pte);
>   	spin_lock(ptl);
>   
>   	/*
> diff --git a/mm/migrate.c b/mm/migrate.c
> index e51588e95f57..a8a960992373 100644
> --- a/mm/migrate.c
> +++ b/mm/migrate.c
> @@ -318,7 +318,8 @@ void migration_entry_wait(struct mm_struct *mm, pmd_t *pmd,
>   void migration_entry_wait_huge(struct vm_area_struct *vma,
>   		struct mm_struct *mm, pte_t *pte)
>   {
> -	spinlock_t *ptl = huge_pte_lockptr(hstate_vma(vma), mm, pte);
> +	spinlock_t *ptl = huge_pte_lockptr(huge_page_shift(hstate_vma(vma)),
> +					   mm, pte);
>   	__migration_entry_wait(mm, pte, ptl);
>   }
>   
> diff --git a/mm/page_vma_mapped.c b/mm/page_vma_mapped.c
> index c10f839fc410..8921dd4e41b1 100644
> --- a/mm/page_vma_mapped.c
> +++ b/mm/page_vma_mapped.c
> @@ -174,7 +174,8 @@ bool page_vma_mapped_walk(struct page_vma_mapped_walk *pvmw)
>   		if (!pvmw->pte)
>   			return false;
>   
> -		pvmw->ptl = huge_pte_lockptr(hstate, mm, pvmw->pte);
> +		pvmw->ptl = huge_pte_lockptr(huge_page_shift(hstate),
> +					     mm, pvmw->pte);
>   		spin_lock(pvmw->ptl);
>   		if (!check_pte(pvmw))
>   			return not_found(pvmw);