From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id D9075EE499F for ; Wed, 11 Sep 2024 12:55:51 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 72F53940032; Wed, 11 Sep 2024 08:55:51 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 6DFDE940021; Wed, 11 Sep 2024 08:55:51 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 5CE0B940032; Wed, 11 Sep 2024 08:55:51 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 40ACA940021 for ; Wed, 11 Sep 2024 08:55:51 -0400 (EDT) Received: from smtpin04.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id DDD3516181B for ; Wed, 11 Sep 2024 12:55:50 +0000 (UTC) X-FDA: 82552454460.04.7D30B3A Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by imf06.hostedemail.com (Postfix) with ESMTP id 2E8E9180019 for ; Wed, 11 Sep 2024 12:55:48 +0000 (UTC) Authentication-Results: imf06.hostedemail.com; dkim=none; spf=pass (imf06.hostedemail.com: domain of dev.jain@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=dev.jain@arm.com; dmarc=pass (policy=none) header.from=arm.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1726059211; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=3iARYYBN2T33bRgkz3tkCLw7KDxYQ64/WC0zqYtS6rE=; b=xkKOoLwc8y6Sv5Nxyl2gLSPVq5WJG7yqnY+gYeWIYEDYFgpgTuuO/ZJd1zwRN1Ac8TD6+6 Ylb3j8mDHmtO+syIYsRvy6M6d4IIR9fupLC1jIgkOuonefeANXpDjt0ZlkpunxK/DAYJTv 3f87+xBqeXBD7vdji4SgKdPXZc6UjIo= ARC-Authentication-Results: i=1; imf06.hostedemail.com; dkim=none; spf=pass (imf06.hostedemail.com: domain of dev.jain@arm.com designates 217.140.110.172 as permitted sender) smtp.mailfrom=dev.jain@arm.com; dmarc=pass (policy=none) header.from=arm.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1726059211; a=rsa-sha256; cv=none; b=yOFkoKC2xjBFdQSl6EiOAVULbwJD2fpGhxDsvdzUcsVSGVoPck03/qeC1iVtmkb7YKQD/j qfUTE7jUi76J8mm/F9VJIh4PfTFZ7wjSSNN7A+gZucUHnnMdakB8QHF7ibU6NvBtcpFXJZ VSJ0LhiIb/IPTMRd2sWAjTCWRgbq0Og= 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 8E3A91007; Wed, 11 Sep 2024 05:56:17 -0700 (PDT) Received: from [10.162.40.31] (e116581.arm.com [10.162.40.31]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id BD6BF3F66E; Wed, 11 Sep 2024 05:55:39 -0700 (PDT) Message-ID: <4b981fae-0f40-4338-990f-839a39c3c190@arm.com> Date: Wed, 11 Sep 2024 18:25:36 +0530 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 1/2] mm: Abstract THP allocation To: David Hildenbrand , akpm@linux-foundation.org, willy@infradead.org, kirill.shutemov@linux.intel.com Cc: ryan.roberts@arm.com, 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, mark.rutland@arm.com, hughd@google.com, aneesh.kumar@kernel.org, yang@os.amperecomputing.com, peterx@redhat.com, ioworker0@gmail.com, jglisse@google.com, wangkefeng.wang@huawei.com, ziy@nvidia.com, linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org References: <20240911065600.1002644-1-dev.jain@arm.com> <20240911065600.1002644-2-dev.jain@arm.com> <552e9b77-c1ea-4a60-8434-e360ca692f1f@arm.com> <18271c5b-d190-448a-b513-34c20de1dcd2@redhat.com> Content-Language: en-US From: Dev Jain In-Reply-To: <18271c5b-d190-448a-b513-34c20de1dcd2@redhat.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: 2E8E9180019 X-Stat-Signature: 85pnc3rza5k4mud4ufngafo1no9bemha X-Rspam-User: X-HE-Tag: 1726059348-40219 X-HE-Meta: U2FsdGVkX19Wnt0OedCaXsceX0dy9LuUBYMM8gG4S3JpZPo0xEEq81TCuSx8UyPI4iJ/wInsYiA9WtUWgUb1Wn4XKeGXwOy+O7z+aIhETFh07C3lI+t6wE/M7jHmZzWM1qHLuzE9kZRrFEt8b612nduIDSSIJW2Pq3ZErhiVRu5LETtH57b4wbleEg0iotrIyk6YUA3Dj8/jVLBT6VvGmrBHxMshSKzrX2xQadIuv48ZkLY7Y8KNC69720lkMzxsCyCVW6evCF3/GIk+6B6Iz0GeDCHW6hfTGRhMYTSfBDYwEVSRrPx9qhaTQH6dkkHqS43xtI7Z/vIRN0cKuPD7VgP0S1V13TFhoRzQOjt+xlaHPXGOUewTGg3BS9VIP7NNjM7y/zeAffc0Fo5fa2NuIbHk2P9fWKFo3b5LcOrdTHYs81m5ePcYhrsCE59oE/gCdTtIjZG0koK3pJemQ1dMqCs3pDlJ3iJXT7RLwVhbiSUjIr/PoH9NZzPIMwt8t94OYnuovcM94Jz1QGgvPBcFh6iRFz54a+s7rE5Lo24548WtO5f388s+K1of79700AEEf7RMHRiyIT8wtwizDvxVP2jw6MSpylmxpP4qgpD7wgXfB/d1xfniOfIIhxct9e3qTJEPb6SmUW1XcBczl6kEC8jKqkTYFoGzURo9dgPmNmQpLj68aigLd9o+fplys3ABi32g+JaeMmU21PdSldoI0Atog5U3pnL/t79sJ4cjTJgTtuvvvRcnKJe54HXdt9pMKdBnv/zk7UhqNHKz8Uf+tWrGQCcncPjfWWgxjoluiP4HtNQwZFxzPqMwuIl59bJSHxvOem6MtLHKjx2kY+RB8WVihgn4ae2VwLtRfLxsKa8zwRPY8VcnXucZzSoMBltK97x1Eb9ZGQCmkdcauhozLr2mXirmpZSa+pVvxIv/j1XL2dl+8tr9t4qu2MKCE+UHf7PgXBsN44FdfVt19+k U2sPMzqJ GtXDx5l4HSFBXoV7B6FFVHUzaBD+0uti60lf5Jtq3cIcbekKMIqekxk8BJG8NPUacehQmGCxdbMIkbuJnuFK50wEcJcUw8lsF7Ug8nyM5CkUGJHSrhf7yvB0bvoWXB0K1f8cMs1QYfhTyIhfPWCjeQRUPhbE7zfYRcVcAbhHYCDYfygY= X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: List-Subscribe: List-Unsubscribe: On 9/11/24 18:05, David Hildenbrand wrote: > [...] > >>> >>>> + >>>> +static void map_pmd_thp(struct folio *folio, struct vm_fault *vmf, >>>> +            struct vm_area_struct *vma, unsigned long haddr) >>>> +{ >>>> +    pmd_t entry; >>>> + >>>> +    entry = mk_huge_pmd(&folio->page, vma->vm_page_prot); >>>> +    entry = maybe_pmd_mkwrite(pmd_mkdirty(entry), vma); >>>> +    folio_add_new_anon_rmap(folio, vma, haddr, RMAP_EXCLUSIVE); >>>> +    folio_add_lru_vma(folio, vma); >>>> +    set_pmd_at(vma->vm_mm, haddr, vmf->pmd, entry); >>>> +    update_mmu_cache_pmd(vma, vmf->address, vmf->pmd); >>> >>> It's quite weird to see a mixture of haddr and vmf->address, and >>> likely this mixture is wrong or not not required. >>> >>> Looking at arc's update_mmu_cache_pmd() implementation, I cannot see >>> how passing in the unaligned address would do the right thing. But >>> maybe arc also doesn't trigger that code path ... who knows :) >>> >>> >>> Staring at some other update_mmu_cache_pmd() users, it's quite >>> inconsistent. Primarily only do_huge_pmd_numa_page() and >>> __do_huge_pmd_anonymous_page() use the unaligned address. The others >>> seem to use the aligned address ... as one would expect when modifying >>> a PMD. >>> >>> >>> I suggest to change this function to *not* pass in the vmf, and rename >>> it to something like: >>> >>> static void folio_map_anon_pmd(struct folio *folio, struct >>> vm_area_struct *vma, pmd_t *pmd, unsigned long haddr) >>> >>> Then use haddr also to do the update_mmu_cache_pmd(). >> >> The code I changed, already was passing vmf->address to >> update_mmu_cache_pmd(). >> I did not change any of the haddr and vmf->address semantics, so really >> can't comment >> on this. > > I'm not saying that you changed it. I say, "we touch it we fix it" :). > We could do that in a separate cleanup patch upfront. Sure. > >> I agree with the name change; vmf will be required for >> set_pmd_at(vmf->pmd), so I should >> just pass pmd? >>> >>>> +    add_mm_counter(vma->vm_mm, MM_ANONPAGES, HPAGE_PMD_NR); >>>> +    mm_inc_nr_ptes(vma->vm_mm); >>>> +} >>>> + >>>> +static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf) >>>> +{ >>>> +    struct vm_area_struct *vma = vmf->vma; >>>> +    struct folio *folio; >>>> +    pgtable_t pgtable; >>>> +    unsigned long haddr = vmf->address & HPAGE_PMD_MASK; >>>> +    vm_fault_t ret = 0; >>>> +    gfp_t gfp = vma_thp_gfp_mask(vma); >>> >>> Nit: While at it, try to use reverse christmas-tree where possible, >>> makes things more reasible. You could make haddr const. >>> >>> struct vm_area_struct *vma = vmf->vma; >>> unsigned long haddr = vmf->address & HPAGE_PMD_MASK; >>> gfp_t gfp = vma_thp_gfp_mask(vma); >>> struct folio *folio; >>> vm_fault_t ret = 0; >> >> Okay. >>> ... >>> >>>> + >>>> +    folio = pmd_thp_fault_alloc(gfp, vma, haddr, vmf->address); >>>> +    if (unlikely(!folio)) { >>>> +        ret = VM_FAULT_FALLBACK; >>>> +        goto release; >>>> +    } >>>> + >>>> +    pgtable = pte_alloc_one(vma->vm_mm); >>>> +    if (unlikely(!pgtable)) { >>>> +        ret = VM_FAULT_OOM; >>>> +        goto release; >>>> +    } >>>>          vmf->ptl = pmd_lock(vma->vm_mm, vmf->pmd); >>>> + >>> >>> Nit Unrelated change. >> >> Are you asking me to align this line with the below line? > > No, just pointing out that you add a newline in a code section you > don't really touch. Sometimes that's deliberate, sometimes not (e.g., > going back and forth while reworking the code and accidentally leaving > that in). We try to avoid such unrelated changes because it adds noise > to the patches. > > If it was deliberate, feel free to leave it in. Ah, I accidentally inserted that new line. Will revert that.