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: <142229ce-fb50-992b-3b11-a1fb5f9175f9@nutanix.com>
Date:   Mon, 27 Jun 2022 19:20:58 +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 12/26] hugetlb: add HugeTLB splitting functionality
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-13-jthoughton@google.com>
From:   "manish.mishra" <manish.mishra@nutanix.com>
In-Reply-To: <20220624173656.2033256-13-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?UHNBbVY0SkUxUDdpZUtzWERDaWtUdDV6bGFibFlnWWdSUG93UnpuUXRZSEFB?=
 =?utf-8?B?VG1mWFZyVWJhNVozOGQxSUU4VTZISXFFTEVSbVVTSWdyU3Y4Zk96VFRuRU41?=
 =?utf-8?B?Ni9mUzRDVHZwYTFuM2o3MU01blVVVnNxNE5aTDNBTUtpMjhGaUJINWZoSHhq?=
 =?utf-8?B?a2UyU1pVVy94T2xxSktFMkNOYWhFQUNNK3ZKNE1vVDdtb1BZZ043VUhIU0ly?=
 =?utf-8?B?L01QT2xzT1g1YjVVaGtFNkRocUQ5WDVxM0g4enlucUprcm5IWkVQUmFOcFNi?=
 =?utf-8?B?bndtMDBhc2FtWVMvNk9XOUk4Y2ExZWN5UXlNeGVicXkrOEhNcHVhek1KSzZn?=
 =?utf-8?B?VmExQzRwR05pWHI0aFF3akErNS8zVlQ2ak4rMkVNc3h3d2VNem5HMnd2cC9u?=
 =?utf-8?B?OHBVbmREWk92TmlJRXdENXdXemdjQUVFZ0VVZThFTDd3YVQ0S2Q2VjVaME01?=
 =?utf-8?B?N2FnandVdkd6UGgrQVA0MUtBenhFTklnRGJTSHJEZTdCSmI1QndCTERmQWNn?=
 =?utf-8?B?MVFwcmRZNm9ocHFYV3k3emhNN2tvaEJDWU9MUVQ5RW80SjZEK2FvRVN5U0g0?=
 =?utf-8?B?bDh1R2JFTEgvM04rU29aaC9XOThYL3NkNEV0UFVVbkZ6L3RGa1pLSTlMb09H?=
 =?utf-8?B?bVJkT0hlS2tVS29ycmt2bFR1ei9mczVSWmdTZnByY1JaVjhBZ3BtWjVJVytj?=
 =?utf-8?B?WGlKYkhac3F4WGxodGc5RTRULzVxR1BqYnRCOHc2RUhuQitVdVBab00rQkc3?=
 =?utf-8?B?d3YvaVl2dzFQcHJZdTZoYkNRaVN0aEFhQ2FXRFpLQXJjamFydEp6aEZoNkF6?=
 =?utf-8?B?UFNIY1V0M0RMRDdMWmlBSGNtZmR1RHluVTdKNGtZdGlpM3gwTDdYY2Z2MVc4?=
 =?utf-8?B?Z1lRc1NBZ0gvdnMrMHA2d1N3Nll3TVlBdFJBRW1lUFVLZkNSdWZqQkJMY2dY?=
 =?utf-8?B?TWtiWHRyUmpRKzJLSmZ6MkNVZG1wVy9LRTZma0RIb3ZaNHZKcTE2V09kT00v?=
 =?utf-8?B?STRhVEhLcnd2TFhMYWNiV0VRUC9vcGl2N0pjVFpDQWxXdk5hb1NvZmNHRUVN?=
 =?utf-8?B?MzF5Mzl3T25WSVJ5cFV1K2twa01NeG1MZ1cwTGs3cVBadU5hRkZubDQwblBY?=
 =?utf-8?B?bEZBVnVnUTJ3NkpxRnhPWTlFWVhNNy9ud2tLNWU1WExFVC9QUUxLbVFUeWR3?=
 =?utf-8?B?THlaNXdpRlNPMWY5THRqUGpZL2g0ZUxiSDVqSUtaYk9pclpObmJzODlHRWJX?=
 =?utf-8?B?MUFZZGsyTllGWVFtQk5OVFRhdkJVMENuNzBPM1dxYjlvbk14RFNpRDlucElF?=
 =?utf-8?B?djRpaDJWK2lCQ3h6aHAzWUxuWHhKSzFsUDUwaHNiOXNMVW9QeXF5R3IvdkZJ?=
 =?utf-8?B?SlZqQXpURE1GWlFKM3ptS0ZLZzA0Z2NiUmlzUjEzK0JsYUk3TFJZSlZIbTk1?=
 =?utf-8?B?ZmZmYnFBYzI4QnZmSnMzNUdUbFpqTW0yWXk4eEhmeWlPZmVydXQyU29LR1hz?=
 =?utf-8?B?UFN0eS9oWC93dzhPVlhpUzV6K1ZFa3Z6UWI5K2FpczBNSFQwV2gzdGJ3OXIz?=
 =?utf-8?B?WklzMnFrQ0RjUVFtMkphZWdmWXJPdkphZUhIcmNmZ1ZNWHZVQXgzRDBXbVpM?=
 =?utf-8?B?WjFjY3M1VlREdzNEN0dISUprV3VXVzJkRjhXZ04yT0JBZXZLa1htSjRkZWZZ?=
 =?utf-8?B?SjZqVUVMM1lhblFEbDVIZFVnbUowL1hvbFZNays5NStaZUF4c0Z1TFI1MVBV?=
 =?utf-8?B?Q0NrU1o2OVhQQXFkK2FPeWdDR2Q0T2RHOXd1MlJNdzE2VWFqdnM1NTk2UU1l?=
 =?utf-8?B?UXdoUVJTUEh4eC92RG5nVStLNWdWemdUM0l4NGZyRHBhQTMyZzMybFlrNk1w?=
 =?utf-8?B?QVlmZVc3a1ppck8rMU11bU1tT2ZaczBaS2U3N21ob2JsQlpYUjdDbXlmZll2?=
 =?utf-8?B?Zkt3YUY4OGpyclNuY0pkclVSSGt1ZWhnNHFCY1A2cWpyc3NObXdSYzhWRTZL?=
 =?utf-8?B?V25vbEtueHlQUDVET1A5eU9ZY1lBYVdsRUR5QitSdWl3czdBdUt3d1hzZmdY?=
 =?utf-8?B?aFE0UmpLRTB5YXBsSGY2TmYrYTFESmVTckZuYUwxMVlsS3RJSXkwMHZBdnA0?=
 =?utf-8?B?V25Sb3FqT1NZWGVCMWpmWDVJN2VVQ3Q2dkp4VkQwWXZxYnNZS1RWSnJRU21x?=
 =?utf-8?B?L3c9PQ==?=
