Message-ID: <0d774995-c35c-60d2-32b8-91b32663b0c9@loongson.cn>
Date: Thu, 6 Nov 2025 16:50:18 +0800
From: Tianyang Zhang <zhangtianyang@...ngson.cn>
To: Bibo Mao <maobibo@...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


在 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.

>
>  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