Received-SPF: pass (google.com: domain of linux-kernel+bounces-57391-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <21c3c12a-c591-4548-891c-9ac66f0a615c@oracle.com>
Date: Wed, 7 Feb 2024 18:35:41 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH 2/3] arm64: mm: HVO: support BBM of vmemmap pgtable safely
Content-Language: en-US
To: Muchun Song <muchun.song@linux.dev>, Nanyong Sun <sunnanyong@huawei.com>,
        catalin.marinas@arm.com, will@kernel.org, mike.kravetz@oracle.com,
        akpm@linux-foundation.org, anshuman.khandual@arm.com
Cc: willy@infradead.org, wangkefeng.wang@huawei.com,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org
References: <20231214073912.1938330-1-sunnanyong@huawei.com>
 <20231214073912.1938330-3-sunnanyong@huawei.com>
 <50f082a8-b805-5d66-45f4-2af58e99a67f@linux.dev>
From: Jane Chu <jane.chu@oracle.com>
In-Reply-To: <50f082a8-b805-5d66-45f4-2af58e99a67f@linux.dev>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: 
	=?utf-8?B?Q3VBRVBzT25DaFZDdGhiZTNROE5TbXNaRVhPbGR5VGliWEYrVFFDbkZnd2NT?=
 =?utf-8?B?YnhFcWMweTNyY0lCYVNJL1ppL2lHQW5STDIrU2RGcGhnVnpubnhvSW8zUjlh?=
 =?utf-8?B?YXFLZmc0UHlDclhCcnVoSGtDdnFhRUxBVS9rdVY4amhuMWh2R245d3BzVXIr?=
 =?utf-8?B?bDRuVWh1b0JBOEx6a3dEdVpTRGsvS1JWOTdieGpJTDdrVmJHdzgrSGgzcHRm?=
 =?utf-8?B?ZnkvNWovT3Q2akovRTJkUjZlTWYwUkFtamswYVBkZWlaRUZSaDhmYlA2OHZ2?=
 =?utf-8?B?VTJJRW5KZS84V2E0M21STWIxZ3RJZFE0MHZVSjcrdnpuekwzU3F0QUkyTVBT?=
 =?utf-8?B?TTBjT2tRcFFBTmxQM3A3UE1BZXgwdUZaVUhQcTR6ZnVYL3lKdmFybjE2dzhP?=
 =?utf-8?B?cW9kV1FRdGY5eDFnQTNqMG96c2RtdnJGdkprMTZYZ3duRmRUYlUxTEJiUVlR?=
 =?utf-8?B?ckExT2w2dGhCTDVRbExPTWE0bUFDak9tNXM3VTBiLy9iYlMzd2w5UlduZEl5?=
 =?utf-8?B?VTY5K2E0MHRFYkxmalBraGp6RkNCeTllM0YyR1I1amZUd1FvYU93SHBGN05U?=
 =?utf-8?B?OGdpS01sbUUwUkFTb2ZobEFvWXg4QVVIMlV5QW9CU0JqREFncVZRRE1CMS9n?=
 =?utf-8?B?cGZScTR4SzNJSm9CS3BvNS90aFQ4Rm5XeXozSzdTaHZLSDR6RHBOQXR2S3FM?=
 =?utf-8?B?ckYwamlaSGthTmg2YU01cE9LK1NlVFNMRmlEK2RuWDdqdE84b3pSNlBpeGhq?=
 =?utf-8?B?RWJucm5USU56eUN3L0VkLzNpZ1BhdTdhRlpUY3Q4SW1KOW4wWW4zR0VYeFkv?=
 =?utf-8?B?UHNkWGZuK1Z5SVVKYWZMZ2J4ek91TEw5QVFVcGwzM3J1Tk1QYWZxWGNoNXdS?=
 =?utf-8?B?TzltSDRFTVJBU1lLemxFQkpYMGdaRFFRT1JnVXdYd1hQTU1qS2xOMFNLa1F4?=
 =?utf-8?B?cklWTGxLcU5oWUhZOERKZ1pTMzNvdFlEb0RiVGVCeTZsL3hTUXUwOFFCOS9v?=
 =?utf-8?B?WFVUUjFlaG91QlBWMFFSVkFXbEVXUW9KakpSUDNXNzJEdW5nWEJTeHQ5dk5N?=
 =?utf-8?B?bWF6S0F6ajBwRnJrQTFBMkJBdXFNeTE4QVVoREpOS0JnUVduZ2x0OExxTjBY?=
 =?utf-8?B?WnVEWWZTNksrdUdSVkdZamI1ckRSQnZJZHNCTzJydE02U1plTHBKT1hTdWt4?=
 =?utf-8?B?SVB5U1BFNDNBeHd2WFZMUTJacEtoMkJib0dqa2x4SkppMkdFN0V2cjVMSHJ6?=
 =?utf-8?B?ampDRlFZRTFZTWlRUWtYMEVIL3U2eFR2YjZpWG5sM3JlaXJhbTZnenVnVy8r?=
 =?utf-8?B?dEpiKzZLODY1NUJZb0tFN3NvMWQwL0RBY1FMcEtBazlOVkprYXRSL0N0czVQ?=
 =?utf-8?B?MC9JSDdidVpsNHNsVS90eE5RemExWkd2S1BmcE9Ka3kwak96NzdQUi8yZVBL?=
 =?utf-8?B?T0lOOFdSdDZqRkdKM2IxTXpWS1YyaGNFQUxyY05tV3ltUzRIMlVSNEJrWXJi?=
 =?utf-8?B?QTdSK00zR1E5NkcxWXZ0amlHcFdGbWhia1Z3alRLQ0JON0JjYWt4ZTZrUTQ4?=
 =?utf-8?B?NGNIa3piY2VlbGVZNmVzcXBSUndkNVg2dmNBZ2VaM3FJTzNIT2s3NnJGR0U4?=
 =?utf-8?B?NWJpb0lNc1E1SzJIcWQwUC92b2ppazhaNDBHQTNIakJmeExsZWlaWmo1UDlO?=
 =?utf-8?B?dVpPVHdQcWFERllpczJ3bXc3eFdvYVMxaEpycEN2TGZiWEtGQXY0QWJYZ1Bq?=
 =?utf-8?B?bms0MWJ6UGhJbnV4WHRHM0k4NkhwNndQa25keWdkb096cVdZMWovYU53eFRT?=
 =?utf-8?B?dithbG5GM3J6L0Yya3R2NXZRMlZ6RWhPaExlZ1UrSmF6bFpBREdaK2RCbFh5?=
 =?utf-8?B?cFhQZUZEUnZJaDFHdU82OTVoMys5c295OWRQZitpS25QQ3BEMnVQTTVGTS9W?=
 =?utf-8?B?eXNtR1U4QWVPcnM0TWM1bGFLN0F3dFNSazJrbTJmN3pHSEJncS9IV1J5Rzd2?=
 =?utf-8?B?NHlmcW9KNTh0Y2tkTHk0QWJ5TXZBeDQ5UUV2VFcwd252d202TEtFTUtITzNm?=
 =?utf-8?B?QWY5WjV5TjhWQ01VcUZFbmhGSXQ3bG5HOUF4M3hjTXVFY3FCdDdlYWdkY1ZW?=
 =?utf-8?Q?l1ryR7o+CNPmqw/nbBkFY5SbV?=
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-ExternalHop-MessageData-0: 
	Sfod7NJ8dyE97Tt4rqk1WUVrCLxrTRDZ/DnFceOBuMv6NTRBg9TCodalFNcW18G32WSR1xAp+V8j9r06nQZHDg6BeHVpSOP5GrYWa7cCKVEwT1lBthJb3Iz+TQgEsL7KfaC0Ecg1M85Jzuv9wAaJKnFxdc+t8kgDAw+nul62EgHq+N4MiItv/cAwEY3LDsgp1arCwCj5x3Wm0GjNUivtYLJgN3DpyHX5eZUCX4v1J70BL9lppTxuV4Q9oqUqvC6x1rMVbVwVbgf+jrgK1Gjb+ComBmxX8yM6d3f0KAWYssUak6iAmB4paICxaw/S4xDfaAQxjW8z5XnDHtp/Qu3X1JyS8dlcgAn+uvDmIWISxQJic3zk3WPJRUkaWMe0OZ2Olx/DeItMiGPAvnPcGcnaS+s1Ucf1gl3TuIr5oTdqRpKFmBdOQ1LLl9JtQ7OHAK6/hslL78WHCjQaqcGIQtL2tBchOcZtuzD3A/SJSMOwmSXbj/rhrTT8ahz6KneWfKwzOiitSeo7u55C/amvqTgdn0OjRlckCqA0z6Gtd0ra5+fELt/vtbz9nwC7x6gcy5J570wGwlRk/B4g7tN4X90Ef3rUlAe41W1x/QzgGd9Msdg=
