2023-07-12 03:34:27

by maobibo

Subject: [PATCH 0/3] LoongArch: mm: Code cleanup with populate pte

There are some confusion between pdg and p4d when populate pte for
kernel address space. This patch modifies this issue and adds unified
function for pcpu and fixmap populate pte.

Bibo Mao (3):
mm/percpu: Remove some local variables in pcpu_populate_pte
LoongArch: Code cleanup in function pcpu_populate_pte
LoongArch: mm: Add unified function populate_kernel_pte

arch/loongarch/include/asm/pgalloc.h | 1 +
arch/loongarch/kernel/numa.c | 35 ++-----------------
arch/loongarch/mm/init.c | 52 ++++++++++++++++------------
mm/percpu.c | 24 +++++--------
4 files changed, 42 insertions(+), 70 deletions(-)

--
2.27.0

2023-07-12 03:41:33

by maobibo

Subject: [PATCH 3/3] LoongArch: mm: Add unified function populate_kernel_pte

Function pcpu_populate_pte and fixmap_pte are similar, they populate
one page from kernel address space. And there is confusion between
pgd and p4d in function fixmap_pte, such as pgd_none always returns
zero. This patch adds unified function populate_kernel_pte and replaces
pcpu_populate_pte and fixmap_pte.

Signed-off-by: Bibo Mao <[email protected]>
---
arch/loongarch/include/asm/pgalloc.h | 1 +
arch/loongarch/kernel/numa.c | 40 +--------------------
arch/loongarch/mm/init.c | 52 ++++++++++++++++------------
3 files changed, 32 insertions(+), 61 deletions(-)

diff --git a/arch/loongarch/include/asm/pgalloc.h b/arch/loongarch/include/asm/pgalloc.h
index af1d1e4a6965..ca17b573dba6 100644
--- a/arch/loongarch/include/asm/pgalloc.h
+++ b/arch/loongarch/include/asm/pgalloc.h
@@ -91,4 +91,5 @@ static inline pud_t *pud_alloc_one(struct mm_struct *mm, unsigned long address)

#endif /* __PAGETABLE_PUD_FOLDED */

+extern pte_t * __init populate_kernel_pte(unsigned long addr);
#endif /* _ASM_PGALLOC_H */
diff --git a/arch/loongarch/kernel/numa.c b/arch/loongarch/kernel/numa.c
index 778e1c20bfb0..24a693b76873 100644
--- a/arch/loongarch/kernel/numa.c
+++ b/arch/loongarch/kernel/numa.c
@@ -67,46 +67,8 @@ static int __init pcpu_cpu_distance(unsigned int from, unsigned int to)

void __init pcpu_populate_pte(unsigned long addr)
{
- pgd_t *pgd = pgd_offset_k(addr);
- p4d_t *p4d = p4d_offset(pgd, addr);
- pud_t *pud;
- pmd_t *pmd;
-
- if (p4d_none(*p4d)) {
- pud = memblock_alloc_raw(PAGE_SIZE, PAGE_SIZE);
- if (!pud)
- goto err_alloc;
- p4d_populate(&init_mm, p4d, pud);
-#ifndef __PAGETABLE_PUD_FOLDED
- pud_init(pud);
-#endif
- }
-
- pud = pud_offset(p4d, addr);
- if (pud_none(*pud)) {
- pmd = memblock_alloc_raw(PAGE_SIZE, PAGE_SIZE);
- if (!pmd)
- goto err_alloc;
- pud_populate(&init_mm, pud, pmd);
-#ifndef __PAGETABLE_PMD_FOLDED
- pmd_init(pmd);
-#endif
- }
-
- pmd = pmd_offset(pud, addr);
- if (!pmd_present(*pmd)) {
- pte_t *pte;
-
- pte = memblock_alloc(PAGE_SIZE, PAGE_SIZE);
- if (!pte)
- goto err_alloc;
- pmd_populate_kernel(&init_mm, pmd, pte);
- }
-
+ populate_kernel_pte(addr);
return;
-
-err_alloc:
- panic("%s: Failed to allocate memory\n", __func__);
}

