Message-ID: <7fa3a535-9b72-4538-a161-7e2940e61f3e@loongson.cn>
Date: Thu, 6 Nov 2025 17:02:52 +0800
From: Bibo Mao <maobibo@...ngson.cn>
To: Tianyang Zhang <zhangtianyang@...ngson.cn>,
 Huacai Chen <chenhuacai@...nel.org>
Cc: kernel@...0n.name, akpm@...ux-foundation.org, willy@...radead.org,
 david@...hat.com, linmag7@...il.com, thuth@...hat.com, apopple@...dia.com,
 loongarch@...ts.linux.dev, linux-kernel@...r.kernel.org,
 Liupu Wang <wangliupu@...ngson.cn>
Subject: Re: [PATCH] Loongarch:Make pte/pmd_modify can set _PAGE_MODIFIED



On 2025/11/6 下午4:50, Tianyang Zhang wrote:
> 
> 在 2025/11/6 上午10:10, Bibo Mao 写道:
>>
>>
>> On 2025/11/6 上午9:55, Tianyang Zhang wrote:
>>> Hi ,Bibao
>>>
>>> 在 2025/11/5 上午9:18, Bibo Mao 写道:
>>>>
>>>>
>>>> On 2025/11/5 上午9:07, Huacai Chen wrote:
>>>>> On Wed, Nov 5, 2025 at 8:57 AM Tianyang Zhang 
>>>>> <zhangtianyang@...ngson.cn> wrote:
>>>>>>
>>>>>> Hi, Huacai
>>>>>>
>>>>>> 在 2025/11/4 下午4:00, Huacai Chen 写道:
>>>>>>> Hi, Tianyang,
>>>>>>>
>>>>>>> The subject line can be:
>>>>>>> LoongArch: Let {pte,pmd}_modify() record the status of 
>>>>>>> _PAGE_DIRTY (If
>>>>>>> I'm right in the later comments).
>>>>>> Ok. I got it
>>>>>>>
>>>>>>> On Tue, Nov 4, 2025 at 3:30 PM Tianyang Zhang 
>>>>>>> <zhangtianyang@...ngson.cn> wrote:
>>>>>>>> In the current pte_modify operation, _PAGE_DIRTY might be 
>>>>>>>> cleared. Since
>>>>>>>> the hardware-page-walk does not have a predefined _PAGE_MODIFIED 
>>>>>>>> flag,
>>>>>>>> this could lead to loss of valid data in certain scenarios.
>>>>>>>>
>>>>>>>> The new modification involves checking whether the original PTE 
>>>>>>>> has the
>>>>>>>> _PAGE_DIRTY flag. If it exists, the _PAGE_MODIFIED bit is set, 
>>>>>>>> ensuring
>>>>>>>> that the pte_dirty interface can return accurate information.
>>>>>>> The description may be wrong here. Because pte_dirty() returns
>>>>>>> pte_val(pte) & (_PAGE_DIRTY | _PAGE_MODIFIED).
>>>>>>> If _PAGE_DIRTY isn't lost, pte_dirty() is always right, no matter
>>>>>>> whether there is or isn't _PAGE_MODIFIED.
>>>>>>>
>>>>>>> I think the real reason is we need to set _PAGE_MODIFIED in
>>>>>>> pte/pmd_modify to record the status of _PAGE_DIRTY, so that we can
>>>>>>> recover _PAGE_DIRTY afterwards, such as in pte/pmd_mkwrite().
>>>>>> Ok, I will adjust the description
>>>>> After some thinking, your original description may be right. Without
>>>>> this patch the scenario maybe like this:
>>>>> The pte is dirty _PAGE_DIRTY but without _PAGE_MODIFIED, after
>>>>> pte_modify() we lose _PAGE_DIRTY, then pte_dirty() returns false. So
>>>>> we need _PAGE_MODIFIED to record _PAGE_DIRTY here.
>>>> In theory pte_modify() is to modify RWX attribute. I think that it 
>>>> is a tricky to remove _PAGE_DIRTY and add _PAGE_MODIFIED with HW PTW 
>>>> system.
>>>>
>>>> Also _PAGE_ACCESSED is lost with pte_modify() API, is there any 
>>>> influence with HW PTW system, or wait until possible problems coming 
>>>> out.
>>>
>>> static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
>>> {
>>>          return __pte((pte_val(pte) & _PAGE_CHG_MASK) |
>>>                       (pgprot_val(newprot) & ~_PAGE_CHG_MASK));
>>> }
>>> In my understand, During the  pte_modify process, it is essential to 
>>> ensure that specific bits are inherited from the original PTE rather 
>>> than simply replaced(as set_pte),
>>>
>>> this guarantees the coherent operation of the memory management system.
>>>
>>> Since _PAGE_CHG_MASK explicitly requires preserving pte_modified, and 
>> The problem is how _PAGE_CHG_MASK should be defined, do you check with 
>> other architectures?
> 
> Under mainstream architectures(like ARM64/X86 ), if the pte_modify 
> interface clears the hard-dirty flag, it will set the soft-dirty flag 
> through some mechanism.
> 
> Thus, at least from the perspective of PAGE_DIRTY logic, this approach 
> is right.
> 
>>> there is an inherent correlation between pte_dirty and pte_modified, 
>>> these attributes must be evaluated and handled accordingly.
>>>
>>> The pte_valid attribute, being a hardware property, is inherently the 
>>> target of modification in the pte_modify interface. Therefore, it is 
>>> reasonable not to preserve it.
>> On HW PTW system, _PAGE_PRESENT will control whether trigger page 
>> fault rather than pte_valid/_PAGE_ACCESSED. For simple, do you think 
>> the following code is ok or not ?
> 
> "On HW PTW system, _PAGE_PRESENT will control whether trigger page fault 
> rather than pte_valid/_PAGE_ACCESSED"
> 
> Yes, indeed, in many cases, PAGE_PRESENT is precisely the cleanup target 
> of pte_modify.
Ok, I have no any question more :) Just do it.