X-OriginatorOrg: oracle.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 748d058a-6b27-4e1d-b3b9-08dc284ea5d0
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR10MB4429.namprd10.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Feb 2024 02:35:43.5652
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 4e2c6054-71cb-48f1-bd6c-3a9705aca71b
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: kbV/nfMu8rEH3lxkINatJD1ir0ap+Ks/Z0xrZ5iFeroRAXZ3VL61DfgE1KHDNsGRo7USUZmqg21OVXZD2rl/Qw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR10MB7246
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.272,Aquarius:18.0.1011,Hydra:6.0.619,FMLib:17.11.176.26
 definitions=2024-02-08_01,2024-02-07_01,2023-05-22_02
X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 malwarescore=0 bulkscore=0
 mlxlogscore=999 phishscore=0 spamscore=0 mlxscore=0 adultscore=0
 suspectscore=0 classifier=spam adjust=0 reason=mlx scancount=1
 engine=8.12.0-2311290000 definitions=main-2402080011
X-Proofpoint-GUID: 3b5TTjHCpiPV9L2gD6KeilHYzW6kamV7
X-Proofpoint-ORIG-GUID: 3b5TTjHCpiPV9L2gD6KeilHYzW6kamV7

On 12/14/2023 8:36 PM, Muchun Song wrote:

