| Message ID | 1547280987-7630-1-git-send-email-guoren@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | riscv: fixup max_low_pfn with PFN_DOWN. | expand |
On Sat, Jan 12, 2019 at 04:16:27PM +0800, guoren@kernel.org wrote: > From: Guo Ren <ren_guo@c-sky.com> > > max_low_pfn should be pfn_size not byte_size. > > Signed-off-by: Guo Ren <ren_guo@c-sky.com> > Signed-off-by: Mao Han <mao_han@c-sky.com> > Cc: Palmer Dabbelt <palmer@sifive.com> > Cc: Albert Ou <aou@eecs.berkeley.edu> > --- > arch/riscv/kernel/setup.c | 2 +- > arch/riscv/mm/init.c | 3 ++- > 2 files changed, 3 insertions(+), 2 deletions(-) > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > index fc8006a..5463e67 100644 > --- a/arch/riscv/kernel/setup.c > +++ b/arch/riscv/kernel/setup.c > @@ -174,7 +174,7 @@ static void __init setup_bootmem(void) > BUG_ON(mem_size == 0); > > set_max_mapnr(PFN_DOWN(mem_size)); > - max_low_pfn = memblock_end_of_DRAM(); > + max_low_pfn = PFN_DOWN(memblock_end_of_DRAM()); I know it is used just above, but can we please just switch this code to use >> PAGE_SHIFT instead of PFN_DOWN, which just horribly obsfucates what is going on? > > #ifdef CONFIG_BLK_DEV_INITRD > setup_initrd(); > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c > index 1d9bfaf..658ebf6 100644 > --- a/arch/riscv/mm/init.c > +++ b/arch/riscv/mm/init.c > @@ -28,7 +28,8 @@ static void __init zone_sizes_init(void) > unsigned long max_zone_pfns[MAX_NR_ZONES] = { 0, }; > > #ifdef CONFIG_ZONE_DMA32 > - max_zone_pfns[ZONE_DMA32] = PFN_DOWN(min(4UL * SZ_1G, max_low_pfn)); > + max_zone_pfns[ZONE_DMA32] = PFN_DOWN(min(4UL * SZ_1G, > + (unsigned long) PFN_PHYS(max_low_pfn))); > #endif Same comment as above here, plus I think we should just use memblock_end_of_DRAM directly, e.g. something like: static const phys_addr_t max_dma32_addr = 4UL * SZ_1G; max_zone_pfns[ZONE_DMA32] = min(memblock_end_of_DRAM(), max_dma32_addr) >> PAGE_SHIFT;
On Tue, Jan 15, 2019 at 07:36:13AM -0800, Christoph Hellwig wrote: > On Sat, Jan 12, 2019 at 04:16:27PM +0800, guoren@kernel.org wrote: > > From: Guo Ren <ren_guo@c-sky.com> > > > > max_low_pfn should be pfn_size not byte_size. > > > > Signed-off-by: Guo Ren <ren_guo@c-sky.com> > > Signed-off-by: Mao Han <mao_han@c-sky.com> > > Cc: Palmer Dabbelt <palmer@sifive.com> > > Cc: Albert Ou <aou@eecs.berkeley.edu> > > --- > > arch/riscv/kernel/setup.c | 2 +- > > arch/riscv/mm/init.c | 3 ++- > > 2 files changed, 3 insertions(+), 2 deletions(-) > > > > diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c > > index fc8006a..5463e67 100644 > > --- a/arch/riscv/kernel/setup.c > > +++ b/arch/riscv/kernel/setup.c > > @@ -174,7 +174,7 @@ static void __init setup_bootmem(void) > > BUG_ON(mem_size == 0); > > > > set_max_mapnr(PFN_DOWN(mem_size)); > > - max_low_pfn = memblock_end_of_DRAM(); > > + max_low_pfn = PFN_DOWN(memblock_end_of_DRAM()); > > I know it is used just above, but can we please just switch this > code to use >> PAGE_SHIFT instead of PFN_DOWN, which just horribly > obsfucates what is going on? ??? #define PFN_DOWN(x) ((x) >> PAGE_SHIFT) phys_addr_t __init_memblock memblock_end_of_DRAM(void) { int idx = memblock.memory.cnt - 1; return (memblock.memory.regions[idx].base + memblock.memory.regions[idx].size); } What's the problem? PFN_DOWN() couldn't be used with function call? > > > > > #ifdef CONFIG_BLK_DEV_INITRD > > setup_initrd(); > > diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c > > index 1d9bfaf..658ebf6 100644 > > --- a/arch/riscv/mm/init.c > > +++ b/arch/riscv/mm/init.c > > @@ -28,7 +28,8 @@ static void __init zone_sizes_init(void) > > unsigned long max_zone_pfns[MAX_NR_ZONES] = { 0, }; > > > > #ifdef CONFIG_ZONE_DMA32 > > - max_zone_pfns[ZONE_DMA32] = PFN_DOWN(min(4UL * SZ_1G, max_low_pfn)); > > + max_zone_pfns[ZONE_DMA32] = PFN_DOWN(min(4UL * SZ_1G, > > + (unsigned long) PFN_PHYS(max_low_pfn))); > > #endif > > Same comment as above here, plus I think we should just use > memblock_end_of_DRAM directly, e.g. something like: > > static const phys_addr_t max_dma32_addr = 4UL * SZ_1G; > > max_zone_pfns[ZONE_DMA32] = > min(memblock_end_of_DRAM(), max_dma32_addr) >> PAGE_SHIFT; Em... The meaning of PFN_PHYS(max_low_pfn) != memblock_end_of_DRAM() in 32-bit highmem system. Of cause, riscv doesn't support highmem, so I think memblock_end_of_DRAM() is also OK. But... static void __init zone_sizes_init(void) { unsigned long max_zone_pfns[MAX_NR_ZONES] = { 0, }; #ifdef CONFIG_ZONE_DMA32 max_zone_pfns[ZONE_DMA32] = PFN_DOWN(min(4UL * SZ_1G, max_low_pfn)); #endif max_zone_pfns[ZONE_NORMAL] = max_low_pfn; free_area_init_nodes(max_zone_pfns); } The max_low_pfn also used by ZONE_NORMAL, So shall we need change that? max_zone_pfns[ZONE_NORMAL] = PFN_DOWN(memblock_end_of_DRAM()); ^^^^^^^^ also must >> PAGE_SHIFT? My patch just want to point out that max_low_pfn is PFN not size. In fact there is no error for running without my patch :P Best Regards Guo Ren
On Wed, Jan 16, 2019 at 12:10:00AM +0800, Guo Ren wrote: > > > set_max_mapnr(PFN_DOWN(mem_size)); > > > - max_low_pfn = memblock_end_of_DRAM(); > > > + max_low_pfn = PFN_DOWN(memblock_end_of_DRAM()); > > > > I know it is used just above, but can we please just switch this > > code to use >> PAGE_SHIFT instead of PFN_DOWN, which just horribly > > obsfucates what is going on? > ??? > #define PFN_DOWN(x) ((x) >> PAGE_SHIFT) > > phys_addr_t __init_memblock memblock_end_of_DRAM(void) > { > int idx = memblock.memory.cnt - 1; > > return (memblock.memory.regions[idx].base + memblock.memory.regions[idx].size); > } > > What's the problem? PFN_DOWN() couldn't be used with function call? PFN_DOWN gives you the correct result. But I think it actually drastically reduces readability over just opencoding it. > My patch just want to point out that max_low_pfn is PFN not size. In fact > there is no error for running without my patch :P No, I think your patch is correct. I just wonder if we could make the code easier to read.
Hi Christoph, I use PFN_DOWN() every where as possible and seems it's a habit problem. So let risc-v maintainer to choose "PFN_DOW()" or ">> PAGE_SHIFT". Also the same with "end_of_DRAM & max_low_pfn". Best Regards Guo Ren On Tue, Jan 15, 2019 at 08:12:54AM -0800, Christoph Hellwig wrote: > On Wed, Jan 16, 2019 at 12:10:00AM +0800, Guo Ren wrote: > > > > set_max_mapnr(PFN_DOWN(mem_size)); > > > > - max_low_pfn = memblock_end_of_DRAM(); > > > > + max_low_pfn = PFN_DOWN(memblock_end_of_DRAM()); > > > > > > I know it is used just above, but can we please just switch this > > > code to use >> PAGE_SHIFT instead of PFN_DOWN, which just horribly > > > obsfucates what is going on? > > ??? > > #define PFN_DOWN(x) ((x) >> PAGE_SHIFT) > > > > phys_addr_t __init_memblock memblock_end_of_DRAM(void) > > { > > int idx = memblock.memory.cnt - 1; > > > > return (memblock.memory.regions[idx].base + memblock.memory.regions[idx].size); > > } > > > > What's the problem? PFN_DOWN() couldn't be used with function call? > > PFN_DOWN gives you the correct result. But I think it actually > drastically reduces readability over just opencoding it. > > > My patch just want to point out that max_low_pfn is PFN not size. In fact > > there is no error for running without my patch :P > > No, I think your patch is correct. I just wonder if we could make > the code easier to read.
On Tue, 15 Jan 2019 17:07:38 PST (-0800), guoren@kernel.org wrote: > Hi Christoph, > > I use PFN_DOWN() every where as possible and seems it's a habit > problem. So let risc-v maintainer to choose "PFN_DOW()" or > ">> PAGE_SHIFT". > > Also the same with "end_of_DRAM & max_low_pfn". PFN_DOWN makes sense to me, as that's what we're trying to do here (round a physical address down to page frame number). Am a I misunderstanding something? > > Best Regards > Guo Ren > > On Tue, Jan 15, 2019 at 08:12:54AM -0800, Christoph Hellwig wrote: >> On Wed, Jan 16, 2019 at 12:10:00AM +0800, Guo Ren wrote: >> > > > set_max_mapnr(PFN_DOWN(mem_size)); >> > > > - max_low_pfn = memblock_end_of_DRAM(); >> > > > + max_low_pfn = PFN_DOWN(memblock_end_of_DRAM()); >> > > >> > > I know it is used just above, but can we please just switch this >> > > code to use >> PAGE_SHIFT instead of PFN_DOWN, which just horribly >> > > obsfucates what is going on? >> > ??? >> > #define PFN_DOWN(x) ((x) >> PAGE_SHIFT) >> > >> > phys_addr_t __init_memblock memblock_end_of_DRAM(void) >> > { >> > int idx = memblock.memory.cnt - 1; >> > >> > return (memblock.memory.regions[idx].base + memblock.memory.regions[idx].size); >> > } >> > >> > What's the problem? PFN_DOWN() couldn't be used with function call? >> >> PFN_DOWN gives you the correct result. But I think it actually >> drastically reduces readability over just opencoding it. >> >> > My patch just want to point out that max_low_pfn is PFN not size. In fact >> > there is no error for running without my patch :P >> >> No, I think your patch is correct. I just wonder if we could make >> the code easier to read.
On Wed, Jan 23, 2019 at 06:00:35PM -0800, Palmer Dabbelt wrote: > On Tue, 15 Jan 2019 17:07:38 PST (-0800), guoren@kernel.org wrote: > >Hi Christoph, > > > >I use PFN_DOWN() every where as possible and seems it's a habit > >problem. So let risc-v maintainer to choose "PFN_DOW()" or > >">> PAGE_SHIFT". > > > >Also the same with "end_of_DRAM & max_low_pfn". > > PFN_DOWN makes sense to me, as that's what we're trying to do here (round a > physical address down to page frame number). Am a I misunderstanding > something? > No, you got it :) Best Regards Guo Ren
diff --git a/arch/riscv/kernel/setup.c b/arch/riscv/kernel/setup.c index fc8006a..5463e67 100644 --- a/arch/riscv/kernel/setup.c +++ b/arch/riscv/kernel/setup.c @@ -174,7 +174,7 @@ static void __init setup_bootmem(void) BUG_ON(mem_size == 0); set_max_mapnr(PFN_DOWN(mem_size)); - max_low_pfn = memblock_end_of_DRAM(); + max_low_pfn = PFN_DOWN(memblock_end_of_DRAM()); #ifdef CONFIG_BLK_DEV_INITRD setup_initrd(); diff --git a/arch/riscv/mm/init.c b/arch/riscv/mm/init.c index 1d9bfaf..658ebf6 100644 --- a/arch/riscv/mm/init.c +++ b/arch/riscv/mm/init.c @@ -28,7 +28,8 @@ static void __init zone_sizes_init(void) unsigned long max_zone_pfns[MAX_NR_ZONES] = { 0, }; #ifdef CONFIG_ZONE_DMA32 - max_zone_pfns[ZONE_DMA32] = PFN_DOWN(min(4UL * SZ_1G, max_low_pfn)); + max_zone_pfns[ZONE_DMA32] = PFN_DOWN(min(4UL * SZ_1G, + (unsigned long) PFN_PHYS(max_low_pfn))); #endif max_zone_pfns[ZONE_NORMAL] = max_low_pfn;