Re: [PATCH v3 7/8] powerpc/mm: Consolidate radix and hash address map details
Michael Ellerman's on April 17, 2019 10:34 pm: > Nicholas Piggin writes: >> Aneesh Kumar K.V's on April 16, 2019 8:07 pm: >>> We now have >>> >>> 4K page size config >>> >>> kernel_region_map_size = 16TB >>> kernel vmalloc start = 0xc0001000 >>> kernel IO start= 0xc0002000 >>> kernel vmemmap start = 0xc0003000 >>> >>> with 64K page size config: >>> >>> kernel_region_map_size = 512TB >>> kernel vmalloc start = 0xc008 >>> kernel IO start= 0xc00a >>> kernel vmemmap start = 0xc00c >> >> Hey Aneesh, >> >> I like the series, I like consolidating the address spaces into 0xc, >> and making the layouts match or similar isn't a bad thing. I don't >> see any real reason to force limitations on one layout or another -- >> you could make the argument that 4k radix should match 64k radix >> as much as matching 4k hash IMO. > > I don't think I agree. The 4K and 64K layouts must be different because > the page size is different and therefore the span of the page tables is > different. (Unless you shrunk the 64K layouts to match 4K but that would > be silly) Both 4K and 64K radix map 52 bits of EA. > On the other hand there's no good reason why hash & radix need to > differ, it's just that radix has a more strictly defined layout and it > didn't match what hash used when we added radix. We really should have > done this realignment of the hash address ranges before we merged radix. Well radix is limited by hardware, hash is more limited by software. There is no real reason to define the addressing limit for radix any less than what the hardware supports, for example. Hash is free to match those limits if that's what we implement but it also does not need to. >> I wouldn't like to tie them too strongly to the same base defines >> that force them to stay in sync. >> >> Can we drop this patch? Or at least keep the users of the H_ and R_ >> defines and set them to the same thing in map.h? > > I don't understand why that would be a good thing? Hash could be expanded in future, radix may be expanded or get different page table layouts. I don't see why you'd tie them together artificially. > We have all this indirection through variables at the moment, for what > appear to be constants. It makes the code harder to follow and it's less > efficient as well. What part is less efficient that matters? Thanks, Nick
Re: [PATCH v3 7/8] powerpc/mm: Consolidate radix and hash address map details
Nicholas Piggin writes: > Aneesh Kumar K.V's on April 16, 2019 8:07 pm: >> We now have >> >> 4K page size config >> >> kernel_region_map_size = 16TB >> kernel vmalloc start = 0xc0001000 >> kernel IO start= 0xc0002000 >> kernel vmemmap start = 0xc0003000 >> >> with 64K page size config: >> >> kernel_region_map_size = 512TB >> kernel vmalloc start = 0xc008 >> kernel IO start= 0xc00a >> kernel vmemmap start = 0xc00c > > Hey Aneesh, > > I like the series, I like consolidating the address spaces into 0xc, > and making the layouts match or similar isn't a bad thing. I don't > see any real reason to force limitations on one layout or another -- > you could make the argument that 4k radix should match 64k radix > as much as matching 4k hash IMO. I don't think I agree. The 4K and 64K layouts must be different because the page size is different and therefore the span of the page tables is different. (Unless you shrunk the 64K layouts to match 4K but that would be silly) On the other hand there's no good reason why hash & radix need to differ, it's just that radix has a more strictly defined layout and it didn't match what hash used when we added radix. We really should have done this realignment of the hash address ranges before we merged radix. > I wouldn't like to tie them too strongly to the same base defines > that force them to stay in sync. > > Can we drop this patch? Or at least keep the users of the H_ and R_ > defines and set them to the same thing in map.h? I don't understand why that would be a good thing? We have all this indirection through variables at the moment, for what appear to be constants. It makes the code harder to follow and it's less efficient as well. cheers
Re: [PATCH v3 7/8] powerpc/mm: Consolidate radix and hash address map details
On 4/16/19 7:33 PM, Nicholas Piggin wrote: Aneesh Kumar K.V's on April 16, 2019 8:07 pm: We now have 4K page size config kernel_region_map_size = 16TB kernel vmalloc start = 0xc0001000 kernel IO start= 0xc0002000 kernel vmemmap start = 0xc0003000 with 64K page size config: kernel_region_map_size = 512TB kernel vmalloc start = 0xc008 kernel IO start= 0xc00a kernel vmemmap start = 0xc00c Hey Aneesh, I like the series, I like consolidating the address spaces into 0xc, and making the layouts match or similar isn't a bad thing. I don't see any real reason to force limitations on one layout or another -- you could make the argument that 4k radix should match 64k radix as much as matching 4k hash IMO. I wouldn't like to tie them too strongly to the same base defines that force them to stay in sync. Can we drop this patch? Or at least keep the users of the H_ and R_ defines and set them to the same thing in map.h? I did that based on the suggestion from Michael Ellerman. I guess he wanted the VMALLOC_START to match. I am not sure whether we should match the kernel_region_map_size too. I did mention that in the cover letter. I agree with your suggestion above. I still can keep the VMALLOC_START at 16TB and keep the region_map_size as 512TB for radix 4k. I am not sure we want to do that. I will wait for feedback from Michael to make the suggested changes. -aneesh
Re: [PATCH v3 7/8] powerpc/mm: Consolidate radix and hash address map details
Aneesh Kumar K.V's on April 16, 2019 8:07 pm: > We now have > > 4K page size config > > kernel_region_map_size = 16TB > kernel vmalloc start = 0xc0001000 > kernel IO start= 0xc0002000 > kernel vmemmap start = 0xc0003000 > > with 64K page size config: > > kernel_region_map_size = 512TB > kernel vmalloc start = 0xc008 > kernel IO start= 0xc00a > kernel vmemmap start = 0xc00c Hey Aneesh, I like the series, I like consolidating the address spaces into 0xc, and making the layouts match or similar isn't a bad thing. I don't see any real reason to force limitations on one layout or another -- you could make the argument that 4k radix should match 64k radix as much as matching 4k hash IMO. I wouldn't like to tie them too strongly to the same base defines that force them to stay in sync. Can we drop this patch? Or at least keep the users of the H_ and R_ defines and set them to the same thing in map.h? > diff --git a/arch/powerpc/include/asm/book3s/64/map.h > b/arch/powerpc/include/asm/book3s/64/map.h > new file mode 100644 > index ..5c01f8c18d61 > --- /dev/null > +++ b/arch/powerpc/include/asm/book3s/64/map.h > @@ -0,0 +1,80 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef _ASM_POWERPC_BOOK3S_64_MAP_H_ > +#define _ASM_POWERPC_BOOK3S_64_MAP_H_ > + > +/* > + * We use MAX_EA_BITS_PER_CONTEXT (hash specific) here just to make sure we > pick > + * the same value for hash and radix. > + */ > +#ifdef CONFIG_PPC_64K_PAGES > + > +/* > + * Each context is 512TB size. SLB miss for first context/default context > + * is handled in the hotpath. Now everything is handled in the slowpath :P I guess that's a copy paste of the comment which my SLB miss patch should have fixed it. Thanks, Nick