X-OriginatorOrg: nutanix.com
X-MS-Exchange-CrossTenant-Network-Message-Id: c9cb2516-aed9-43a0-760b-08da58441988
X-MS-Exchange-CrossTenant-AuthSource: PH0PR02MB7384.namprd02.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Jun 2022 13:51:13.8565
 (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: QFlt5QIUHxbrQntAO1LgqXU0UF5UIA6ZNViJOCSTYCPLX1zyBasrc9CDp6A2mbWLLTchShYuWAKdlpUNh8VBQtB/UXiStcpmRCurcewMqhs=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM6PR02MB6364
X-Proofpoint-ORIG-GUID: WsWMcN4sx5v1nP_DL21v0I0D_nO356v8
X-Proofpoint-GUID: WsWMcN4sx5v1nP_DL21v0I0D_nO356v8
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:
> The new function, hugetlb_split_to_shift, will optimally split the page
> table to map a particular address at a particular granularity.
>
> This is useful for punching a hole in the mapping and for mapping small
> sections of a HugeTLB page (via UFFDIO_CONTINUE, for example).
>
> Signed-off-by: James Houghton <jthoughton@google.com>
> ---
>   mm/hugetlb.c | 122 +++++++++++++++++++++++++++++++++++++++++++++++++++
>   1 file changed, 122 insertions(+)
>
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 3ec2a921ee6f..eaffe7b4f67c 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -102,6 +102,18 @@ struct mutex *hugetlb_fault_mutex_table ____cacheline_aligned_in_smp;
>   /* Forward declaration */
>   static int hugetlb_acct_memory(struct hstate *h, long delta);
>   
> +/*
> + * Find the subpage that corresponds to `addr` in `hpage`.
> + */
> +static struct page *hugetlb_find_subpage(struct hstate *h, struct page *hpage,
> +				 unsigned long addr)
> +{
> +	size_t idx = (addr & ~huge_page_mask(h))/PAGE_SIZE;
> +
> +	BUG_ON(idx >= pages_per_huge_page(h));
> +	return &hpage[idx];
> +}
> +
>   static inline bool subpool_is_free(struct hugepage_subpool *spool)
>   {
>   	if (spool->count)
> @@ -7044,6 +7056,116 @@ static unsigned int __shift_for_hstate(struct hstate *h)
>   	for ((tmp_h) = hstate; (shift) = __shift_for_hstate(tmp_h), \
>   			       (tmp_h) <= &hstates[hugetlb_max_hstate]; \
>   			       (tmp_h)++)
> +
> +/*
> + * Given a particular address, split the HugeTLB PTE that currently maps it
> + * so that, for the given address, the PTE that maps it is `desired_shift`.
> + * This function will always split the HugeTLB PTE optimally.
> + *
> + * For example, given a HugeTLB 1G page that is mapped from VA 0 to 1G. If we
> + * call this function with addr=0 and desired_shift=PAGE_SHIFT, will result in
> + * these changes to the page table:
> + * 1. The PUD will be split into 2M PMDs.
> + * 2. The first PMD will be split again into 4K PTEs.
> + */
> +static int hugetlb_split_to_shift(struct mm_struct *mm, struct vm_area_struct *vma,
> +			   const struct hugetlb_pte *hpte,
> +			   unsigned long addr, unsigned long desired_shift)
> +{
> +	unsigned long start, end, curr;
> +	unsigned long desired_sz = 1UL << desired_shift;
> +	struct hstate *h = hstate_vma(vma);
> +	int ret;
> +	struct hugetlb_pte new_hpte;
> +	struct mmu_notifier_range range;
> +	struct page *hpage = NULL;
> +	struct page *subpage;
> +	pte_t old_entry;
> +	struct mmu_gather tlb;
> +
> +	BUG_ON(!hpte->ptep);
> +	BUG_ON(hugetlb_pte_size(hpte) == desired_sz);
can it be BUG_ON(hugetlb_pte_size(hpte) <= desired_sz)
> +
> +	start = addr & hugetlb_pte_mask(hpte);
> +	end = start + hugetlb_pte_size(hpte);
> +
> +	i_mmap_assert_write_locked(vma->vm_file->f_mapping);

As it is just changing mappings is holding f_mapping required? I mean in future

is it any paln or way to use some per process level sub-lock?

> +
> +	BUG_ON(!hpte->ptep);
> +	/* This function only works if we are looking at a leaf-level PTE. */
> +	BUG_ON(!hugetlb_pte_none(hpte) && !hugetlb_pte_present_leaf(hpte));
> +
> +	/*
> +	 * Clear the PTE so that we will allocate the PT structures when
> +	 * walking the page table.
> +	 */
> +	old_entry = huge_ptep_get_and_clear(mm, start, hpte->ptep);
> +
> +	if (!huge_pte_none(old_entry))
> +		hpage = pte_page(old_entry);
> +
> +	BUG_ON(!IS_ALIGNED(start, desired_sz));
> +	BUG_ON(!IS_ALIGNED(end, desired_sz));
> +
> +	for (curr = start; curr < end;) {
> +		struct hstate *tmp_h;
> +		unsigned int shift;
> +
> +		for_each_hgm_shift(h, tmp_h, shift) {
> +			unsigned long sz = 1UL << shift;
> +
> +			if (!IS_ALIGNED(curr, sz) || curr + sz > end)
> +				continue;
> +			/*
> +			 * If we are including `addr`, we need to make sure
> +			 * splitting down to the correct size. Go to a smaller
> +			 * size if we are not.
> +			 */
> +			if (curr <= addr && curr + sz > addr &&
> +					shift > desired_shift)
> +				continue;
> +
> +			/*
> +			 * Continue the page table walk to the level we want,
> +			 * allocate PT structures as we go.
> +			 */

As i understand this for_each_hgm_shift loop is just to find right size of shift,

then code below this line can be put out of loop, no strong feeling but it looks

more proper may make code easier to understand.

> +			hugetlb_pte_copy(&new_hpte, hpte);
> +			ret = hugetlb_walk_to(mm, &new_hpte, curr, sz,
> +					      /*stop_at_none=*/false);
> +			if (ret)
> +				goto err;
> +			BUG_ON(hugetlb_pte_size(&new_hpte) != sz);
> +			if (hpage) {
> +				pte_t new_entry;
> +
> +				subpage = hugetlb_find_subpage(h, hpage, curr);
> +				new_entry = make_huge_pte_with_shift(vma, subpage,
> +								     huge_pte_write(old_entry),
> +								     shift);
> +				set_huge_pte_at(mm, curr, new_hpte.ptep, new_entry);
> +			}
> +			curr += sz;
> +			goto next;
> +		}
> +		/* We couldn't find a size that worked. */
> +		BUG();
> +next:
> +		continue;
> +	}
> +
> +	mmu_notifier_range_init(&range, MMU_NOTIFY_CLEAR, 0, vma, vma->vm_mm,
> +				start, end);
> +	mmu_notifier_invalidate_range_start(&range);

sorry did not understand where tlb flush will be taken care in case of success?

I see set_huge_pte_at does not do it internally by self.

> +	return 0;
> +err:
> +	tlb_gather_mmu(&tlb, mm);
> +	/* Free any newly allocated page table entries. */
> +	hugetlb_free_range(&tlb, hpte, start, curr);
> +	/* Restore the old entry. */
> +	set_huge_pte_at(mm, start, hpte->ptep, old_entry);
> +	tlb_finish_mmu(&tlb);
> +	return ret;
> +}
>   #endif /* CONFIG_HUGETLB_HIGH_GRANULARITY_MAPPING */
>   
>   /*