Regards
Bibo Mao
> 
>>
>>  static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
>>  {
>> -       return __pte((pte_val(pte) & _PAGE_CHG_MASK) |
>> -                    (pgprot_val(newprot) & ~_PAGE_CHG_MASK));
>> +       unsigned long mask = _PAGE_CHG_MASK;
>> +
>> +       if (cpu_has_ptw)
>> +               mask |= _PAGE_DIRTY | _PAGE_ACCESSED;
>> +       return __pte((pte_val(pte) & mask) |
>> +                    (pgprot_val(newprot) & ~mask));
>>  }
>>
> This modification is inappropriate.
> 
> Firstly, _PAGE_ACCESSED(bit 0, as _PAGE_PRESENT) and _PAGE_DIRTY bits 
> are inherently the targets of pte_modify operations. Some 
> sub-memory-system like numa_balance precisely rely on
> 
> clearing these bits to trigger hardware exceptions and complete 
> subsequent processes, this appears to be unrelated to hardware-ptw
> 
> And, under hardware-ptw scenarios, the WRITE=0 && DIRTY=1 condition 
> should never occur, therefore, we cannot preserve the DIRTY bit in advance.
> 
> Thanks
> 
> Tianyang
> 
>> Regards
>> Bibo Mao
>>
>>>
>>> Thanks
>>>
>>> Tianyang
>>>
>>>>
>>>> Regards
>>>> Bibo Mao
>>>>>
>>>>> But the description also needs to be updated.
>>>>>
>>>>>>>
>>>>>>>> Co-developed-by: Liupu Wang <wangliupu@...ngson.cn>
>>>>>>>> Signed-off-by: Liupu Wang <wangliupu@...ngson.cn>
>>>>>>>> Signed-off-by: Tianyang Zhang <zhangtianyang@...ngson.cn>
>>>>>>>> ---
>>>>>>>>    arch/loongarch/include/asm/pgtable.h | 17 +++++++++++++----
>>>>>>>>    1 file changed, 13 insertions(+), 4 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/arch/loongarch/include/asm/pgtable.h 
>>>>>>>> b/arch/loongarch/include/asm/pgtable.h
>>>>>>>> index bd128696e96d..106abfa5183b 100644
>>>>>>>> --- a/arch/loongarch/include/asm/pgtable.h
>>>>>>>> +++ b/arch/loongarch/include/asm/pgtable.h
>>>>>>>> @@ -424,8 +424,13 @@ static inline unsigned long 
>>>>>>>> pte_accessible(struct mm_struct *mm, pte_t a)
>>>>>>>>
>>>>>>>>    static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
>>>>>>>>    {
>>>>>>>> -       return __pte((pte_val(pte) & _PAGE_CHG_MASK) |
>>>>>>>> -                    (pgprot_val(newprot) & ~_PAGE_CHG_MASK));
>>>>>>>> +       unsigned long val = (pte_val(pte) & _PAGE_CHG_MASK) |
>>>>>>>> +                    (pgprot_val(newprot) & ~_PAGE_CHG_MASK);
>>>>>>>> +
>>>>>>>> +       if (pte_val(pte) & _PAGE_DIRTY)
>>>>>>>> +               val |= _PAGE_MODIFIED;
>>>>>>>> +
>>>>>>>> +       return __pte(val);
>>>>>>>>    }
>>>>>>>>
>>>>>>>>    extern void __update_tlb(struct vm_area_struct *vma,
>>>>>>>> @@ -547,9 +552,13 @@ static inline struct page *pmd_page(pmd_t pmd)
>>>>>>>>
>>>>>>>>    static inline pmd_t pmd_modify(pmd_t pmd, pgprot_t newprot)
>>>>>>>>    {
>>>>>>>> -       pmd_val(pmd) = (pmd_val(pmd) & _HPAGE_CHG_MASK) |
>>>>>>>> +       unsigned long val = (pmd_val(pmd) & _HPAGE_CHG_MASK) |
>>>>>>>>                                   (pgprot_val(newprot) & 
>>>>>>>> ~_HPAGE_CHG_MASK);
>>>>>>>> -       return pmd;
>>>>>>>> +
>>>>>>>> +       if (pmd_val(pmd) & _PAGE_DIRTY)
>>>>>>>> +               val |= _PAGE_MODIFIED;
>>>>>>>> +
>>>>>>>> +       return __pmd(val);
>>>>>>>>    }
>>>>>>> A minimal modification can be:
>>>>>>> diff --git a/arch/loongarch/include/asm/pgtable.h
>>>>>>> b/arch/loongarch/include/asm/pgtable.h
>>>>>>> index 1f20e9280062..907ece0199e0 100644
>>>>>>> --- a/arch/loongarch/include/asm/pgtable.h
>>>>>>> +++ b/arch/loongarch/include/asm/pgtable.h
>>>>>>> @@ -448,8 +448,13 @@ static inline unsigned long 
>>>>>>> pte_accessible(struct
>>>>>>> mm_struct *mm, pte_t a)
>>>>>>>
>>>>>>>    static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
>>>>>>>    {
>>>>>>> -       return __pte((pte_val(pte) & _PAGE_CHG_MASK) |
>>>>>>> -                    (pgprot_val(newprot) & ~_PAGE_CHG_MASK));
>>>>>>> +       pte_val(pte) = (pte_val(pte) & _PAGE_CHG_MASK) |
>>>>>>> +                       (pgprot_val(newprot) & ~_PAGE_CHG_MASK);
>>>>>>> +
>>>>>>> +       if (pte_val(pte) & _PAGE_DIRTY)
>>>>>>> +               pte_val(pte) |= _PAGE_MODIFIED;
>>>>>>> +
>>>>>>> +       return pte;
>>>>>>>    }
>>>>>>
>>>>>> +       pte_val(pte) = (pte_val(pte) & _PAGE_CHG_MASK) |
>>>>>> +                       (pgprot_val(newprot) & ~_PAGE_CHG_MASK);
>>>>>>
>>>>>> After this step, _PAGE_DIRTY may have already disappeared,
>>>>>> If no new variables are added, they can be modified in follow way:
>>>>>>
>>>>>>    static inline pte_t pte_modify(pte_t pte, pgprot_t newprot)
>>>>>>    {
>>>>>> +       if (pte_val(pte) & _PAGE_DIRTY)
>>>>>> +               pte_val(pte) |= _PAGE_MODIFIED;
>>>>>> +
>>>>>>          return __pte((pte_val(pte) & _PAGE_CHG_MASK) |
>>>>>>           (pgprot_val(newprot) & ~_PAGE_CHG_MASK));
>>>>>>
>>>>>>    }
>>>>> OK, it makes sense.
>>>>>
>>>>> Huacai
>>>>>>
>>>>>>>
>>>>>>>    extern void __update_tlb(struct vm_area_struct *vma,
>>>>>>> @@ -583,7 +588,11 @@ static inline struct page *pmd_page(pmd_t pmd)
>>>>>>>    static inline pmd_t pmd_modify(pmd_t pmd, pgprot_t newprot)
>>>>>>>    {
>>>>>>>           pmd_val(pmd) = (pmd_val(pmd) & _HPAGE_CHG_MASK) |
>>>>>>> -                               (pgprot_val(newprot) & 
>>>>>>> ~_HPAGE_CHG_MASK);
>>>>>>> +                       (pgprot_val(newprot) & ~_HPAGE_CHG_MASK);
>>>>>>> +
>>>>>>> +       if (pmd_val(pmd) & _PAGE_DIRTY)
>>>>>>> +               pmd_val(pmd) |= _PAGE_MODIFIED;
>>>>>>> +
>>>>>>>           return pmd;
>>>>>>>    }
>>>>>>>
>>>>>>> You needn't define a new variable.
>>>>>>>
>>>>>>>
>>>>>>> Huacai
>>>>>>>
>>>>>>>>    static inline pmd_t pmd_mkinvalid(pmd_t pmd)
>>>>>>>> -- 
>>>>>>>> 2.41.0
>>>>>>>>
>>>>>>>>
>>>>>> Thanks
>>>>>>
>>>>>> Tianyang
>>>>>>
> 

Powered by blists - more mailing lists