[2/3] mm/huge_memory.c: update tlb entry if pmd is changed
diff mbox series

Message ID 1592990792-1923-2-git-send-email-maobibo@loongson.cn
State New
Headers show
Series
  • [1/3] mm: set page fault address for update_mmu_cache_pmd
Related show

Commit Message

maobibo June 24, 2020, 9:26 a.m. UTC
When set_pmd_at is called in function do_huge_pmd_anonymous_page,
new tlb entry can be added by software on MIPS platform.

Here add update_mmu_cache_pmd when pmd entry is set, and
update_mmu_cache_pmd is defined as empty excepts arc/mips platform.
This patch has no negative effect on other platforms except arc/mips
system.

Signed-off-by: Bibo Mao <maobibo@loongson.cn>
---
 mm/huge_memory.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Mike Kravetz June 25, 2020, 12:30 a.m. UTC | #1
On 6/24/20 2:26 AM, Bibo Mao wrote:
> When set_pmd_at is called in function do_huge_pmd_anonymous_page,
> new tlb entry can be added by software on MIPS platform.
> 
> Here add update_mmu_cache_pmd when pmd entry is set, and
> update_mmu_cache_pmd is defined as empty excepts arc/mips platform.
> This patch has no negative effect on other platforms except arc/mips
> system.

I am confused by this comment.  It appears that update_mmu_cache_pmd
is defined as non-empty on arc, mips, powerpc and sparc architectures.
Am I missing something?

If those architectures do provide update_mmu_cache_pmd, then the previous
patch and this one now call update_mmu_cache_pmd with the actual faulting
address instead of the huge page aligned address.  This was intentional
for mips.  However, are there any potential issues on the other architectures?
I am no expert in any of those architectures.  arc looks like it could be
problematic as update_mmu_cache_pmd calls update_mmu_cache and then
operates on (address & PAGE_MASK).  That could now be different.
maobibo June 25, 2020, 9:57 a.m. UTC | #2
On 06/25/2020 08:30 AM, Mike Kravetz wrote:
> On 6/24/20 2:26 AM, Bibo Mao wrote:
>> When set_pmd_at is called in function do_huge_pmd_anonymous_page,
>> new tlb entry can be added by software on MIPS platform.
>>
>> Here add update_mmu_cache_pmd when pmd entry is set, and
>> update_mmu_cache_pmd is defined as empty excepts arc/mips platform.
>> This patch has no negative effect on other platforms except arc/mips
>> system.
> 
> I am confused by this comment.  It appears that update_mmu_cache_pmd
> is defined as non-empty on arc, mips, powerpc and sparc architectures.
> Am I missing something?
ohh, sparc is missing here, it not defined as empty. On powerpc it is defined
as empty.

> 
> If those architectures do provide update_mmu_cache_pmd, then the previous
> patch and this one now call update_mmu_cache_pmd with the actual faulting
> address instead of the huge page aligned address.  This was intentional
> for mips.  However, are there any potential issues on the other architectures?
It is not special for mips, only that fault address is useful on mips system.
In function huge_pmd_set_accessed/do_huge_pmd_wp_page, update_mmu_cache_pmd is
called with vmf->address, rather than start address of pmd page.

regards
bibo,mao

> I am no expert in any of those architectures.  arc looks like it could be
> problematic as update_mmu_cache_pmd calls update_mmu_cache and then
> operates on (address & PAGE_MASK).  That could now be different.
Aneesh Kumar K.V June 25, 2020, 12:01 p.m. UTC | #3
Mike Kravetz <mike.kravetz@oracle.com> writes:

> On 6/24/20 2:26 AM, Bibo Mao wrote:
>> When set_pmd_at is called in function do_huge_pmd_anonymous_page,
>> new tlb entry can be added by software on MIPS platform.
>> 
>> Here add update_mmu_cache_pmd when pmd entry is set, and
>> update_mmu_cache_pmd is defined as empty excepts arc/mips platform.
>> This patch has no negative effect on other platforms except arc/mips
>> system.
>
> I am confused by this comment.  It appears that update_mmu_cache_pmd
> is defined as non-empty on arc, mips, powerpc and sparc architectures.
> Am I missing something?
>
> If those architectures do provide update_mmu_cache_pmd, then the previous
> patch and this one now call update_mmu_cache_pmd with the actual faulting
> address instead of the huge page aligned address.  This was intentional
> for mips.  However, are there any potential issues on the other architectures?
> I am no expert in any of those architectures.  arc looks like it could be
> problematic as update_mmu_cache_pmd calls update_mmu_cache and then
> operates on (address & PAGE_MASK).  That could now be different.
>