void __init setup_per_cpu_areas(void)
diff --git a/arch/loongarch/mm/init.c b/arch/loongarch/mm/init.c
index 3b7d8129570b..6cd2948373ae 100644
--- a/arch/loongarch/mm/init.c
+++ b/arch/loongarch/mm/init.c
@@ -191,46 +191,49 @@ void vmemmap_free(unsigned long start, unsigned long end, struct vmem_altmap *al
#endif
#endif

-static pte_t *fixmap_pte(unsigned long addr)
+pte_t * __init populate_kernel_pte(unsigned long addr)
{
- pgd_t *pgd;
- p4d_t *p4d;
+ pgd_t *pgd = pgd_offset_k(addr);
+ p4d_t *p4d = p4d_offset(pgd, addr);
pud_t *pud;
pmd_t *pmd;

- pgd = pgd_offset_k(addr);
- p4d = p4d_offset(pgd, addr);
-
- if (pgd_none(*pgd)) {
- pud_t *new __maybe_unused;
-
- new = memblock_alloc_low(PAGE_SIZE, PAGE_SIZE);
- pgd_populate(&init_mm, pgd, new);
+ if (p4d_none(*p4d)) {
+ pud = memblock_alloc_raw(PAGE_SIZE, PAGE_SIZE);
+ if (!pud)
+ goto err_alloc;
+ p4d_populate(&init_mm, p4d, pud);
#ifndef __PAGETABLE_PUD_FOLDED
- pud_init(new);
+ pud_init(pud);
#endif
}

pud = pud_offset(p4d, addr);
if (pud_none(*pud)) {
- pmd_t *new __maybe_unused;
-
- new = memblock_alloc_low(PAGE_SIZE, PAGE_SIZE);
- pud_populate(&init_mm, pud, new);
+ pmd = memblock_alloc_raw(PAGE_SIZE, PAGE_SIZE);
+ if (!pmd)
+ goto err_alloc;
+ pud_populate(&init_mm, pud, pmd);
#ifndef __PAGETABLE_PMD_FOLDED
- pmd_init(new);
+ pmd_init(pmd);
#endif
}

pmd = pmd_offset(pud, addr);
- if (pmd_none(*pmd)) {
- pte_t *new __maybe_unused;
+ if (!pmd_present(*pmd)) {
+ pte_t *pte;

- new = memblock_alloc_low(PAGE_SIZE, PAGE_SIZE);
- pmd_populate_kernel(&init_mm, pmd, new);
+ pte = memblock_alloc(PAGE_SIZE, PAGE_SIZE);
+ if (!pte)
+ goto err_alloc;
+ pmd_populate_kernel(&init_mm, pmd, pte);
}

return pte_offset_kernel(pmd, addr);
+
+err_alloc:
+ panic("%s: Failed to allocate memory\n", __func__);
+ return NULL;
}

void __init __set_fixmap(enum fixed_addresses idx,
@@ -241,7 +244,12 @@ void __init __set_fixmap(enum fixed_addresses idx,

BUG_ON(idx <= FIX_HOLE || idx >= __end_of_fixed_addresses);

- ptep = fixmap_pte(addr);
+ /*
+ * Now only FIX_EARLYCON_MEM_BASE fixed map is used
+ * __set_fixmap must be called before mem_init since function
+ * populate_kernel_pte allocates memory with memblock_alloc method.
+ */
+ ptep = populate_kernel_pte(addr);
if (!pte_none(*ptep)) {
pte_ERROR(*ptep);
return;
--
2.27.0

2023-07-25 00:52:44

by maobibo

Subject: Re: [PATCH 0/3] LoongArch: mm: Code cleanup with populate pte

slightly ping.

在 2023/7/12 11:16, Bibo Mao 写道:
> There are some confusion between pdg and p4d when populate pte for
> kernel address space. This patch modifies this issue and adds unified
> function for pcpu and fixmap populate pte.
>
> Bibo Mao (3):
> mm/percpu: Remove some local variables in pcpu_populate_pte
> LoongArch: Code cleanup in function pcpu_populate_pte
> LoongArch: mm: Add unified function populate_kernel_pte
>
> arch/loongarch/include/asm/pgalloc.h | 1 +
> arch/loongarch/kernel/numa.c | 35 ++-----------------
> arch/loongarch/mm/init.c | 52 ++++++++++++++++------------
> mm/percpu.c | 24 +++++--------
> 4 files changed, 42 insertions(+), 70 deletions(-)
>

2023-07-25 02:46:57

by Dennis Zhou

Subject: Re: [PATCH 0/3] LoongArch: mm: Code cleanup with populate pte

Hello,

On Tue, Jul 25, 2023 at 08:36:22AM +0800, bibo mao wrote:
> slightly ping.
>

Sorry, I'm not sure how I missed this. I'll take a look at this
tomorrow.

Thanks,
Dennis

> 在 2023/7/12 11:16, Bibo Mao 写道:
> > There are some confusion between pdg and p4d when populate pte for
> > kernel address space. This patch modifies this issue and adds unified
> > function for pcpu and fixmap populate pte.
> >
> > Bibo Mao (3):
> > mm/percpu: Remove some local variables in pcpu_populate_pte
> > LoongArch: Code cleanup in function pcpu_populate_pte
> > LoongArch: mm: Add unified function populate_kernel_pte
> >
> > arch/loongarch/include/asm/pgalloc.h | 1 +
> > arch/loongarch/kernel/numa.c | 35 ++-----------------
> > arch/loongarch/mm/init.c | 52 ++++++++++++++++------------
> > mm/percpu.c | 24 +++++--------
> > 4 files changed, 42 insertions(+), 70 deletions(-)
> >
>

2023-07-31 14:41:06

by Huacai Chen

Subject: Re: [PATCH 3/3] LoongArch: mm: Add unified function populate_kernel_pte

On Wed, Jul 12, 2023 at 11:16 AM Bibo Mao <[email protected]> wrote:
>
> Function pcpu_populate_pte and fixmap_pte are similar, they populate
> one page from kernel address space. And there is confusion between
> pgd and p4d in function fixmap_pte, such as pgd_none always returns
> zero. This patch adds unified function populate_kernel_pte and replaces
> pcpu_populate_pte and fixmap_pte.
>
> Signed-off-by: Bibo Mao <[email protected]>
> ---
> arch/loongarch/include/asm/pgalloc.h | 1 +
> arch/loongarch/kernel/numa.c | 40 +--------------------
> arch/loongarch/mm/init.c | 52 ++++++++++++++++------------
> 3 files changed, 32 insertions(+), 61 deletions(-)
>
> diff --git a/arch/loongarch/include/asm/pgalloc.h b/arch/loongarch/include/asm/pgalloc.h
> index af1d1e4a6965..ca17b573dba6 100644
> --- a/arch/loongarch/include/asm/pgalloc.h
> +++ b/arch/loongarch/include/asm/pgalloc.h
> @@ -91,4 +91,5 @@ static inline pud_t *pud_alloc_one(struct mm_struct *mm, unsigned long address)
>
> #endif /* __PAGETABLE_PUD_FOLDED */
>
> +extern pte_t * __init populate_kernel_pte(unsigned long addr);
> #endif /* _ASM_PGALLOC_H */
> diff --git a/arch/loongarch/kernel/numa.c b/arch/loongarch/kernel/numa.c
> index 778e1c20bfb0..24a693b76873 100644
> --- a/arch/loongarch/kernel/numa.c
> +++ b/arch/loongarch/kernel/numa.c
> @@ -67,46 +67,8 @@ static int __init pcpu_cpu_distance(unsigned int from, unsigned int to)
>
> void __init pcpu_populate_pte(unsigned long addr)
> {
> - pgd_t *pgd = pgd_offset_k(addr);
> - p4d_t *p4d = p4d_offset(pgd, addr);
> - pud_t *pud;
> - pmd_t *pmd;
> -
> - if (p4d_none(*p4d)) {
> - pud = memblock_alloc_raw(PAGE_SIZE, PAGE_SIZE);
> - if (!pud)
> - goto err_alloc;
> - p4d_populate(&init_mm, p4d, pud);
> -#ifndef __PAGETABLE_PUD_FOLDED
> - pud_init(pud);
> -#endif
> - }
> -
> - pud = pud_offset(p4d, addr);
> - if (pud_none(*pud)) {
> - pmd = memblock_alloc_raw(PAGE_SIZE, PAGE_SIZE);
> - if (!pmd)
> - goto err_alloc;
> - pud_populate(&init_mm, pud, pmd);
> -#ifndef __PAGETABLE_PMD_FOLDED
> - pmd_init(pmd);
> -#endif
> - }
> -
> - pmd = pmd_offset(pud, addr);
> - if (!pmd_present(*pmd)) {
> - pte_t *pte;
> -
> - pte = memblock_alloc(PAGE_SIZE, PAGE_SIZE);
> - if (!pte)
> - goto err_alloc;
> - pmd_populate_kernel(&init_mm, pmd, pte);
> - }
> -
> + populate_kernel_pte(addr);
> return;
> -
> -err_alloc:
> - panic("%s: Failed to allocate memory\n", __func__);
> }
>
> void __init setup_per_cpu_areas(void)
> diff --git a/arch/loongarch/mm/init.c b/arch/loongarch/mm/init.c
> index 3b7d8129570b..6cd2948373ae 100644
> --- a/arch/loongarch/mm/init.c
> +++ b/arch/loongarch/mm/init.c
> @@ -191,46 +191,49 @@ void vmemmap_free(unsigned long start, unsigned long end, struct vmem_altmap *al
> #endif
> #endif
>
> -static pte_t *fixmap_pte(unsigned long addr)
> +pte_t * __init populate_kernel_pte(unsigned long addr)
> {
> - pgd_t *pgd;
> - p4d_t *p4d;
> + pgd_t *pgd = pgd_offset_k(addr);
> + p4d_t *p4d = p4d_offset(pgd, addr);
> pud_t *pud;
> pmd_t *pmd;
>
> - pgd = pgd_offset_k(addr);
> - p4d = p4d_offset(pgd, addr);
> -
> - if (pgd_none(*pgd)) {
> - pud_t *new __maybe_unused;
> -
> - new = memblock_alloc_low(PAGE_SIZE, PAGE_SIZE);
> - pgd_populate(&init_mm, pgd, new);
> + if (p4d_none(*p4d)) {
> + pud = memblock_alloc_raw(PAGE_SIZE, PAGE_SIZE);
> + if (!pud)
> + goto err_alloc;
> + p4d_populate(&init_mm, p4d, pud);
> #ifndef __PAGETABLE_PUD_FOLDED
> - pud_init(new);
> + pud_init(pud);
> #endif
> }
>
> pud = pud_offset(p4d, addr);
> if (pud_none(*pud)) {
> - pmd_t *new __maybe_unused;
> -
> - new = memblock_alloc_low(PAGE_SIZE, PAGE_SIZE);
> - pud_populate(&init_mm, pud, new);
> + pmd = memblock_alloc_raw(PAGE_SIZE, PAGE_SIZE);
> + if (!pmd)
> + goto err_alloc;
> + pud_populate(&init_mm, pud, pmd);
> #ifndef __PAGETABLE_PMD_FOLDED
> - pmd_init(new);
> + pmd_init(pmd);
> #endif
> }
>
> pmd = pmd_offset(pud, addr);
> - if (pmd_none(*pmd)) {
> - pte_t *new __maybe_unused;
> + if (!pmd_present(*pmd)) {
> + pte_t *pte;
>
> - new = memblock_alloc_low(PAGE_SIZE, PAGE_SIZE);
> - pmd_populate_kernel(&init_mm, pmd, new);
> + pte = memblock_alloc(PAGE_SIZE, PAGE_SIZE);
I don't think memblock_alloc_low() here can be replaced by memblock_alloc().


Huacai
> + if (!pte)
> + goto err_alloc;
> + pmd_populate_kernel(&init_mm, pmd, pte);
> }
>
> return pte_offset_kernel(pmd, addr);
> +
> +err_alloc:
> + panic("%s: Failed to allocate memory\n", __func__);
> + return NULL;
> }
>
> void __init __set_fixmap(enum fixed_addresses idx,
> @@ -241,7 +244,12 @@ void __init __set_fixmap(enum fixed_addresses idx,
>
> BUG_ON(idx <= FIX_HOLE || idx >= __end_of_fixed_addresses);
>
> - ptep = fixmap_pte(addr);
> + /*
> + * Now only FIX_EARLYCON_MEM_BASE fixed map is used
> + * __set_fixmap must be called before mem_init since function
> + * populate_kernel_pte allocates memory with memblock_alloc method.
> + */
> + ptep = populate_kernel_pte(addr);
> if (!pte_none(*ptep)) {
> pte_ERROR(*ptep);
> return;
> --
> 2.27.0
>

2023-08-01 01:33:59

by maobibo

Subject: Re: [PATCH 3/3] LoongArch: mm: Add unified function populate_kernel_pte

在 2023/7/31 22:15, Huacai Chen 写道:
> On Wed, Jul 12, 2023 at 11:16 AM Bibo Mao <[email protected]> wrote:
>>
>> Function pcpu_populate_pte and fixmap_pte are similar, they populate
>> one page from kernel address space. And there is confusion between
>> pgd and p4d in function fixmap_pte, such as pgd_none always returns
>> zero. This patch adds unified function populate_kernel_pte and replaces
>> pcpu_populate_pte and fixmap_pte.
>>
>> Signed-off-by: Bibo Mao <[email protected]>
>> ---
>> arch/loongarch/include/asm/pgalloc.h | 1 +
>> arch/loongarch/kernel/numa.c | 40 +--------------------
>> arch/loongarch/mm/init.c | 52 ++++++++++++++++------------
>> 3 files changed, 32 insertions(+), 61 deletions(-)
>>
>> diff --git a/arch/loongarch/include/asm/pgalloc.h b/arch/loongarch/include/asm/pgalloc.h
>> index af1d1e4a6965..ca17b573dba6 100644
>> --- a/arch/loongarch/include/asm/pgalloc.h
>> +++ b/arch/loongarch/include/asm/pgalloc.h
>> @@ -91,4 +91,5 @@ static inline pud_t *pud_alloc_one(struct mm_struct *mm, unsigned long address)
>>
>> #endif /* __PAGETABLE_PUD_FOLDED */
>>
>> +extern pte_t * __init populate_kernel_pte(unsigned long addr);
>> #endif /* _ASM_PGALLOC_H */
>> diff --git a/arch/loongarch/kernel/numa.c b/arch/loongarch/kernel/numa.c
>> index 778e1c20bfb0..24a693b76873 100644
>> --- a/arch/loongarch/kernel/numa.c
>> +++ b/arch/loongarch/kernel/numa.c
>> @@ -67,46 +67,8 @@ static int __init pcpu_cpu_distance(unsigned int from, unsigned int to)
>>
>> void __init pcpu_populate_pte(unsigned long addr)
>> {
>> - pgd_t *pgd = pgd_offset_k(addr);
>> - p4d_t *p4d = p4d_offset(pgd, addr);
>> - pud_t *pud;
>> - pmd_t *pmd;
>> -
>> - if (p4d_none(*p4d)) {
>> - pud = memblock_alloc_raw(PAGE_SIZE, PAGE_SIZE);
>> - if (!pud)
>> - goto err_alloc;
>> - p4d_populate(&init_mm, p4d, pud);
>> -#ifndef __PAGETABLE_PUD_FOLDED
>> - pud_init(pud);
>> -#endif
>> - }
>> -
>> - pud = pud_offset(p4d, addr);
>> - if (pud_none(*pud)) {
>> - pmd = memblock_alloc_raw(PAGE_SIZE, PAGE_SIZE);
>> - if (!pmd)
>> - goto err_alloc;
>> - pud_populate(&init_mm, pud, pmd);
>> -#ifndef __PAGETABLE_PMD_FOLDED
>> - pmd_init(pmd);
>> -#endif
>> - }
>> -
>> - pmd = pmd_offset(pud, addr);
>> - if (!pmd_present(*pmd)) {
>> - pte_t *pte;
>> -
>> - pte = memblock_alloc(PAGE_SIZE, PAGE_SIZE);
>> - if (!pte)
>> - goto err_alloc;
>> - pmd_populate_kernel(&init_mm, pmd, pte);
>> - }
>> -
>> + populate_kernel_pte(addr);
>> return;
>> -
>> -err_alloc:
>> - panic("%s: Failed to allocate memory\n", __func__);
>> }
>>
>> void __init setup_per_cpu_areas(void)
>> diff --git a/arch/loongarch/mm/init.c b/arch/loongarch/mm/init.c
>> index 3b7d8129570b..6cd2948373ae 100644
>> --- a/arch/loongarch/mm/init.c
>> +++ b/arch/loongarch/mm/init.c
>> @@ -191,46 +191,49 @@ void vmemmap_free(unsigned long start, unsigned long end, struct vmem_altmap *al
>> #endif
>> #endif
>>
>> -static pte_t *fixmap_pte(unsigned long addr)
>> +pte_t * __init populate_kernel_pte(unsigned long addr)
>> {
>> - pgd_t *pgd;
>> - p4d_t *p4d;
>> + pgd_t *pgd = pgd_offset_k(addr);
>> + p4d_t *p4d = p4d_offset(pgd, addr);
>> pud_t *pud;
>> pmd_t *pmd;
>>
>> - pgd = pgd_offset_k(addr);
>> - p4d = p4d_offset(pgd, addr);
>> -
>> - if (pgd_none(*pgd)) {
>> - pud_t *new __maybe_unused;
>> -
>> - new = memblock_alloc_low(PAGE_SIZE, PAGE_SIZE);
>> - pgd_populate(&init_mm, pgd, new);
>> + if (p4d_none(*p4d)) {
>> + pud = memblock_alloc_raw(PAGE_SIZE, PAGE_SIZE);
>> + if (!pud)
>> + goto err_alloc;
>> + p4d_populate(&init_mm, p4d, pud);
>> #ifndef __PAGETABLE_PUD_FOLDED
>> - pud_init(new);
>> + pud_init(pud);
>> #endif
>> }
>>
>> pud = pud_offset(p4d, addr);
>> if (pud_none(*pud)) {
>> - pmd_t *new __maybe_unused;
>> -
>> - new = memblock_alloc_low(PAGE_SIZE, PAGE_SIZE);
>> - pud_populate(&init_mm, pud, new);
>> + pmd = memblock_alloc_raw(PAGE_SIZE, PAGE_SIZE);
>> + if (!pmd)
>> + goto err_alloc;
>> + pud_populate(&init_mm, pud, pmd);
>> #ifndef __PAGETABLE_PMD_FOLDED
>> - pmd_init(new);
>> + pmd_init(pmd);
>> #endif
>> }
>>
>> pmd = pmd_offset(pud, addr);
>> - if (pmd_none(*pmd)) {
>> - pte_t *new __maybe_unused;
>> + if (!pmd_present(*pmd)) {
>> + pte_t *pte;
>>
>> - new = memblock_alloc_low(PAGE_SIZE, PAGE_SIZE);
>> - pmd_populate_kernel(&init_mm, pmd, new);
>> + pte = memblock_alloc(PAGE_SIZE, PAGE_SIZE);
> I don't think memblock_alloc_low() here can be replaced by memblock_alloc().
Can you share me the points that pte table must be allocated with memblock_alloc_low
in this place?

Regards
Bibo Mao
>
>
> Huacai
>> + if (!pte)
>> + goto err_alloc;
>> + pmd_populate_kernel(&init_mm, pmd, pte);
>> }
>>
>> return pte_offset_kernel(pmd, addr);
>> +
>> +err_alloc:
>> + panic("%s: Failed to allocate memory\n", __func__);
>> + return NULL;
>> }
>>
>> void __init __set_fixmap(enum fixed_addresses idx,
>> @@ -241,7 +244,12 @@ void __init __set_fixmap(enum fixed_addresses idx,
>>
>> BUG_ON(idx <= FIX_HOLE || idx >= __end_of_fixed_addresses);
>>
>> - ptep = fixmap_pte(addr);
>> + /*
>> + * Now only FIX_EARLYCON_MEM_BASE fixed map is used
>> + * __set_fixmap must be called before mem_init since function
>> + * populate_kernel_pte allocates memory with memblock_alloc method.
>> + */
>> + ptep = populate_kernel_pte(addr);
>> if (!pte_none(*ptep)) {
>> pte_ERROR(*ptep);
>> return;
>> --
>> 2.27.0
>>

2023-08-02 07:38:03

by Huacai Chen

Subject: Re: [PATCH 3/3] LoongArch: mm: Add unified function populate_kernel_pte

On Tue, Aug 1, 2023 at 9:22 AM bibo mao <[email protected]> wrote:
>
>
>
> 在 2023/7/31 22:15, Huacai Chen 写道:
> > On Wed, Jul 12, 2023 at 11:16 AM Bibo Mao <[email protected]> wrote:
> >>
> >> Function pcpu_populate_pte and fixmap_pte are similar, they populate
> >> one page from kernel address space. And there is confusion between
> >> pgd and p4d in function fixmap_pte, such as pgd_none always returns
> >> zero. This patch adds unified function populate_kernel_pte and replaces
> >> pcpu_populate_pte and fixmap_pte.
> >>
> >> Signed-off-by: Bibo Mao <[email protected]>
> >> ---
> >> arch/loongarch/include/asm/pgalloc.h | 1 +
> >> arch/loongarch/kernel/numa.c | 40 +--------------------
> >> arch/loongarch/mm/init.c | 52 ++++++++++++++++------------
> >> 3 files changed, 32 insertions(+), 61 deletions(-)
> >>
> >> diff --git a/arch/loongarch/include/asm/pgalloc.h b/arch/loongarch/include/asm/pgalloc.h
> >> index af1d1e4a6965..ca17b573dba6 100644
> >> --- a/arch/loongarch/include/asm/pgalloc.h
> >> +++ b/arch/loongarch/include/asm/pgalloc.h
> >> @@ -91,4 +91,5 @@ static inline pud_t *pud_alloc_one(struct mm_struct *mm, unsigned long address)
> >>
> >> #endif /* __PAGETABLE_PUD_FOLDED */
> >>
> >> +extern pte_t * __init populate_kernel_pte(unsigned long addr);
> >> #endif /* _ASM_PGALLOC_H */
> >> diff --git a/arch/loongarch/kernel/numa.c b/arch/loongarch/kernel/numa.c
> >> index 778e1c20bfb0..24a693b76873 100644
> >> --- a/arch/loongarch/kernel/numa.c
> >> +++ b/arch/loongarch/kernel/numa.c
> >> @@ -67,46 +67,8 @@ static int __init pcpu_cpu_distance(unsigned int from, unsigned int to)
> >>
> >> void __init pcpu_populate_pte(unsigned long addr)
> >> {
> >> - pgd_t *pgd = pgd_offset_k(addr);
> >> - p4d_t *p4d = p4d_offset(pgd, addr);
> >> - pud_t *pud;
> >> - pmd_t *pmd;
> >> -
> >> - if (p4d_none(*p4d)) {
> >> - pud = memblock_alloc_raw(PAGE_SIZE, PAGE_SIZE);
> >> - if (!pud)
> >> - goto err_alloc;
> >> - p4d_populate(&init_mm, p4d, pud);
> >> -#ifndef __PAGETABLE_PUD_FOLDED
> >> - pud_init(pud);
> >> -#endif
> >> - }
> >> -
> >> - pud = pud_offset(p4d, addr);
> >> - if (pud_none(*pud)) {
> >> - pmd = memblock_alloc_raw(PAGE_SIZE, PAGE_SIZE);
> >> - if (!pmd)
> >> - goto err_alloc;
> >> - pud_populate(&init_mm, pud, pmd);
> >> -#ifndef __PAGETABLE_PMD_FOLDED
> >> - pmd_init(pmd);
> >> -#endif
> >> - }
> >> -
> >> - pmd = pmd_offset(pud, addr);
> >> - if (!pmd_present(*pmd)) {
> >> - pte_t *pte;
> >> -
> >> - pte = memblock_alloc(PAGE_SIZE, PAGE_SIZE);
> >> - if (!pte)
> >> - goto err_alloc;
> >> - pmd_populate_kernel(&init_mm, pmd, pte);
> >> - }
> >> -
> >> + populate_kernel_pte(addr);
> >> return;
> >> -
> >> -err_alloc:
> >> - panic("%s: Failed to allocate memory\n", __func__);
> >> }
> >>
> >> void __init setup_per_cpu_areas(void)
> >> diff --git a/arch/loongarch/mm/init.c b/arch/loongarch/mm/init.c
> >> index 3b7d8129570b..6cd2948373ae 100644
> >> --- a/arch/loongarch/mm/init.c
> >> +++ b/arch/loongarch/mm/init.c
> >> @@ -191,46 +191,49 @@ void vmemmap_free(unsigned long start, unsigned long end, struct vmem_altmap *al
> >> #endif
> >> #endif
> >>
> >> -static pte_t *fixmap_pte(unsigned long addr)
> >> +pte_t * __init populate_kernel_pte(unsigned long addr)
> >> {
> >> - pgd_t *pgd;
> >> - p4d_t *p4d;
> >> + pgd_t *pgd = pgd_offset_k(addr);
> >> + p4d_t *p4d = p4d_offset(pgd, addr);
> >> pud_t *pud;
> >> pmd_t *pmd;
> >>
> >> - pgd = pgd_offset_k(addr);
> >> - p4d = p4d_offset(pgd, addr);
> >> -
> >> - if (pgd_none(*pgd)) {
> >> - pud_t *new __maybe_unused;
> >> -
> >> - new = memblock_alloc_low(PAGE_SIZE, PAGE_SIZE);
> >> - pgd_populate(&init_mm, pgd, new);
> >> + if (p4d_none(*p4d)) {
> >> + pud = memblock_alloc_raw(PAGE_SIZE, PAGE_SIZE);
> >> + if (!pud)
> >> + goto err_alloc;
> >> + p4d_populate(&init_mm, p4d, pud);
> >> #ifndef __PAGETABLE_PUD_FOLDED
> >> - pud_init(new);
> >> + pud_init(pud);
> >> #endif
> >> }
> >>
> >> pud = pud_offset(p4d, addr);
> >> if (pud_none(*pud)) {
> >> - pmd_t *new __maybe_unused;
> >> -
> >> - new = memblock_alloc_low(PAGE_SIZE, PAGE_SIZE);
> >> - pud_populate(&init_mm, pud, new);
> >> + pmd = memblock_alloc_raw(PAGE_SIZE, PAGE_SIZE);
> >> + if (!pmd)
> >> + goto err_alloc;
> >> + pud_populate(&init_mm, pud, pmd);
> >> #ifndef __PAGETABLE_PMD_FOLDED
> >> - pmd_init(new);
> >> + pmd_init(pmd);
> >> #endif
> >> }
> >>
> >> pmd = pmd_offset(pud, addr);
> >> - if (pmd_none(*pmd)) {
> >> - pte_t *new __maybe_unused;
> >> + if (!pmd_present(*pmd)) {
> >> + pte_t *pte;
> >>
> >> - new = memblock_alloc_low(PAGE_SIZE, PAGE_SIZE);
> >> - pmd_populate_kernel(&init_mm, pmd, new);
> >> + pte = memblock_alloc(PAGE_SIZE, PAGE_SIZE);
> > I don't think memblock_alloc_low() here can be replaced by memblock_alloc().
> Can you share me the points that pte table must be allocated with memblock_alloc_low
> in this place?
I forget the reason now, so if you confirm memblock_alloc() works well
here, you can use it. But please don't use memblock_alloc_raw().

Huacai
>
> Regards
> Bibo Mao
> >
> >
> > Huacai
> >> + if (!pte)
> >> + goto err_alloc;
> >> + pmd_populate_kernel(&init_mm, pmd, pte);
> >> }
> >>
> >> return pte_offset_kernel(pmd, addr);
> >> +
> >> +err_alloc:
> >> + panic("%s: Failed to allocate memory\n", __func__);
> >> + return NULL;
> >> }
> >>
> >> void __init __set_fixmap(enum fixed_addresses idx,
> >> @@ -241,7 +244,12 @@ void __init __set_fixmap(enum fixed_addresses idx,
> >>
> >> BUG_ON(idx <= FIX_HOLE || idx >= __end_of_fixed_addresses);
> >>
> >> - ptep = fixmap_pte(addr);
> >> + /*
> >> + * Now only FIX_EARLYCON_MEM_BASE fixed map is used
> >> + * __set_fixmap must be called before mem_init since function
> >> + * populate_kernel_pte allocates memory with memblock_alloc method.
> >> + */
> >> + ptep = populate_kernel_pte(addr);
> >> if (!pte_none(*ptep)) {
> >> pte_ERROR(*ptep);
> >> return;
> >> --
> >> 2.27.0
> >>
>
>

2023-08-10 04:56:54

by Huacai Chen

Subject: Re: [PATCH 3/3] LoongArch: mm: Add unified function populate_kernel_pte

On Thu, Aug 10, 2023 at 12:09 PM bibo mao <[email protected]> wrote:
>
>
>
> 在 2023/8/2 15:25, Huacai Chen 写道:
> > On Tue, Aug 1, 2023 at 9:22 AM bibo mao <[email protected]> wrote:
> >>
> >>
> >>
> >> 在 2023/7/31 22:15, Huacai Chen 写道:
> >>> On Wed, Jul 12, 2023 at 11:16 AM Bibo Mao <[email protected]> wrote:
> >>>>
> >>>> Function pcpu_populate_pte and fixmap_pte are similar, they populate
> >>>> one page from kernel address space. And there is confusion between
> >>>> pgd and p4d in function fixmap_pte, such as pgd_none always returns
> >>>> zero. This patch adds unified function populate_kernel_pte and replaces
> >>>> pcpu_populate_pte and fixmap_pte.
> >>>>
> >>>> Signed-off-by: Bibo Mao <[email protected]>
> >>>> ---
> >>>> arch/loongarch/include/asm/pgalloc.h | 1 +
> >>>> arch/loongarch/kernel/numa.c | 40 +--------------------
> >>>> arch/loongarch/mm/init.c | 52 ++++++++++++++++------------
> >>>> 3 files changed, 32 insertions(+), 61 deletions(-)
> >>>>
> >>>> diff --git a/arch/loongarch/include/asm/pgalloc.h b/arch/loongarch/include/asm/pgalloc.h
> >>>> index af1d1e4a6965..ca17b573dba6 100644
> >>>> --- a/arch/loongarch/include/asm/pgalloc.h
> >>>> +++ b/arch/loongarch/include/asm/pgalloc.h
> >>>> @@ -91,4 +91,5 @@ static inline pud_t *pud_alloc_one(struct mm_struct *mm, unsigned long address)
> >>>>
> >>>> #endif /* __PAGETABLE_PUD_FOLDED */
> >>>>
> >>>> +extern pte_t * __init populate_kernel_pte(unsigned long addr);
> >>>> #endif /* _ASM_PGALLOC_H */
> >>>> diff --git a/arch/loongarch/kernel/numa.c b/arch/loongarch/kernel/numa.c
> >>>> index 778e1c20bfb0..24a693b76873 100644
> >>>> --- a/arch/loongarch/kernel/numa.c
> >>>> +++ b/arch/loongarch/kernel/numa.c
> >>>> @@ -67,46 +67,8 @@ static int __init pcpu_cpu_distance(unsigned int from, unsigned int to)
> >>>>
> >>>> void __init pcpu_populate_pte(unsigned long addr)
> >>>> {
> >>>> - pgd_t *pgd = pgd_offset_k(addr);
> >>>> - p4d_t *p4d = p4d_offset(pgd, addr);
> >>>> - pud_t *pud;
> >>>> - pmd_t *pmd;
> >>>> -
> >>>> - if (p4d_none(*p4d)) {
> >>>> - pud = memblock_alloc_raw(PAGE_SIZE, PAGE_SIZE);
> >>>> - if (!pud)
> >>>> - goto err_alloc;
> >>>> - p4d_populate(&init_mm, p4d, pud);
> >>>> -#ifndef __PAGETABLE_PUD_FOLDED
> >>>> - pud_init(pud);
> >>>> -#endif
> >>>> - }
> >>>> -
> >>>> - pud = pud_offset(p4d, addr);
> >>>> - if (pud_none(*pud)) {
> >>>> - pmd = memblock_alloc_raw(PAGE_SIZE, PAGE_SIZE);
> >>>> - if (!pmd)
> >>>> - goto err_alloc;
> >>>> - pud_populate(&init_mm, pud, pmd);
> >>>> -#ifndef __PAGETABLE_PMD_FOLDED
> >>>> - pmd_init(pmd);
> >>>> -#endif
> >>>> - }
> >>>> -
> >>>> - pmd = pmd_offset(pud, addr);
> >>>> - if (!pmd_present(*pmd)) {
> >>>> - pte_t *pte;
> >>>> -
> >>>> - pte = memblock_alloc(PAGE_SIZE, PAGE_SIZE);
> >>>> - if (!pte)
> >>>> - goto err_alloc;
> >>>> - pmd_populate_kernel(&init_mm, pmd, pte);
> >>>> - }
> >>>> -
> >>>> + populate_kernel_pte(addr);
> >>>> return;
> >>>> -
> >>>> -err_alloc:
> >>>> - panic("%s: Failed to allocate memory\n", __func__);
> >>>> }
> >>>>
> >>>> void __init setup_per_cpu_areas(void)
> >>>> diff --git a/arch/loongarch/mm/init.c b/arch/loongarch/mm/init.c
> >>>> index 3b7d8129570b..6cd2948373ae 100644
> >>>> --- a/arch/loongarch/mm/init.c
> >>>> +++ b/arch/loongarch/mm/init.c
> >>>> @@ -191,46 +191,49 @@ void vmemmap_free(unsigned long start, unsigned long end, struct vmem_altmap *al
> >>>> #endif
> >>>> #endif
> >>>>
> >>>> -static pte_t *fixmap_pte(unsigned long addr)
> >>>> +pte_t * __init populate_kernel_pte(unsigned long addr)
> >>>> {
> >>>> - pgd_t *pgd;
> >>>> - p4d_t *p4d;
> >>>> + pgd_t *pgd = pgd_offset_k(addr);
> >>>> + p4d_t *p4d = p4d_offset(pgd, addr);
> >>>> pud_t *pud;
> >>>> pmd_t *pmd;
> >>>>
> >>>> - pgd = pgd_offset_k(addr);
> >>>> - p4d = p4d_offset(pgd, addr);
> >>>> -
> >>>> - if (pgd_none(*pgd)) {
> >>>> - pud_t *new __maybe_unused;
> >>>> -
> >>>> - new = memblock_alloc_low(PAGE_SIZE, PAGE_SIZE);
> >>>> - pgd_populate(&init_mm, pgd, new);
> >>>> + if (p4d_none(*p4d)) {
> >>>> + pud = memblock_alloc_raw(PAGE_SIZE, PAGE_SIZE);
> >>>> + if (!pud)
> >>>> + goto err_alloc;
> >>>> + p4d_populate(&init_mm, p4d, pud);
> >>>> #ifndef __PAGETABLE_PUD_FOLDED
> >>>> - pud_init(new);
> >>>> + pud_init(pud);
> >>>> #endif
> >>>> }
> >>>>
> >>>> pud = pud_offset(p4d, addr);
> >>>> if (pud_none(*pud)) {
> >>>> - pmd_t *new __maybe_unused;
> >>>> -
> >>>> - new = memblock_alloc_low(PAGE_SIZE, PAGE_SIZE);
> >>>> - pud_populate(&init_mm, pud, new);
> >>>> + pmd = memblock_alloc_raw(PAGE_SIZE, PAGE_SIZE);
> >>>> + if (!pmd)
> >>>> + goto err_alloc;
> >>>> + pud_populate(&init_mm, pud, pmd);
> >>>> #ifndef __PAGETABLE_PMD_FOLDED
> >>>> - pmd_init(new);
> >>>> + pmd_init(pmd);
> >>>> #endif
> >>>> }
> >>>>
> >>>> pmd = pmd_offset(pud, addr);
> >>>> - if (pmd_none(*pmd)) {
> >>>> - pte_t *new __maybe_unused;
> >>>> + if (!pmd_present(*pmd)) {
> >>>> + pte_t *pte;
> >>>>
> >>>> - new = memblock_alloc_low(PAGE_SIZE, PAGE_SIZE);
> >>>> - pmd_populate_kernel(&init_mm, pmd, new);
> >>>> + pte = memblock_alloc(PAGE_SIZE, PAGE_SIZE);
> >>> I don't think memblock_alloc_low() here can be replaced by memblock_alloc().
> >> Can you share me the points that pte table must be allocated with memblock_alloc_low
> >> in this place?
> > I forget the reason now, so if you confirm memblock_alloc() works well
> > here, you can use it. But please don't use memblock_alloc_raw().
> what a mess, there is more comments if there is special reason, else everyone can
> forgot by elapsed time.
>
> why the function memblock_alloc_raw can not be use? there is one useless page copy.
This is not a performance critical path, keeping consistency with
mm/percpu.c can make life easier.

Huacai

>
> Regards
> Bibo Mao
>
>
> >
> > Huacai
> >>
> >> Regards
> >> Bibo Mao
> >>>
> >>>
> >>> Huacai
> >>>> + if (!pte)
> >>>> + goto err_alloc;
> >>>> + pmd_populate_kernel(&init_mm, pmd, pte);
> >>>> }
> >>>>
> >>>> return pte_offset_kernel(pmd, addr);
> >>>> +
> >>>> +err_alloc:
> >>>> + panic("%s: Failed to allocate memory\n", __func__);
> >>>> + return NULL;
> >>>> }
> >>>>
> >>>> void __init __set_fixmap(enum fixed_addresses idx,
> >>>> @@ -241,7 +244,12 @@ void __init __set_fixmap(enum fixed_addresses idx,
> >>>>
> >>>> BUG_ON(idx <= FIX_HOLE || idx >= __end_of_fixed_addresses);
> >>>>
> >>>> - ptep = fixmap_pte(addr);
> >>>> + /*
> >>>> + * Now only FIX_EARLYCON_MEM_BASE fixed map is used
> >>>> + * __set_fixmap must be called before mem_init since function
> >>>> + * populate_kernel_pte allocates memory with memblock_alloc method.
> >>>> + */
> >>>> + ptep = populate_kernel_pte(addr);
> >>>> if (!pte_none(*ptep)) {
> >>>> pte_ERROR(*ptep);
> >>>> return;
> >>>> --
> >>>> 2.27.0
> >>>>
> >>
> >>
>
>

2023-08-10 05:07:08

by maobibo

Subject: Re: [PATCH 3/3] LoongArch: mm: Add unified function populate_kernel_pte

在 2023/8/10 12:27, Huacai Chen 写道:
> On Thu, Aug 10, 2023 at 12:09 PM bibo mao <[email protected]> wrote:
>>
>>
>>
>> 在 2023/8/2 15:25, Huacai Chen 写道:
>>> On Tue, Aug 1, 2023 at 9:22 AM bibo mao <[email protected]> wrote:
>>>>
>>>>
>>>>
>>>> 在 2023/7/31 22:15, Huacai Chen 写道:
>>>>> On Wed, Jul 12, 2023 at 11:16 AM Bibo Mao <[email protected]> wrote:
>>>>>>
>>>>>> Function pcpu_populate_pte and fixmap_pte are similar, they populate
>>>>>> one page from kernel address space. And there is confusion between
>>>>>> pgd and p4d in function fixmap_pte, such as pgd_none always returns
>>>>>> zero. This patch adds unified function populate_kernel_pte and replaces
>>>>>> pcpu_populate_pte and fixmap_pte.
>>>>>>
>>>>>> Signed-off-by: Bibo Mao <[email protected]>
>>>>>> ---
>>>>>> arch/loongarch/include/asm/pgalloc.h | 1 +
>>>>>> arch/loongarch/kernel/numa.c | 40 +--------------------
>>>>>> arch/loongarch/mm/init.c | 52 ++++++++++++++++------------
>>>>>> 3 files changed, 32 insertions(+), 61 deletions(-)
>>>>>>
>>>>>> diff --git a/arch/loongarch/include/asm/pgalloc.h b/arch/loongarch/include/asm/pgalloc.h
>>>>>> index af1d1e4a6965..ca17b573dba6 100644
>>>>>> --- a/arch/loongarch/include/asm/pgalloc.h
>>>>>> +++ b/arch/loongarch/include/asm/pgalloc.h
>>>>>> @@ -91,4 +91,5 @@ static inline pud_t *pud_alloc_one(struct mm_struct *mm, unsigned long address)
>>>>>>
>>>>>> #endif /* __PAGETABLE_PUD_FOLDED */
>>>>>>
>>>>>> +extern pte_t * __init populate_kernel_pte(unsigned long addr);
>>>>>> #endif /* _ASM_PGALLOC_H */
>>>>>> diff --git a/arch/loongarch/kernel/numa.c b/arch/loongarch/kernel/numa.c
>>>>>> index 778e1c20bfb0..24a693b76873 100644
>>>>>> --- a/arch/loongarch/kernel/numa.c
>>>>>> +++ b/arch/loongarch/kernel/numa.c
>>>>>> @@ -67,46 +67,8 @@ static int __init pcpu_cpu_distance(unsigned int from, unsigned int to)
>>>>>>
>>>>>> void __init pcpu_populate_pte(unsigned long addr)
>>>>>> {
>>>>>> - pgd_t *pgd = pgd_offset_k(addr);
>>>>>> - p4d_t *p4d = p4d_offset(pgd, addr);
>>>>>> - pud_t *pud;
>>>>>> - pmd_t *pmd;
>>>>>> -
>>>>>> - if (p4d_none(*p4d)) {
>>>>>> - pud = memblock_alloc_raw(PAGE_SIZE, PAGE_SIZE);
>>>>>> - if (!pud)
>>>>>> - goto err_alloc;
>>>>>> - p4d_populate(&init_mm, p4d, pud);
>>>>>> -#ifndef __PAGETABLE_PUD_FOLDED
>>>>>> - pud_init(pud);
>>>>>> -#endif
>>>>>> - }
>>>>>> -
>>>>>> - pud = pud_offset(p4d, addr);
>>>>>> - if (pud_none(*pud)) {
>>>>>> - pmd = memblock_alloc_raw(PAGE_SIZE, PAGE_SIZE);
>>>>>> - if (!pmd)
>>>>>> - goto err_alloc;
>>>>>> - pud_populate(&init_mm, pud, pmd);
>>>>>> -#ifndef __PAGETABLE_PMD_FOLDED
>>>>>> - pmd_init(pmd);
>>>>>> -#endif
>>>>>> - }
>>>>>> -
>>>>>> - pmd = pmd_offset(pud, addr);
>>>>>> - if (!pmd_present(*pmd)) {
>>>>>> - pte_t *pte;
>>>>>> -
>>>>>> - pte = memblock_alloc(PAGE_SIZE, PAGE_SIZE);
>>>>>> - if (!pte)
>>>>>> - goto err_alloc;
>>>>>> - pmd_populate_kernel(&init_mm, pmd, pte);
>>>>>> - }
>>>>>> -
>>>>>> + populate_kernel_pte(addr);
>>>>>> return;
>>>>>> -
>>>>>> -err_alloc:
>>>>>> - panic("%s: Failed to allocate memory\n", __func__);
>>>>>> }
>>>>>>
>>>>>> void __init setup_per_cpu_areas(void)
>>>>>> diff --git a/arch/loongarch/mm/init.c b/arch/loongarch/mm/init.c
>>>>>> index 3b7d8129570b..6cd2948373ae 100644
>>>>>> --- a/arch/loongarch/mm/init.c
>>>>>> +++ b/arch/loongarch/mm/init.c
>>>>>> @@ -191,46 +191,49 @@ void vmemmap_free(unsigned long start, unsigned long end, struct vmem_altmap *al
>>>>>> #endif
>>>>>> #endif
>>>>>>
>>>>>> -static pte_t *fixmap_pte(unsigned long addr)
>>>>>> +pte_t * __init populate_kernel_pte(unsigned long addr)
>>>>>> {
>>>>>> - pgd_t *pgd;
>>>>>> - p4d_t *p4d;
>>>>>> + pgd_t *pgd = pgd_offset_k(addr);
>>>>>> + p4d_t *p4d = p4d_offset(pgd, addr);
>>>>>> pud_t *pud;
>>>>>> pmd_t *pmd;
>>>>>>
>>>>>> - pgd = pgd_offset_k(addr);
>>>>>> - p4d = p4d_offset(pgd, addr);
>>>>>> -
>>>>>> - if (pgd_none(*pgd)) {
>>>>>> - pud_t *new __maybe_unused;
>>>>>> -
>>>>>> - new = memblock_alloc_low(PAGE_SIZE, PAGE_SIZE);
>>>>>> - pgd_populate(&init_mm, pgd, new);
>>>>>> + if (p4d_none(*p4d)) {
>>>>>> + pud = memblock_alloc_raw(PAGE_SIZE, PAGE_SIZE);
>>>>>> + if (!pud)
>>>>>> + goto err_alloc;
>>>>>> + p4d_populate(&init_mm, p4d, pud);
>>>>>> #ifndef __PAGETABLE_PUD_FOLDED
>>>>>> - pud_init(new);
>>>>>> + pud_init(pud);
>>>>>> #endif
>>>>>> }
>>>>>>
>>>>>> pud = pud_offset(p4d, addr);
>>>>>> if (pud_none(*pud)) {
>>>>>> - pmd_t *new __maybe_unused;
>>>>>> -
>>>>>> - new = memblock_alloc_low(PAGE_SIZE, PAGE_SIZE);
>>>>>> - pud_populate(&init_mm, pud, new);
>>>>>> + pmd = memblock_alloc_raw(PAGE_SIZE, PAGE_SIZE);
>>>>>> + if (!pmd)
>>>>>> + goto err_alloc;
>>>>>> + pud_populate(&init_mm, pud, pmd);
>>>>>> #ifndef __PAGETABLE_PMD_FOLDED
>>>>>> - pmd_init(new);
>>>>>> + pmd_init(pmd);
>>>>>> #endif
>>>>>> }
>>>>>>
>>>>>> pmd = pmd_offset(pud, addr);
>>>>>> - if (pmd_none(*pmd)) {
>>>>>> - pte_t *new __maybe_unused;
>>>>>> + if (!pmd_present(*pmd)) {
>>>>>> + pte_t *pte;
>>>>>>
>>>>>> - new = memblock_alloc_low(PAGE_SIZE, PAGE_SIZE);
>>>>>> - pmd_populate_kernel(&init_mm, pmd, new);
>>>>>> + pte = memblock_alloc(PAGE_SIZE, PAGE_SIZE);
>>>>> I don't think memblock_alloc_low() here can be replaced by memblock_alloc().
>>>> Can you share me the points that pte table must be allocated with memblock_alloc_low
>>>> in this place?
>>> I forget the reason now, so if you confirm memblock_alloc() works well
>>> here, you can use it. But please don't use memblock_alloc_raw().
>> what a mess, there is more comments if there is special reason, else everyone can
>> forgot by elapsed time.
>>
>> why the function memblock_alloc_raw can not be use? there is one useless page copy.
> This is not a performance critical path, keeping consistency with
> mm/percpu.c can make life easier.
yes, it is not critical path, it influences boot speed. Can we make it better? else it
will be just so so.

Regards
Bibo Mao
>
> Huacai
>
>>
>> Regards
>> Bibo Mao
>>
>>
>>>
>>> Huacai
>>>>
>>>> Regards
>>>> Bibo Mao
>>>>>
>>>>>
>>>>> Huacai
>>>>>> + if (!pte)
>>>>>> + goto err_alloc;
>>>>>> + pmd_populate_kernel(&init_mm, pmd, pte);
>>>>>> }
>>>>>>
>>>>>> return pte_offset_kernel(pmd, addr);
>>>>>> +
>>>>>> +err_alloc:
>>>>>> + panic("%s: Failed to allocate memory\n", __func__);
>>>>>> + return NULL;
>>>>>> }
>>>>>>
>>>>>> void __init __set_fixmap(enum fixed_addresses idx,
>>>>>> @@ -241,7 +244,12 @@ void __init __set_fixmap(enum fixed_addresses idx,
>>>>>>
>>>>>> BUG_ON(idx <= FIX_HOLE || idx >= __end_of_fixed_addresses);
>>>>>>
>>>>>> - ptep = fixmap_pte(addr);
>>>>>> + /*
>>>>>> + * Now only FIX_EARLYCON_MEM_BASE fixed map is used
>>>>>> + * __set_fixmap must be called before mem_init since function
>>>>>> + * populate_kernel_pte allocates memory with memblock_alloc method.
>>>>>> + */
>>>>>> + ptep = populate_kernel_pte(addr);
>>>>>> if (!pte_none(*ptep)) {
>>>>>> pte_ERROR(*ptep);
>>>>>> return;
>>>>>> --
>>>>>> 2.27.0
>>>>>>
>>>>
>>>>
>>
>>

2023-08-10 05:16:00

by maobibo

Subject: Re: [PATCH 3/3] LoongArch: mm: Add unified function populate_kernel_pte

在 2023/8/2 15:25, Huacai Chen 写道:
> On Tue, Aug 1, 2023 at 9:22 AM bibo mao <[email protected]> wrote:
>>
>>
>>
>> 在 2023/7/31 22:15, Huacai Chen 写道:
>>> On Wed, Jul 12, 2023 at 11:16 AM Bibo Mao <[email protected]> wrote:
>>>>
>>>> Function pcpu_populate_pte and fixmap_pte are similar, they populate
>>>> one page from kernel address space. And there is confusion between
>>>> pgd and p4d in function fixmap_pte, such as pgd_none always returns
>>>> zero. This patch adds unified function populate_kernel_pte and replaces
>>>> pcpu_populate_pte and fixmap_pte.
>>>>
>>>> Signed-off-by: Bibo Mao <[email protected]>
>>>> ---
>>>> arch/loongarch/include/asm/pgalloc.h | 1 +
>>>> arch/loongarch/kernel/numa.c | 40 +--------------------
>>>> arch/loongarch/mm/init.c | 52 ++++++++++++++++------------
>>>> 3 files changed, 32 insertions(+), 61 deletions(-)
>>>>
>>>> diff --git a/arch/loongarch/include/asm/pgalloc.h b/arch/loongarch/include/asm/pgalloc.h
>>>> index af1d1e4a6965..ca17b573dba6 100644
>>>> --- a/arch/loongarch/include/asm/pgalloc.h
>>>> +++ b/arch/loongarch/include/asm/pgalloc.h
>>>> @@ -91,4 +91,5 @@ static inline pud_t *pud_alloc_one(struct mm_struct *mm, unsigned long address)
>>>>
>>>> #endif /* __PAGETABLE_PUD_FOLDED */
>>>>
>>>> +extern pte_t * __init populate_kernel_pte(unsigned long addr);
>>>> #endif /* _ASM_PGALLOC_H */
>>>> diff --git a/arch/loongarch/kernel/numa.c b/arch/loongarch/kernel/numa.c
>>>> index 778e1c20bfb0..24a693b76873 100644
>>>> --- a/arch/loongarch/kernel/numa.c
>>>> +++ b/arch/loongarch/kernel/numa.c
>>>> @@ -67,46 +67,8 @@ static int __init pcpu_cpu_distance(unsigned int from, unsigned int to)
>>>>
>>>> void __init pcpu_populate_pte(unsigned long addr)
>>>> {
>>>> - pgd_t *pgd = pgd_offset_k(addr);
>>>> - p4d_t *p4d = p4d_offset(pgd, addr);
>>>> - pud_t *pud;
>>>> - pmd_t *pmd;
>>>> -
>>>> - if (p4d_none(*p4d)) {
>>>> - pud = memblock_alloc_raw(PAGE_SIZE, PAGE_SIZE);
>>>> - if (!pud)
>>>> - goto err_alloc;
>>>> - p4d_populate(&init_mm, p4d, pud);
>>>> -#ifndef __PAGETABLE_PUD_FOLDED
>>>> - pud_init(pud);
>>>> -#endif
>>>> - }
>>>> -
>>>> - pud = pud_offset(p4d, addr);
>>>> - if (pud_none(*pud)) {
>>>> - pmd = memblock_alloc_raw(PAGE_SIZE, PAGE_SIZE);
>>>> - if (!pmd)
>>>> - goto err_alloc;
>>>> - pud_populate(&init_mm, pud, pmd);
>>>> -#ifndef __PAGETABLE_PMD_FOLDED
>>>> - pmd_init(pmd);
>>>> -#endif
>>>> - }
>>>> -
>>>> - pmd = pmd_offset(pud, addr);
>>>> - if (!pmd_present(*pmd)) {
>>>> - pte_t *pte;
>>>> -
>>>> - pte = memblock_alloc(PAGE_SIZE, PAGE_SIZE);
>>>> - if (!pte)
>>>> - goto err_alloc;
>>>> - pmd_populate_kernel(&init_mm, pmd, pte);
>>>> - }
>>>> -
>>>> + populate_kernel_pte(addr);
>>>> return;
>>>> -
>>>> -err_alloc:
>>>> - panic("%s: Failed to allocate memory\n", __func__);
>>>> }
>>>>
>>>> void __init setup_per_cpu_areas(void)
>>>> diff --git a/arch/loongarch/mm/init.c b/arch/loongarch/mm/init.c
>>>> index 3b7d8129570b..6cd2948373ae 100644
>>>> --- a/arch/loongarch/mm/init.c
>>>> +++ b/arch/loongarch/mm/init.c
>>>> @@ -191,46 +191,49 @@ void vmemmap_free(unsigned long start, unsigned long end, struct vmem_altmap *al
>>>> #endif
>>>> #endif
>>>>
>>>> -static pte_t *fixmap_pte(unsigned long addr)
>>>> +pte_t * __init populate_kernel_pte(unsigned long addr)
>>>> {
>>>> - pgd_t *pgd;
>>>> - p4d_t *p4d;
>>>> + pgd_t *pgd = pgd_offset_k(addr);
>>>> + p4d_t *p4d = p4d_offset(pgd, addr);
>>>> pud_t *pud;
>>>> pmd_t *pmd;
>>>>
>>>> - pgd = pgd_offset_k(addr);
>>>> - p4d = p4d_offset(pgd, addr);
>>>> -
>>>> - if (pgd_none(*pgd)) {
>>>> - pud_t *new __maybe_unused;
>>>> -
>>>> - new = memblock_alloc_low(PAGE_SIZE, PAGE_SIZE);
>>>> - pgd_populate(&init_mm, pgd, new);
>>>> + if (p4d_none(*p4d)) {
>>>> + pud = memblock_alloc_raw(PAGE_SIZE, PAGE_SIZE);
>>>> + if (!pud)
>>>> + goto err_alloc;
>>>> + p4d_populate(&init_mm, p4d, pud);
>>>> #ifndef __PAGETABLE_PUD_FOLDED
>>>> - pud_init(new);
>>>> + pud_init(pud);
>>>> #endif
>>>> }
>>>>
>>>> pud = pud_offset(p4d, addr);
>>>> if (pud_none(*pud)) {
>>>> - pmd_t *new __maybe_unused;
>>>> -
>>>> - new = memblock_alloc_low(PAGE_SIZE, PAGE_SIZE);
>>>> - pud_populate(&init_mm, pud, new);
>>>> + pmd = memblock_alloc_raw(PAGE_SIZE, PAGE_SIZE);
>>>> + if (!pmd)
>>>> + goto err_alloc;
>>>> + pud_populate(&init_mm, pud, pmd);
>>>> #ifndef __PAGETABLE_PMD_FOLDED
>>>> - pmd_init(new);
>>>> + pmd_init(pmd);
>>>> #endif
>>>> }
>>>>
>>>> pmd = pmd_offset(pud, addr);
>>>> - if (pmd_none(*pmd)) {
>>>> - pte_t *new __maybe_unused;
>>>> + if (!pmd_present(*pmd)) {
>>>> + pte_t *pte;
>>>>
>>>> - new = memblock_alloc_low(PAGE_SIZE, PAGE_SIZE);
>>>> - pmd_populate_kernel(&init_mm, pmd, new);
>>>> + pte = memblock_alloc(PAGE_SIZE, PAGE_SIZE);
>>> I don't think memblock_alloc_low() here can be replaced by memblock_alloc().
>> Can you share me the points that pte table must be allocated with memblock_alloc_low
>> in this place?
> I forget the reason now, so if you confirm memblock_alloc() works well
> here, you can use it. But please don't use memblock_alloc_raw().
what a mess, there is more comments if there is special reason, else everyone can
forgot by elapsed time.

why the function memblock_alloc_raw can not be use? there is one useless page copy.

Regards
Bibo Mao


>
> Huacai
>>
>> Regards
>> Bibo Mao
>>>
>>>
>>> Huacai
>>>> + if (!pte)
>>>> + goto err_alloc;
>>>> + pmd_populate_kernel(&init_mm, pmd, pte);
>>>> }
>>>>
>>>> return pte_offset_kernel(pmd, addr);
>>>> +
>>>> +err_alloc:
>>>> + panic("%s: Failed to allocate memory\n", __func__);
>>>> + return NULL;
>>>> }
>>>>
>>>> void __init __set_fixmap(enum fixed_addresses idx,
>>>> @@ -241,7 +244,12 @@ void __init __set_fixmap(enum fixed_addresses idx,
>>>>
>>>> BUG_ON(idx <= FIX_HOLE || idx >= __end_of_fixed_addresses);
>>>>
>>>> - ptep = fixmap_pte(addr);
>>>> + /*
>>>> + * Now only FIX_EARLYCON_MEM_BASE fixed map is used
>>>> + * __set_fixmap must be called before mem_init since function
>>>> + * populate_kernel_pte allocates memory with memblock_alloc method.
>>>> + */
>>>> + ptep = populate_kernel_pte(addr);
>>>> if (!pte_none(*ptep)) {
>>>> pte_ERROR(*ptep);
>>>> return;
>>>> --
>>>> 2.27.0
>>>>
>>
>>