| Message ID | 20190508224727.11549-3-sstabellini@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/3] xen/arm: fix nr_pdxs calculation | expand |
>>> On 09.05.19 at 00:47, <sstabellini@kernel.org> wrote: > --- a/xen/common/pdx.c > +++ b/xen/common/pdx.c > @@ -50,9 +50,13 @@ static u64 __init fill_mask(u64 mask) > return mask; > } > > +/* > + * We always map the first 1<<MAX_ORDER pages of RAM, hence, they > + * are left uncompressed. > + */ > u64 __init pdx_init_mask(u64 base_addr) > { > - return fill_mask(base_addr - 1); > + return fill_mask(max(base_addr, (u64)1 << (MAX_ORDER + PAGE_SHIFT)) - 1); Personally I think that despite the surrounding u64 this should be uint64_t. You could avoid this altogether by using 1ULL. > @@ -80,6 +84,8 @@ void __init pfn_pdx_hole_setup(unsigned long mask) > * This guarantees that page-pointer arithmetic remains valid within > * contiguous aligned ranges of 2^MAX_ORDER pages. Among others, our > * buddy allocator relies on this assumption. > + * > + * If the logic changes here, we might have to update init_pdx too. > */ > for ( j = MAX_ORDER-1; ; ) > { At first I was puzzled by a reference to something that I didn't think would exist, and I was then assuming you mean pdx_init_mask(). But then I thought I'd use grep nevertheless, and found the Arm instance of it (which this patch actually changes, but I'm rarely looking at the "diff -p" symbols when context is otherwise obvious). If you make such a reference in common code (I think I'd prefer if it was simply omitted), then please name the specific architecture as well, or make otherwise clear that this isn't a symbol that's common or required to be supplied by each arch. Jan
Hi Stefano, On 5/8/19 11:47 PM, Stefano Stabellini wrote: > The mask calculation in pdx_init_mask is wrong when the first bank > starts at address 0x0. The reason is that pdx_init_mask will do '0 - 1' > causing an underflow. As a result, the mask becomes 0xffffffffffffffff > which is the biggest possible mask and ends up causing a significant > memory waste in the frametable size computation. > > For instance, on platforms that have a low memory bank starting at 0x0 > and a high memory bank, the frametable will end up covering all the > holes in between. > > The purpose of the mask is to be passed as a parameter to > pfn_pdx_hole_setup, which based on the mask parameter calculates > pfn_pdx_hole_shift, pfn_pdx_bottom_mask, etc. which are actually the > important masks for frametable initialization later on. > > pfn_pdx_hole_setup never compresses addresses below MAX_ORDER bits (1GB > on ARM). Thus, it is safe to initialize mask passing 1ULL << (MAX_ORDER > + PAGE_SHIFT) as start address to pdx_init_mask. > > Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> > CC: JBeulich@suse.com > CC: andrew.cooper3@citrix.com > CC: George.Dunlap@eu.citrix.com > CC: ian.jackson@eu.citrix.com > CC: konrad.wilk@oracle.com > CC: tim@xen.org > CC: wei.liu2@citrix.com > --- > > Changes in v2: > - update commit message > - add in-code comments regarding update sites > - improve in-code comments > - move the mask initialization changes to pdx_init_mask > --- > xen/arch/arm/setup.c | 9 ++++++++- > xen/common/pdx.c | 8 +++++++- > 2 files changed, 15 insertions(+), 2 deletions(-) > > diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c > index ccb0f181ea..afaafe7b84 100644 > --- a/xen/arch/arm/setup.c > +++ b/xen/arch/arm/setup.c > @@ -482,7 +482,14 @@ static void __init init_pdx(void) > { > paddr_t bank_start, bank_size, bank_end; > > - u64 mask = pdx_init_mask(bootinfo.mem.bank[0].start); > + /* > + * Pass 0x0 to pdx_init_mask to get a mask initialized with the > + * first to 1<<MAX_ORDER pages of RAM left uncompressed. What matter is Arm doesn't have any specific restriction on the compression, but the common code may have. So how about something: "Arm does not have any restriction on the bits to compress. Pass 0 to let the common code to further restrict". > + * > + * If the logic changes in pfn_pdx_hole_setup we might have to > + * update this function too. > + */ > + u64 mask = pdx_init_mask(0x0); > int bank; > > for ( bank = 0 ; bank < bootinfo.mem.nr_banks; bank++ ) > diff --git a/xen/common/pdx.c b/xen/common/pdx.c > index bb7e437049..268d6f7ec3 100644 > --- a/xen/common/pdx.c > +++ b/xen/common/pdx.c > @@ -50,9 +50,13 @@ static u64 __init fill_mask(u64 mask) > return mask; > } > > +/* > + * We always map the first 1<<MAX_ORDER pages of RAM, hence, they I thought I already pointed out in the previous version. At least on Arm, we never map the first 1 << MAX_ORDER of RAM. Instead what you want to say is that we don't compress the first N bits of the address. > + * are left uncompressed. > + */ Cheers,
On Thu, 9 May 2019, Jan Beulich wrote: > >>> On 09.05.19 at 00:47, <sstabellini@kernel.org> wrote: > > --- a/xen/common/pdx.c > > +++ b/xen/common/pdx.c > > @@ -50,9 +50,13 @@ static u64 __init fill_mask(u64 mask) > > return mask; > > } > > > > +/* > > + * We always map the first 1<<MAX_ORDER pages of RAM, hence, they > > + * are left uncompressed. > > + */ > > u64 __init pdx_init_mask(u64 base_addr) > > { > > - return fill_mask(base_addr - 1); > > + return fill_mask(max(base_addr, (u64)1 << (MAX_ORDER + PAGE_SHIFT)) - 1); > > Personally I think that despite the surrounding u64 this should be > uint64_t. You could avoid this altogether by using 1ULL. I cannot use 1ULL because it would break the build: the reason is that u64 is defined as unsigned long on some architectures and unsigned long long on others. The pointers comparison in the max macro will fail to compile. I could use uint64_t, that works, but I think is not a good idea to use potentially different types between the arguments passed to max. If you still would like me to change (u64)1 to (uint64_t)1 please explain why in more details. > > @@ -80,6 +84,8 @@ void __init pfn_pdx_hole_setup(unsigned long mask) > > * This guarantees that page-pointer arithmetic remains valid within > > * contiguous aligned ranges of 2^MAX_ORDER pages. Among others, our > > * buddy allocator relies on this assumption. > > + * > > + * If the logic changes here, we might have to update init_pdx too. > > */ > > for ( j = MAX_ORDER-1; ; ) > > { > > At first I was puzzled by a reference to something that I didn't > think would exist, and I was then assuming you mean > pdx_init_mask(). But then I thought I'd use grep nevertheless, > and found the Arm instance of it (which this patch actually > changes, but I'm rarely looking at the "diff -p" symbols when > context is otherwise obvious). If you make such a reference in > common code (I think I'd prefer if it was simply omitted), then > please name the specific architecture as well, or make otherwise > clear that this isn't a symbol that's common or required to be > supplied by each arch. I'll make it clear
Hi Stefano, On 6/3/19 10:56 PM, Stefano Stabellini wrote: > On Thu, 9 May 2019, Jan Beulich wrote: >>>>> On 09.05.19 at 00:47, <sstabellini@kernel.org> wrote: >>> --- a/xen/common/pdx.c >>> +++ b/xen/common/pdx.c >>> @@ -50,9 +50,13 @@ static u64 __init fill_mask(u64 mask) >>> return mask; >>> } >>> >>> +/* >>> + * We always map the first 1<<MAX_ORDER pages of RAM, hence, they >>> + * are left uncompressed. >>> + */ >>> u64 __init pdx_init_mask(u64 base_addr) >>> { >>> - return fill_mask(base_addr - 1); >>> + return fill_mask(max(base_addr, (u64)1 << (MAX_ORDER + PAGE_SHIFT)) - 1); >> >> Personally I think that despite the surrounding u64 this should be >> uint64_t. You could avoid this altogether by using 1ULL. > > I cannot use 1ULL because it would break the build: the reason is that > u64 is defined as unsigned long on some architectures and unsigned long > long on others. The pointers comparison in the max macro will fail to > compile. > > I could use uint64_t, that works, but I think is not a good idea to use > potentially different types between the arguments passed to max. If you > still would like me to change (u64)1 to (uint64_t)1 please explain why > in more details. We are phasing out uXX in favor of uintXX_t so I agree with Jan that we want to avoid introducing more here. Except the way they are defined, u64 and uint64_t will always be equal to 64-bits. So you can easily update the interface to use uint64_t instead of u64 without worrying about type issue. Cheers,
>>> On 04.06.19 at 00:02, <julien.grall@arm.com> wrote: > On 6/3/19 10:56 PM, Stefano Stabellini wrote: >> On Thu, 9 May 2019, Jan Beulich wrote: >>>>>> On 09.05.19 at 00:47, <sstabellini@kernel.org> wrote: >>>> --- a/xen/common/pdx.c >>>> +++ b/xen/common/pdx.c >>>> @@ -50,9 +50,13 @@ static u64 __init fill_mask(u64 mask) >>>> return mask; >>>> } >>>> >>>> +/* >>>> + * We always map the first 1<<MAX_ORDER pages of RAM, hence, they >>>> + * are left uncompressed. >>>> + */ >>>> u64 __init pdx_init_mask(u64 base_addr) >>>> { >>>> - return fill_mask(base_addr - 1); >>>> + return fill_mask(max(base_addr, (u64)1 << (MAX_ORDER + PAGE_SHIFT)) - 1); >>> >>> Personally I think that despite the surrounding u64 this should be >>> uint64_t. You could avoid this altogether by using 1ULL. >> >> I cannot use 1ULL because it would break the build: the reason is that >> u64 is defined as unsigned long on some architectures and unsigned long >> long on others. The pointers comparison in the max macro will fail to >> compile. Hmm, ugly - we indeed have UINT64_C() only in crypto code right now. >> I could use uint64_t, that works, but I think is not a good idea to use >> potentially different types between the arguments passed to max. If you >> still would like me to change (u64)1 to (uint64_t)1 please explain why >> in more details. > > We are phasing out uXX in favor of uintXX_t so I agree with Jan that we > want to avoid introducing more here. Is this sufficient detail for you? (Honestly I wouldn't what else to add.) Jan
diff --git a/xen/arch/arm/setup.c b/xen/arch/arm/setup.c index ccb0f181ea..afaafe7b84 100644 --- a/xen/arch/arm/setup.c +++ b/xen/arch/arm/setup.c @@ -482,7 +482,14 @@ static void __init init_pdx(void) { paddr_t bank_start, bank_size, bank_end; - u64 mask = pdx_init_mask(bootinfo.mem.bank[0].start); + /* + * Pass 0x0 to pdx_init_mask to get a mask initialized with the + * first to 1<<MAX_ORDER pages of RAM left uncompressed. + * + * If the logic changes in pfn_pdx_hole_setup we might have to + * update this function too. + */ + u64 mask = pdx_init_mask(0x0); int bank; for ( bank = 0 ; bank < bootinfo.mem.nr_banks; bank++ ) diff --git a/xen/common/pdx.c b/xen/common/pdx.c index bb7e437049..268d6f7ec3 100644 --- a/xen/common/pdx.c +++ b/xen/common/pdx.c @@ -50,9 +50,13 @@ static u64 __init fill_mask(u64 mask) return mask; } +/* + * We always map the first 1<<MAX_ORDER pages of RAM, hence, they + * are left uncompressed. + */ u64 __init pdx_init_mask(u64 base_addr) { - return fill_mask(base_addr - 1); + return fill_mask(max(base_addr, (u64)1 << (MAX_ORDER + PAGE_SHIFT)) - 1); } u64 __init pdx_region_mask(u64 base, u64 len) @@ -80,6 +84,8 @@ void __init pfn_pdx_hole_setup(unsigned long mask) * This guarantees that page-pointer arithmetic remains valid within * contiguous aligned ranges of 2^MAX_ORDER pages. Among others, our * buddy allocator relies on this assumption. + * + * If the logic changes here, we might have to update init_pdx too. */ for ( j = MAX_ORDER-1; ; ) {
The mask calculation in pdx_init_mask is wrong when the first bank starts at address 0x0. The reason is that pdx_init_mask will do '0 - 1' causing an underflow. As a result, the mask becomes 0xffffffffffffffff which is the biggest possible mask and ends up causing a significant memory waste in the frametable size computation. For instance, on platforms that have a low memory bank starting at 0x0 and a high memory bank, the frametable will end up covering all the holes in between. The purpose of the mask is to be passed as a parameter to pfn_pdx_hole_setup, which based on the mask parameter calculates pfn_pdx_hole_shift, pfn_pdx_bottom_mask, etc. which are actually the important masks for frametable initialization later on. pfn_pdx_hole_setup never compresses addresses below MAX_ORDER bits (1GB on ARM). Thus, it is safe to initialize mask passing 1ULL << (MAX_ORDER + PAGE_SHIFT) as start address to pdx_init_mask. Signed-off-by: Stefano Stabellini <stefanos@xilinx.com> CC: JBeulich@suse.com CC: andrew.cooper3@citrix.com CC: George.Dunlap@eu.citrix.com CC: ian.jackson@eu.citrix.com CC: konrad.wilk@oracle.com CC: tim@xen.org CC: wei.liu2@citrix.com --- Changes in v2: - update commit message - add in-code comments regarding update sites - improve in-code comments - move the mask initialization changes to pdx_init_mask --- xen/arch/arm/setup.c | 9 ++++++++- xen/common/pdx.c | 8 +++++++- 2 files changed, 15 insertions(+), 2 deletions(-)