Also we added update_mmu_cache_pmd to update a THP entry. That could be
different from a hugetlb entry on some architectures. If we need to do
hugetlb equivalent for update_mmu_cache, we should add a different
function.

-aneesh
Mike Kravetz June 25, 2020, 4:46 p.m. UTC | #4
On 6/25/20 5:01 AM, Aneesh Kumar K.V wrote:
> Mike Kravetz <mike.kravetz@oracle.com> writes:
> 
>> On 6/24/20 2:26 AM, Bibo Mao wrote:
>>> When set_pmd_at is called in function do_huge_pmd_anonymous_page,
>>> new tlb entry can be added by software on MIPS platform.
>>>
>>> Here add update_mmu_cache_pmd when pmd entry is set, and
>>> update_mmu_cache_pmd is defined as empty excepts arc/mips platform.
>>> This patch has no negative effect on other platforms except arc/mips
>>> system.
>>
>> I am confused by this comment.  It appears that update_mmu_cache_pmd
>> is defined as non-empty on arc, mips, powerpc and sparc architectures.
>> Am I missing something?
>>
>> If those architectures do provide update_mmu_cache_pmd, then the previous
>> patch and this one now call update_mmu_cache_pmd with the actual faulting
>> address instead of the huge page aligned address.  This was intentional
>> for mips.  However, are there any potential issues on the other architectures?
>> I am no expert in any of those architectures.  arc looks like it could be
>> problematic as update_mmu_cache_pmd calls update_mmu_cache and then
>> operates on (address & PAGE_MASK).  That could now be different.
>>
> 
> Also we added update_mmu_cache_pmd to update a THP entry. That could be
> different from a hugetlb entry on some architectures. If we need to do
> hugetlb equivalent for update_mmu_cache, we should add a different
> function.

I do not know the mips architecture well enough or if the motivation for
this patch was based on THP or hugetlb pages.  However, it will change
the address passed to update_mmu_cache_pmd from huge page aligned to the
actual faulting address.  Will such a change in the passed address impact
the powerpc update_mmu_cache_pmd routine?
Aneesh Kumar K.V June 26, 2020, 8:13 a.m. UTC | #5
On 6/25/20 10:16 PM, Mike Kravetz wrote:
> On 6/25/20 5:01 AM, Aneesh Kumar K.V wrote:
>> Mike Kravetz <mike.kravetz@oracle.com> writes:
>>
>>> On 6/24/20 2:26 AM, Bibo Mao wrote:
>>>> When set_pmd_at is called in function do_huge_pmd_anonymous_page,
>>>> new tlb entry can be added by software on MIPS platform.
>>>>
>>>> Here add update_mmu_cache_pmd when pmd entry is set, and
>>>> update_mmu_cache_pmd is defined as empty excepts arc/mips platform.
>>>> This patch has no negative effect on other platforms except arc/mips
>>>> system.
>>>
>>> I am confused by this comment.  It appears that update_mmu_cache_pmd
>>> is defined as non-empty on arc, mips, powerpc and sparc architectures.
>>> Am I missing something?
>>>
>>> If those architectures do provide update_mmu_cache_pmd, then the previous
>>> patch and this one now call update_mmu_cache_pmd with the actual faulting
>>> address instead of the huge page aligned address.  This was intentional
>>> for mips.  However, are there any potential issues on the other architectures?
>>> I am no expert in any of those architectures.  arc looks like it could be
>>> problematic as update_mmu_cache_pmd calls update_mmu_cache and then
>>> operates on (address & PAGE_MASK).  That could now be different.
>>>
>>
>> Also we added update_mmu_cache_pmd to update a THP entry. That could be
>> different from a hugetlb entry on some architectures. If we need to do
>> hugetlb equivalent for update_mmu_cache, we should add a different
>> function.
> 
> I do not know the mips architecture well enough or if the motivation for
> this patch was based on THP or hugetlb pages.  However, it will change
> the address passed to update_mmu_cache_pmd from huge page aligned to the
> actual faulting address.  Will such a change in the passed address impact
> the powerpc update_mmu_cache_pmd routine?
> 

