| Message ID | 20210325230938.30752-7-joao.m.martins@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm, sparse-vmemmap: Introduce compound pagemaps | expand |
I suspect it's a good sign I'm only finding cosmetic and changelog changes in the review... I have some more: A year for now if I'm tracking down a problem and looking through mm commits I would appreciate a subject line like the following: "refactor core of vmemmap_populate_basepages() to helper" that gives an idea of the impact and side effects of the change. On Thu, Mar 25, 2021 at 4:10 PM Joao Martins <joao.m.martins@oracle.com> wrote: > I would add a lead in phrase like: "In preparation for describing a memmap with compound pages, move the actual..." > Move the actual pte population logic into a separate function > vmemmap_populate_address() and have vmemmap_populate_basepages() > walk through all base pages it needs to populate. Aside from the above, looks good. > > Signed-off-by: Joao Martins <joao.m.martins@oracle.com> > --- > mm/sparse-vmemmap.c | 44 ++++++++++++++++++++++++++------------------ > 1 file changed, 26 insertions(+), 18 deletions(-) > > diff --git a/mm/sparse-vmemmap.c b/mm/sparse-vmemmap.c > index 370728c206ee..8814876edcfa 100644 > --- a/mm/sparse-vmemmap.c > +++ b/mm/sparse-vmemmap.c > @@ -216,33 +216,41 @@ pgd_t * __meminit vmemmap_pgd_populate(unsigned long addr, int node) > return pgd; > } > > -int __meminit vmemmap_populate_basepages(unsigned long start, unsigned long end, > - int node, struct vmem_altmap *altmap) > +static int __meminit vmemmap_populate_address(unsigned long addr, int node, > + struct vmem_altmap *altmap) > { > - unsigned long addr = start; > pgd_t *pgd; > p4d_t *p4d; > pud_t *pud; > pmd_t *pmd; > pte_t *pte; > > + pgd = vmemmap_pgd_populate(addr, node); > + if (!pgd) > + return -ENOMEM; > + p4d = vmemmap_p4d_populate(pgd, addr, node); > + if (!p4d) > + return -ENOMEM; > + pud = vmemmap_pud_populate(p4d, addr, node); > + if (!pud) > + return -ENOMEM; > + pmd = vmemmap_pmd_populate(pud, addr, node); > + if (!pmd) > + return -ENOMEM; > + pte = vmemmap_pte_populate(pmd, addr, node, altmap); > + if (!pte) > + return -ENOMEM; > + vmemmap_verify(pte, node, addr, addr + PAGE_SIZE); > +} > + > +int __meminit vmemmap_populate_basepages(unsigned long start, unsigned long end, > + int node, struct vmem_altmap *altmap) > +{ > + unsigned long addr = start; > + > for (; addr < end; addr += PAGE_SIZE) { > - pgd = vmemmap_pgd_populate(addr, node); > - if (!pgd) > - return -ENOMEM; > - p4d = vmemmap_p4d_populate(pgd, addr, node); > - if (!p4d) > - return -ENOMEM; > - pud = vmemmap_pud_populate(p4d, addr, node); > - if (!pud) > - return -ENOMEM; > - pmd = vmemmap_pmd_populate(pud, addr, node); > - if (!pmd) > - return -ENOMEM; > - pte = vmemmap_pte_populate(pmd, addr, node, altmap); > - if (!pte) > + if (vmemmap_populate_address(addr, node, altmap)) > return -ENOMEM; > - vmemmap_verify(pte, node, addr, addr + PAGE_SIZE); > } > > return 0; > -- > 2.17.1 >
On 5/5/21 11:43 PM, Dan Williams wrote: > I suspect it's a good sign I'm only finding cosmetic and changelog > changes in the review... Hopefully it continues that way, but the meat of the series is located in patches 4, 6, 7 and 11. *Specially* 6 and 7. I very strongly suspect I am gonna get non-cosmetic comments there. > I have some more: > > A year for now if I'm tracking down a problem and looking through mm > commits I would appreciate a subject line like the following: > "refactor core of vmemmap_populate_basepages() to helper" that gives > an idea of the impact and side effects of the change. > Fixed. > On Thu, Mar 25, 2021 at 4:10 PM Joao Martins <joao.m.martins@oracle.com> wrote: >> > > I would add a lead in phrase like: "In preparation for describing a > memmap with compound pages, move the actual..." > >> Move the actual pte population logic into a separate function >> vmemmap_populate_address() and have vmemmap_populate_basepages() >> walk through all base pages it needs to populate. > > Aside from the above, looks good. > Cool!
On 5/6/21 11:27 AM, Joao Martins wrote: > On 5/5/21 11:43 PM, Dan Williams wrote: >> I suspect it's a good sign I'm only finding cosmetic and changelog >> changes in the review... > > Hopefully it continues that way, but the meat of the series is located > in patches 4, 6, 7 and 11. *Specially* 6 and 7. > Ugh, I meant 7 and 8 *not* 6 and 7. > I very strongly suspect I am gonna get non-cosmetic comments there.
diff --git a/mm/sparse-vmemmap.c b/mm/sparse-vmemmap.c index 370728c206ee..8814876edcfa 100644 --- a/mm/sparse-vmemmap.c +++ b/mm/sparse-vmemmap.c @@ -216,33 +216,41 @@ pgd_t * __meminit vmemmap_pgd_populate(unsigned long addr, int node) return pgd; } -int __meminit vmemmap_populate_basepages(unsigned long start, unsigned long end, - int node, struct vmem_altmap *altmap) +static int __meminit vmemmap_populate_address(unsigned long addr, int node, + struct vmem_altmap *altmap) { - unsigned long addr = start; pgd_t *pgd; p4d_t *p4d; pud_t *pud; pmd_t *pmd; pte_t *pte; + pgd = vmemmap_pgd_populate(addr, node); + if (!pgd) + return -ENOMEM; + p4d = vmemmap_p4d_populate(pgd, addr, node); + if (!p4d) + return -ENOMEM; + pud = vmemmap_pud_populate(p4d, addr, node); + if (!pud) + return -ENOMEM; + pmd = vmemmap_pmd_populate(pud, addr, node); + if (!pmd) + return -ENOMEM; + pte = vmemmap_pte_populate(pmd, addr, node, altmap); + if (!pte) + return -ENOMEM; + vmemmap_verify(pte, node, addr, addr + PAGE_SIZE); +} + +int __meminit vmemmap_populate_basepages(unsigned long start, unsigned long end, + int node, struct vmem_altmap *altmap) +{ + unsigned long addr = start; + for (; addr < end; addr += PAGE_SIZE) { - pgd = vmemmap_pgd_populate(addr, node); - if (!pgd) - return -ENOMEM; - p4d = vmemmap_p4d_populate(pgd, addr, node); - if (!p4d) - return -ENOMEM; - pud = vmemmap_pud_populate(p4d, addr, node); - if (!pud) - return -ENOMEM; - pmd = vmemmap_pmd_populate(pud, addr, node); - if (!pmd) - return -ENOMEM; - pte = vmemmap_pte_populate(pmd, addr, node, altmap); - if (!pte) + if (vmemmap_populate_address(addr, node, altmap)) return -ENOMEM; - vmemmap_verify(pte, node, addr, addr + PAGE_SIZE); } return 0;
Move the actual pte population logic into a separate function vmemmap_populate_address() and have vmemmap_populate_basepages() walk through all base pages it needs to populate. Signed-off-by: Joao Martins <joao.m.martins@oracle.com> --- mm/sparse-vmemmap.c | 44 ++++++++++++++++++++++++++------------------ 1 file changed, 26 insertions(+), 18 deletions(-)