>
> On 2023/12/14 15:39, Nanyong Sun wrote:
>> Implement vmemmap_update_pmd and vmemmap_update_pte on arm64 to do
>> BBM(break-before-make) logic when change the page table of vmemmap
>> address, they will under the init_mm.page_table_lock.
>> If a translation fault of vmemmap address concurrently happened after
>> pte/pmd cleared, vmemmap page fault handler will acquire the
>> init_mm.page_table_lock to wait for vmemmap update to complete,
>> by then the virtual address is valid again, so PF can return and
>> access can continue.
>> In other case, do the traditional kernel fault.
>
> Yes. BTW, we already use the same scheme to support arm64
> in our internal production. So the whole approach LGTM.

Glad to hear that.  I have a question to both of you then.

How long does it take to allocate 3/4 of 1TB memory to hugetlb pool?  
and to free them?

thanks!

-jane

>
>> Implement flush_tlb_vmemmap_all and flush_tlb_vmemmap_range on arm64
>> with nothing to do because tlb already flushed in every single BBM.
>>
>> Signed-off-by: Nanyong Sun <sunnanyong@huawei.com>
>> ---
>>   arch/arm64/include/asm/esr.h |  4 ++
>>   arch/arm64/include/asm/mmu.h | 20 ++++++++
>>   arch/arm64/mm/fault.c        | 94 ++++++++++++++++++++++++++++++++++++
>>   arch/arm64/mm/mmu.c          | 28 +++++++++++
>>   4 files changed, 146 insertions(+)
>>
>> diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
>> index ae35939f395b..1c63256efd25 100644
>> --- a/arch/arm64/include/asm/esr.h
>> +++ b/arch/arm64/include/asm/esr.h
>> @@ -116,6 +116,10 @@
>>   #define ESR_ELx_FSC_SERROR    (0x11)
>>   #define ESR_ELx_FSC_ACCESS    (0x08)
>>   #define ESR_ELx_FSC_FAULT    (0x04)
>> +#define ESR_ELx_FSC_FAULT_L0    (0x04)
>> +#define ESR_ELx_FSC_FAULT_L1    (0x05)
>> +#define ESR_ELx_FSC_FAULT_L2    (0x06)
>> +#define ESR_ELx_FSC_FAULT_L3    (0x07)
>>   #define ESR_ELx_FSC_PERM    (0x0C)
>>   #define ESR_ELx_FSC_SEA_TTW0    (0x14)
>>   #define ESR_ELx_FSC_SEA_TTW1    (0x15)
>> diff --git a/arch/arm64/include/asm/mmu.h b/arch/arm64/include/asm/mmu.h
>> index 2fcf51231d6e..fcec5827f54f 100644
>> --- a/arch/arm64/include/asm/mmu.h
>> +++ b/arch/arm64/include/asm/mmu.h
>> @@ -76,5 +76,25 @@ extern bool kaslr_requires_kpti(void);
>>   #define INIT_MM_CONTEXT(name)    \
>>       .pgd = init_pg_dir,
>>   +#ifdef CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP
>> +void vmemmap_update_pmd(unsigned long start, pmd_t *pmd, pte_t 
>> *pgtable);
>> +#define vmemmap_update_pmd vmemmap_update_pmd
>> +void vmemmap_update_pte(unsigned long addr, pte_t *pte, pte_t entry);
>> +#define vmemmap_update_pte vmemmap_update_pte
>> +
>> +static inline void flush_tlb_vmemmap_all(void)
>> +{
>> +    /* do nothing, already flushed tlb in every single BBM */
>> +}
>> +#define flush_tlb_vmemmap_all flush_tlb_vmemmap_all
>> +
>> +static inline void flush_tlb_vmemmap_range(unsigned long start,
>> +                       unsigned long end)
>> +{
>> +    /* do nothing, already flushed tlb in every single BBM */
>> +}
>> +#define flush_tlb_vmemmap_range flush_tlb_vmemmap_range
>> +#endif
>> +
>>   #endif    /* !__ASSEMBLY__ */
>>   #endif
>> diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
>> index 460d799e1296..7066a273c1e0 100644
>> --- a/arch/arm64/mm/fault.c
>> +++ b/arch/arm64/mm/fault.c
>> @@ -368,6 +368,97 @@ static bool 
>> is_el1_mte_sync_tag_check_fault(unsigned long esr)
>>       return false;
>>   }
>>   +#ifdef CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP
>> +static inline bool is_vmemmap_address(unsigned long addr)
>> +{
>> +    return (addr >= VMEMMAP_START) && (addr < VMEMMAP_END);
>> +}
>> +
>> +static inline bool vmemmap_fault_may_fixup(unsigned long addr,
>> +                       unsigned long esr)
>> +{
>> +    if (!is_vmemmap_address(addr))
>> +        return false;
>> +
>> +    /*
>> +     * Only try to handle translation fault level 2 or level 3,
>> +     * because hugetlb vmemmap optimize only clear pmd or pte.
>> +     */
>> +    switch (esr & ESR_ELx_FSC) {
>> +    case ESR_ELx_FSC_FAULT_L2:
>> +    case ESR_ELx_FSC_FAULT_L3:
>> +        return true;
>> +    default:
>> +        return false;
>> +    }
>> +}
>> +
>> +/*
>> + * PMD mapped vmemmap should has been split as PTE mapped
>> + * by HVO now, here we only check this case, other cases
>> + * should fail.
>> + * Also should check the addr is healthy enough that will not cause
>> + * a level2 or level3 translation fault again after page fault
>> + * handled with success, so we need check both bits[1:0] of PMD and
>> + * PTE as ARM Spec mentioned below:
>> + * A Translation fault is generated if bits[1:0] of a translation
>> + * table descriptor identify the descriptor as either a Fault
>> + * encoding or a reserved encoding.
>> + */
>> +static inline bool vmemmap_addr_healthy(unsigned long addr)
>> +{
>> +    pgd_t *pgdp;
>> +    p4d_t *p4dp;
>> +    pud_t *pudp, pud;
>> +    pmd_t *pmdp, pmd;
>> +    pte_t *ptep, pte;
>> +
>> +    pgdp = pgd_offset_k(addr);
>> +    if (pgd_none(READ_ONCE(*pgdp)))
>> +        return false;
>> +
>> +    p4dp = p4d_offset(pgdp, addr);
>> +    if (p4d_none(READ_ONCE(*p4dp)))
>> +        return false;
>> +
>> +    pudp = pud_offset(p4dp, addr);
>> +    pud = READ_ONCE(*pudp);
>> +    if (pud_none(pud))
>> +        return false;
>> +
>> +    pmdp = pmd_offset(pudp, addr);
>
> We already make sure it is a translation fault of level 2 or 3
> here, so we could use pmd_offset_k() macro to simplify the code
> a little. Right?
>
>> +    pmd = READ_ONCE(*pmdp);
>> +    if (!pmd_table(pmd))
>> +        return false;
>> +
>> +    ptep = pte_offset_kernel(pmdp, addr);
>> +    pte = READ_ONCE(*ptep);
>
> Please use ptep_get (which is supposed to do this) to access the
> raw pte, see commit c33c794828f21217.
>
>> +    return (pte_val(pte) & PTE_TYPE_MASK) == PTE_TYPE_PAGE;
>> +}
>> +
>> +static bool vmemmap_handle_page_fault(unsigned long addr,
>> +                      unsigned long esr)
>> +{
>> +    bool ret = false;
>> +
>> +    if (likely(!vmemmap_fault_may_fixup(addr, esr)))
>> +        return false;
>> +
>> +    spin_lock(&init_mm.page_table_lock);
>> +    if (vmemmap_addr_healthy(addr))
>> +        ret = true;
>
> It is to assign the return value to ret directly. Like:
>
>         ret = vmemmap_addr_healthy(addr);
>
> The the initializetion to ret also can be dropped.
>
>> + spin_unlock(&init_mm.page_table_lock);
>> +
>> +    return ret;
>> +}
>> +#else
>> +static inline bool vmemmap_handle_page_fault(unsigned long addr,
>> +                         unsigned long esr)
>> +{
>> +    return false;
>> +}
>> +#endif /*CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP */
>           ^
> Miss a blank between "*" and "C" here.
>
> Thanks.
>
>> +
>>   static bool is_translation_fault(unsigned long esr)
>>   {
>>       return (esr & ESR_ELx_FSC_TYPE) == ESR_ELx_FSC_FAULT;
>> @@ -409,6 +500,9 @@ static void __do_kernel_fault(unsigned long addr, 
>> unsigned long esr,
>>               kfence_handle_page_fault(addr, esr & ESR_ELx_WNR, regs))
>>               return;
>>   +        if (vmemmap_handle_page_fault(addr, esr))
>> +            return;
>> +
>>           msg = "paging request";
>>       }
>>   diff --git a/arch/arm64/mm/mmu.c b/arch/arm64/mm/mmu.c
>> index 15f6347d23b6..81a600ccac7c 100644
>> --- a/arch/arm64/mm/mmu.c
>> +++ b/arch/arm64/mm/mmu.c
>> @@ -1146,6 +1146,34 @@ int __meminit vmemmap_check_pmd(pmd_t *pmdp, 
>> int node,
>>       return 1;
>>   }
>>   +#ifdef CONFIG_HUGETLB_PAGE_OPTIMIZE_VMEMMAP
>> +/*
>> + * In the window between the page table entry is cleared and filled
>> + * with a new value, other threads have the opportunity to concurrently
>> + * access the vmemmap area then page translation fault occur.
>> + * Therefore, we need to ensure that the init_mm.page_table_lock is 
>> held
>> + * to synchronize the vmemmap page fault handling which will wait for
>> + * this lock to be released to ensure that the page table entry has 
>> been
>> + * refreshed with a new valid value.
>> + */
>> +void vmemmap_update_pmd(unsigned long start, pmd_t *pmd, pte_t 
>> *pgtable)
>> +{
>> +    lockdep_assert_held(&init_mm.page_table_lock);
>> +    pmd_clear(pmd);
>> +    flush_tlb_kernel_range(start, start + PMD_SIZE);
>> +    pmd_populate_kernel(&init_mm, pmd, pgtable);
>> +}
>> +
>> +void vmemmap_update_pte(unsigned long addr, pte_t *pte, pte_t entry)
>> +{
>> +    spin_lock(&init_mm.page_table_lock);
>> +    pte_clear(&init_mm, addr, pte);
>> +    flush_tlb_kernel_range(addr, addr + PAGE_SIZE);
>> +    set_pte_at(&init_mm, addr, pte, entry);
>> +    spin_unlock(&init_mm.page_table_lock);
>> +}
>> +#endif
>> +
>>   int __meminit vmemmap_populate(unsigned long start, unsigned long 
>> end, int node,
>>           struct vmem_altmap *altmap)
>>   {
>
>