Right now powerpc update_mmu_cache_pmd() is a dummy function. But I 
agree we should audit arch to make sure such a change can work with 
architectures. My comment was related to the fact that mmu cache update 
w.r.t THP and hugetlb can be different on some platforms. So we may
want to avoid using the same function for both.

-aneesh
Andrew Morton Aug. 7, 2020, 4:35 a.m. UTC | #6
On Fri, 26 Jun 2020 13:43:06 +0530 "Aneesh Kumar K.V" <aneesh.kumar@linux.ibm.com> wrote:

> On 6/25/20 10:16 PM, Mike Kravetz wrote:
> > On 6/25/20 5:01 AM, Aneesh Kumar K.V wrote:
> >> Mike Kravetz <mike.kravetz@oracle.com> writes:
> >>
> >>> On 6/24/20 2:26 AM, Bibo Mao wrote:
> >>>> When set_pmd_at is called in function do_huge_pmd_anonymous_page,
> >>>> new tlb entry can be added by software on MIPS platform.
> >>>>
> >>>> Here add update_mmu_cache_pmd when pmd entry is set, and
> >>>> update_mmu_cache_pmd is defined as empty excepts arc/mips platform.
> >>>> This patch has no negative effect on other platforms except arc/mips
> >>>> system.
> >>>
> >>> I am confused by this comment.  It appears that update_mmu_cache_pmd
> >>> is defined as non-empty on arc, mips, powerpc and sparc architectures.
> >>> Am I missing something?
> >>>
> >>> If those architectures do provide update_mmu_cache_pmd, then the previous
> >>> patch and this one now call update_mmu_cache_pmd with the actual faulting
> >>> address instead of the huge page aligned address.  This was intentional
> >>> for mips.  However, are there any potential issues on the other architectures?
> >>> I am no expert in any of those architectures.  arc looks like it could be
> >>> problematic as update_mmu_cache_pmd calls update_mmu_cache and then
> >>> operates on (address & PAGE_MASK).  That could now be different.
> >>>
> >>
> >> Also we added update_mmu_cache_pmd to update a THP entry. That could be
> >> different from a hugetlb entry on some architectures. If we need to do
> >> hugetlb equivalent for update_mmu_cache, we should add a different
> >> function.
> > 
> > I do not know the mips architecture well enough or if the motivation for
> > this patch was based on THP or hugetlb pages.  However, it will change
> > the address passed to update_mmu_cache_pmd from huge page aligned to the
> > actual faulting address.  Will such a change in the passed address impact
> > the powerpc update_mmu_cache_pmd routine?
> > 
> 
> Right now powerpc update_mmu_cache_pmd() is a dummy function. But I 
> agree we should audit arch to make sure such a change can work with 
> architectures. My comment was related to the fact that mmu cache update 
> w.r.t THP and hugetlb can be different on some platforms. So we may
> want to avoid using the same function for both.

So I'll assume that this patch is stalled until such an audit has taken
place?

Patch
diff mbox series

diff --git a/mm/huge_memory.c b/mm/huge_memory.c
index 0f9187b..8b4ccf7 100644
--- a/mm/huge_memory.c
+++ b/mm/huge_memory.c
@@ -643,6 +643,7 @@  static vm_fault_t __do_huge_pmd_anonymous_page(struct vm_fault *vmf,
 		lru_cache_add_active_or_unevictable(page, vma);
 		pgtable_trans_huge_deposit(vma->vm_mm, vmf->pmd, pgtable);
 		set_pmd_at(vma->vm_mm, haddr, vmf->pmd, entry);
+		update_mmu_cache_pmd(vma, vmf->address, vmf->pmd);
 		add_mm_counter(vma->vm_mm, MM_ANONPAGES, HPAGE_PMD_NR);
 		mm_inc_nr_ptes(vma->vm_mm);
 		spin_unlock(vmf->ptl);
@@ -756,6 +757,7 @@  vm_fault_t do_huge_pmd_anonymous_page(struct vm_fault *vmf)
 			} else {
 				set_huge_zero_page(pgtable, vma->vm_mm, vma,
 						   haddr, vmf->pmd, zero_page);
+				update_mmu_cache_pmd(vma, vmf->address, vmf->pmd);
 				spin_unlock(vmf->ptl);
 				set = true;
 			}