| Message ID | 1487055180-128750-1-git-send-email-zhouxianrong@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
if the reserved area by user were so big which caused the memmap big, and the reserved area's memamp did not be used by kernel, then user could free the the reserved area's memamp by memblock_mark_raw_pfn interface which is added by me. On 2017/2/14 14:53, zhouxianrong@huawei.com wrote: > From: zhouxianrong <zhouxianrong@huawei.com> > > just like freeing no-map area's memmap (gaps of memblock.memory) > we could free reserved area's memmap (areas of memblock.reserved) > as well only when user of reserved area indicate that we can do > this in drivers. that is, user of reserved area know how to > use the reserved area who could not memblock_free or free_reserved_xxx > the reserved area and regard the area as raw pfn usage by kernel. > the patch supply a way to users who want to utilize the memmap > memory corresponding to raw pfn reserved areas as many as possible. > users can do this by memblock_mark_raw_pfn interface which mark the > reserved area as raw pfn and tell free_unused_memmap that this area's > memmap could be freeed. > > Signed-off-by: zhouxianrong <zhouxianrong@huawei.com> > --- > arch/arm64/mm/init.c | 14 +++++++++++++- > include/linux/memblock.h | 3 +++ > mm/memblock.c | 24 ++++++++++++++++++++++++ > 3 files changed, 40 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c > index 380ebe7..7e62ef8 100644 > --- a/arch/arm64/mm/init.c > +++ b/arch/arm64/mm/init.c > @@ -358,7 +358,7 @@ static inline void free_memmap(unsigned long start_pfn, unsigned long end_pfn) > */ > static void __init free_unused_memmap(void) > { > - unsigned long start, prev_end = 0; > + unsigned long start, end, prev_end = 0; > struct memblock_region *reg; > > for_each_memblock(memory, reg) { > @@ -391,6 +391,18 @@ static void __init free_unused_memmap(void) > if (!IS_ALIGNED(prev_end, PAGES_PER_SECTION)) > free_memmap(prev_end, ALIGN(prev_end, PAGES_PER_SECTION)); > #endif > + > + for_each_memblock(reserved, reg) { > + if (!(reg->flags & MEMBLOCK_RAW_PFN)) > + continue; > + > + start = memblock_region_memory_base_pfn(reg); > + end = round_down(memblock_region_memory_end_pfn(reg), > + MAX_ORDER_NR_PAGES); > + > + if (start < end) > + free_memmap(start, end); > + } > } > #endif /* !CONFIG_SPARSEMEM_VMEMMAP */ > > diff --git a/include/linux/memblock.h b/include/linux/memblock.h > index 5b759c9..9f8d277 100644 > --- a/include/linux/memblock.h > +++ b/include/linux/memblock.h > @@ -26,6 +26,7 @@ enum { > MEMBLOCK_HOTPLUG = 0x1, /* hotpluggable region */ > MEMBLOCK_MIRROR = 0x2, /* mirrored region */ > MEMBLOCK_NOMAP = 0x4, /* don't add to kernel direct mapping */ > + MEMBLOCK_RAW_PFN = 0x8, /* region whose memmap never be used */ > }; > > struct memblock_region { > @@ -92,6 +93,8 @@ bool memblock_overlaps_region(struct memblock_type *type, > int memblock_clear_hotplug(phys_addr_t base, phys_addr_t size); > int memblock_mark_mirror(phys_addr_t base, phys_addr_t size); > int memblock_mark_nomap(phys_addr_t base, phys_addr_t size); > +int memblock_mark_raw_pfn(phys_addr_t base, phys_addr_t size); > +int memblock_clear_raw_pfn(phys_addr_t base, phys_addr_t size); > ulong choose_memblock_flags(void); > > /* Low level functions */ > diff --git a/mm/memblock.c b/mm/memblock.c > index 7608bc3..c103b94 100644 > --- a/mm/memblock.c > +++ b/mm/memblock.c > @@ -814,6 +814,30 @@ int __init_memblock memblock_mark_nomap(phys_addr_t base, phys_addr_t size) > } > > /** > + * memblock_mark_raw_pfn - Mark raw pfn memory with flag MEMBLOCK_RAW_PFN. > + * @base: the base phys addr of the region > + * @size: the size of the region > + * > + * Return 0 on succees, -errno on failure. > + */ > +int __init_memblock memblock_mark_raw_pfn(phys_addr_t base, phys_addr_t size) > +{ > + return memblock_setclr_flag(base, size, 1, MEMBLOCK_RAW_PFN); > +} > + > +/** > + * memblock_clear_raw_pfn - Clear flag MEMBLOCK_RAW_PFN for a specified region. > + * @base: the base phys addr of the region > + * @size: the size of the region > + * > + * Return 0 on succees, -errno on failure. > + */ > +int __init_memblock memblock_clear_raw_pfn(phys_addr_t base, phys_addr_t size) > +{ > + return memblock_setclr_flag(base, size, 0, MEMBLOCK_RAW_PFN); > +} > + > +/** > * __next_reserved_mem_region - next function for for_each_reserved_region() > * @idx: pointer to u64 loop variable > * @out_start: ptr to phys_addr_t for start address of the region, can be %NULL >
On 14 February 2017 at 06:53, <zhouxianrong@huawei.com> wrote: > From: zhouxianrong <zhouxianrong@huawei.com> > > just like freeing no-map area's memmap (gaps of memblock.memory) > we could free reserved area's memmap (areas of memblock.reserved) > as well only when user of reserved area indicate that we can do > this in drivers. that is, user of reserved area know how to > use the reserved area who could not memblock_free or free_reserved_xxx > the reserved area and regard the area as raw pfn usage by kernel. > the patch supply a way to users who want to utilize the memmap > memory corresponding to raw pfn reserved areas as many as possible. > users can do this by memblock_mark_raw_pfn interface which mark the > reserved area as raw pfn and tell free_unused_memmap that this area's > memmap could be freeed. > Could you give an example how much memory we actually recover by doing this? I understand it depends on the size of the reserved regions, but I'm sure you have an actual example that inspired you to write this patch. > Signed-off-by: zhouxianrong <zhouxianrong@huawei.com> > --- > arch/arm64/mm/init.c | 14 +++++++++++++- > include/linux/memblock.h | 3 +++ > mm/memblock.c | 24 ++++++++++++++++++++++++ > 3 files changed, 40 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c > index 380ebe7..7e62ef8 100644 > --- a/arch/arm64/mm/init.c > +++ b/arch/arm64/mm/init.c > @@ -358,7 +358,7 @@ static inline void free_memmap(unsigned long start_pfn, unsigned long end_pfn) > */ > static void __init free_unused_memmap(void) > { > - unsigned long start, prev_end = 0; > + unsigned long start, end, prev_end = 0; > struct memblock_region *reg; > > for_each_memblock(memory, reg) { > @@ -391,6 +391,18 @@ static void __init free_unused_memmap(void) > if (!IS_ALIGNED(prev_end, PAGES_PER_SECTION)) > free_memmap(prev_end, ALIGN(prev_end, PAGES_PER_SECTION)); > #endif > + > + for_each_memblock(reserved, reg) { > + if (!(reg->flags & MEMBLOCK_RAW_PFN)) > + continue; > + > + start = memblock_region_memory_base_pfn(reg); > + end = round_down(memblock_region_memory_end_pfn(reg), > + MAX_ORDER_NR_PAGES); > + Why are you rounding down end only? Shouldn't you round up start and round down end? Or does free_memmap() deal with that already? In any case, it is good to emphasize that on 4 KB pagesize kernels, we will only free multiples of 8 MB that are 8 MB aligned, resulting in 128 KB of memmap backing to be released. > + if (start < end) > + free_memmap(start, end); > + } > } > #endif /* !CONFIG_SPARSEMEM_VMEMMAP */ > > diff --git a/include/linux/memblock.h b/include/linux/memblock.h > index 5b759c9..9f8d277 100644 > --- a/include/linux/memblock.h > +++ b/include/linux/memblock.h > @@ -26,6 +26,7 @@ enum { > MEMBLOCK_HOTPLUG = 0x1, /* hotpluggable region */ > MEMBLOCK_MIRROR = 0x2, /* mirrored region */ > MEMBLOCK_NOMAP = 0x4, /* don't add to kernel direct mapping */ > + MEMBLOCK_RAW_PFN = 0x8, /* region whose memmap never be used */ I think we should be *very* careful about the combinatorial explosion that results when combining all these flags, given that this is not a proper enum but a bit field. In any case, the generic memblock change should be in a separate patch from the arm64 change. > }; > > struct memblock_region { > @@ -92,6 +93,8 @@ bool memblock_overlaps_region(struct memblock_type *type, > int memblock_clear_hotplug(phys_addr_t base, phys_addr_t size); > int memblock_mark_mirror(phys_addr_t base, phys_addr_t size); > int memblock_mark_nomap(phys_addr_t base, phys_addr_t size); > +int memblock_mark_raw_pfn(phys_addr_t base, phys_addr_t size); > +int memblock_clear_raw_pfn(phys_addr_t base, phys_addr_t size); > ulong choose_memblock_flags(void); > > /* Low level functions */ > diff --git a/mm/memblock.c b/mm/memblock.c > index 7608bc3..c103b94 100644 > --- a/mm/memblock.c > +++ b/mm/memblock.c > @@ -814,6 +814,30 @@ int __init_memblock memblock_mark_nomap(phys_addr_t base, phys_addr_t size) > } > > /** > + * memblock_mark_raw_pfn - Mark raw pfn memory with flag MEMBLOCK_RAW_PFN. > + * @base: the base phys addr of the region > + * @size: the size of the region > + * > + * Return 0 on succees, -errno on failure. > + */ > +int __init_memblock memblock_mark_raw_pfn(phys_addr_t base, phys_addr_t size) > +{ > + return memblock_setclr_flag(base, size, 1, MEMBLOCK_RAW_PFN); > +} > + > +/** > + * memblock_clear_raw_pfn - Clear flag MEMBLOCK_RAW_PFN for a specified region. > + * @base: the base phys addr of the region > + * @size: the size of the region > + * > + * Return 0 on succees, -errno on failure. > + */ > +int __init_memblock memblock_clear_raw_pfn(phys_addr_t base, phys_addr_t size) > +{ > + return memblock_setclr_flag(base, size, 0, MEMBLOCK_RAW_PFN); > +} > + > +/** > * __next_reserved_mem_region - next function for for_each_reserved_region() > * @idx: pointer to u64 loop variable > * @out_start: ptr to phys_addr_t for start address of the region, can be %NULL > -- > 1.7.9.5 > > > _______________________________________________ > linux-arm-kernel mailing list > linux-arm-kernel@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On 2017/2/14 17:03, Ard Biesheuvel wrote: > On 14 February 2017 at 06:53, <zhouxianrong@huawei.com> wrote: >> From: zhouxianrong <zhouxianrong@huawei.com> >> >> just like freeing no-map area's memmap (gaps of memblock.memory) >> we could free reserved area's memmap (areas of memblock.reserved) >> as well only when user of reserved area indicate that we can do >> this in drivers. that is, user of reserved area know how to >> use the reserved area who could not memblock_free or free_reserved_xxx >> the reserved area and regard the area as raw pfn usage by kernel. >> the patch supply a way to users who want to utilize the memmap >> memory corresponding to raw pfn reserved areas as many as possible. >> users can do this by memblock_mark_raw_pfn interface which mark the >> reserved area as raw pfn and tell free_unused_memmap that this area's >> memmap could be freeed. >> > > Could you give an example how much memory we actually recover by doing > this? I understand it depends on the size of the reserved regions, but > I'm sure you have an actual example that inspired you to write this > patch. i did statistics in our platform, the memmap of reserved region that can be freed is about 6MB. it's fewer. > >> Signed-off-by: zhouxianrong <zhouxianrong@huawei.com> >> --- >> arch/arm64/mm/init.c | 14 +++++++++++++- >> include/linux/memblock.h | 3 +++ >> mm/memblock.c | 24 ++++++++++++++++++++++++ >> 3 files changed, 40 insertions(+), 1 deletion(-) >> >> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c >> index 380ebe7..7e62ef8 100644 >> --- a/arch/arm64/mm/init.c >> +++ b/arch/arm64/mm/init.c >> @@ -358,7 +358,7 @@ static inline void free_memmap(unsigned long start_pfn, unsigned long end_pfn) >> */ >> static void __init free_unused_memmap(void) >> { >> - unsigned long start, prev_end = 0; >> + unsigned long start, end, prev_end = 0; >> struct memblock_region *reg; >> >> for_each_memblock(memory, reg) { >> @@ -391,6 +391,18 @@ static void __init free_unused_memmap(void) >> if (!IS_ALIGNED(prev_end, PAGES_PER_SECTION)) >> free_memmap(prev_end, ALIGN(prev_end, PAGES_PER_SECTION)); >> #endif >> + >> + for_each_memblock(reserved, reg) { >> + if (!(reg->flags & MEMBLOCK_RAW_PFN)) >> + continue; >> + >> + start = memblock_region_memory_base_pfn(reg); >> + end = round_down(memblock_region_memory_end_pfn(reg), >> + MAX_ORDER_NR_PAGES); >> + > > Why are you rounding down end only? Shouldn't you round up start and > round down end? Or does free_memmap() deal with that already? ok, i could round up start. > > In any case, it is good to emphasize that on 4 KB pagesize kernels, we > will only free multiples of 8 MB that are 8 MB aligned, resulting in > 128 KB of memmap backing to be released. > > >> + if (start < end) >> + free_memmap(start, end); >> + } >> } >> #endif /* !CONFIG_SPARSEMEM_VMEMMAP */ >> >> diff --git a/include/linux/memblock.h b/include/linux/memblock.h >> index 5b759c9..9f8d277 100644 >> --- a/include/linux/memblock.h >> +++ b/include/linux/memblock.h >> @@ -26,6 +26,7 @@ enum { >> MEMBLOCK_HOTPLUG = 0x1, /* hotpluggable region */ >> MEMBLOCK_MIRROR = 0x2, /* mirrored region */ >> MEMBLOCK_NOMAP = 0x4, /* don't add to kernel direct mapping */ >> + MEMBLOCK_RAW_PFN = 0x8, /* region whose memmap never be used */ > > I think we should be *very* careful about the combinatorial explosion > that results when combining all these flags, given that this is not a > proper enum but a bit field. > > In any case, the generic memblock change should be in a separate patch > from the arm64 change. MEMBLOCK_RAW_PFN and MEMBLOCK_NOMAP can not be set at the same time > >> }; >> >> struct memblock_region { >> @@ -92,6 +93,8 @@ bool memblock_overlaps_region(struct memblock_type *type, >> int memblock_clear_hotplug(phys_addr_t base, phys_addr_t size); >> int memblock_mark_mirror(phys_addr_t base, phys_addr_t size); >> int memblock_mark_nomap(phys_addr_t base, phys_addr_t size); >> +int memblock_mark_raw_pfn(phys_addr_t base, phys_addr_t size); >> +int memblock_clear_raw_pfn(phys_addr_t base, phys_addr_t size); >> ulong choose_memblock_flags(void); >> >> /* Low level functions */ >> diff --git a/mm/memblock.c b/mm/memblock.c >> index 7608bc3..c103b94 100644 >> --- a/mm/memblock.c >> +++ b/mm/memblock.c >> @@ -814,6 +814,30 @@ int __init_memblock memblock_mark_nomap(phys_addr_t base, phys_addr_t size) >> } >> >> /** >> + * memblock_mark_raw_pfn - Mark raw pfn memory with flag MEMBLOCK_RAW_PFN. >> + * @base: the base phys addr of the region >> + * @size: the size of the region >> + * >> + * Return 0 on succees, -errno on failure. >> + */ >> +int __init_memblock memblock_mark_raw_pfn(phys_addr_t base, phys_addr_t size) >> +{ >> + return memblock_setclr_flag(base, size, 1, MEMBLOCK_RAW_PFN); >> +} >> + >> +/** >> + * memblock_clear_raw_pfn - Clear flag MEMBLOCK_RAW_PFN for a specified region. >> + * @base: the base phys addr of the region >> + * @size: the size of the region >> + * >> + * Return 0 on succees, -errno on failure. >> + */ >> +int __init_memblock memblock_clear_raw_pfn(phys_addr_t base, phys_addr_t size) >> +{ >> + return memblock_setclr_flag(base, size, 0, MEMBLOCK_RAW_PFN); >> +} >> + >> +/** >> * __next_reserved_mem_region - next function for for_each_reserved_region() >> * @idx: pointer to u64 loop variable >> * @out_start: ptr to phys_addr_t for start address of the region, can be %NULL >> -- >> 1.7.9.5 >> >> >> _______________________________________________ >> linux-arm-kernel mailing list >> linux-arm-kernel@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel > > . >
On 15 February 2017 at 01:44, zhouxianrong <zhouxianrong@huawei.com> wrote: > > > On 2017/2/14 17:03, Ard Biesheuvel wrote: >> >> On 14 February 2017 at 06:53, <zhouxianrong@huawei.com> wrote: >>> >>> From: zhouxianrong <zhouxianrong@huawei.com> >>> >>> just like freeing no-map area's memmap (gaps of memblock.memory) >>> we could free reserved area's memmap (areas of memblock.reserved) >>> as well only when user of reserved area indicate that we can do >>> this in drivers. that is, user of reserved area know how to >>> use the reserved area who could not memblock_free or free_reserved_xxx >>> the reserved area and regard the area as raw pfn usage by kernel. >>> the patch supply a way to users who want to utilize the memmap >>> memory corresponding to raw pfn reserved areas as many as possible. >>> users can do this by memblock_mark_raw_pfn interface which mark the >>> reserved area as raw pfn and tell free_unused_memmap that this area's >>> memmap could be freeed. >>> >> >> Could you give an example how much memory we actually recover by doing >> this? I understand it depends on the size of the reserved regions, but >> I'm sure you have an actual example that inspired you to write this >> patch. > > > i did statistics in our platform, the memmap of reserved region that can be > freed > is about 6MB. it's fewer. > So if you round up the start and round down the end to 8 MB alignment, as you do in this patch, how much do you end up freeing? > >> >>> Signed-off-by: zhouxianrong <zhouxianrong@huawei.com> >>> --- >>> arch/arm64/mm/init.c | 14 +++++++++++++- >>> include/linux/memblock.h | 3 +++ >>> mm/memblock.c | 24 ++++++++++++++++++++++++ >>> 3 files changed, 40 insertions(+), 1 deletion(-) >>> >>> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c >>> index 380ebe7..7e62ef8 100644 >>> --- a/arch/arm64/mm/init.c >>> +++ b/arch/arm64/mm/init.c >>> @@ -358,7 +358,7 @@ static inline void free_memmap(unsigned long >>> start_pfn, unsigned long end_pfn) >>> */ >>> static void __init free_unused_memmap(void) >>> { >>> - unsigned long start, prev_end = 0; >>> + unsigned long start, end, prev_end = 0; >>> struct memblock_region *reg; >>> >>> for_each_memblock(memory, reg) { >>> @@ -391,6 +391,18 @@ static void __init free_unused_memmap(void) >>> if (!IS_ALIGNED(prev_end, PAGES_PER_SECTION)) >>> free_memmap(prev_end, ALIGN(prev_end, >>> PAGES_PER_SECTION)); >>> #endif >>> + >>> + for_each_memblock(reserved, reg) { >>> + if (!(reg->flags & MEMBLOCK_RAW_PFN)) >>> + continue; >>> + >>> + start = memblock_region_memory_base_pfn(reg); >>> + end = round_down(memblock_region_memory_end_pfn(reg), >>> + MAX_ORDER_NR_PAGES); >>> + >> >> >> Why are you rounding down end only? Shouldn't you round up start and >> round down end? Or does free_memmap() deal with that already? > > > ok, i could round up start. > Yes, but is that necessary? Could you explain a) why you round down end, and b) why the reason under a) does not apply to start? >> >> In any case, it is good to emphasize that on 4 KB pagesize kernels, we >> will only free multiples of 8 MB that are 8 MB aligned, resulting in >> 128 KB of memmap backing to be released. > > > >> >> >>> + if (start < end) >>> + free_memmap(start, end); >>> + } >>> } >>> #endif /* !CONFIG_SPARSEMEM_VMEMMAP */ >>> >>> diff --git a/include/linux/memblock.h b/include/linux/memblock.h >>> index 5b759c9..9f8d277 100644 >>> --- a/include/linux/memblock.h >>> +++ b/include/linux/memblock.h >>> @@ -26,6 +26,7 @@ enum { >>> MEMBLOCK_HOTPLUG = 0x1, /* hotpluggable region */ >>> MEMBLOCK_MIRROR = 0x2, /* mirrored region */ >>> MEMBLOCK_NOMAP = 0x4, /* don't add to kernel direct >>> mapping */ >>> + MEMBLOCK_RAW_PFN = 0x8, /* region whose memmap never be >>> used */ >> >> >> I think we should be *very* careful about the combinatorial explosion >> that results when combining all these flags, given that this is not a >> proper enum but a bit field. >> >> In any case, the generic memblock change should be in a separate patch >> from the arm64 change. > > > MEMBLOCK_RAW_PFN and MEMBLOCK_NOMAP can not be set at the same time > They should not. But if I call memblock_mark_raw_pfn() on a MEMBLOCK_NOMAP region, it will have both flags set. In summary, I don't think we need this patch. And if you can convince us otherwise, you should really be more methodical and explicit in implementing this RAW_PFN flag, not add it as a byproduct of the arch code that uses it. Also, you should explain how RAW_PFN relates to NOMAP, and ensure that RAW_PFN and NOMAP regions don't intersect if that is an unsupported combination. Regards, Ard. >> >>> }; >>> >>> struct memblock_region { >>> @@ -92,6 +93,8 @@ bool memblock_overlaps_region(struct memblock_type >>> *type, >>> int memblock_clear_hotplug(phys_addr_t base, phys_addr_t size); >>> int memblock_mark_mirror(phys_addr_t base, phys_addr_t size); >>> int memblock_mark_nomap(phys_addr_t base, phys_addr_t size); >>> +int memblock_mark_raw_pfn(phys_addr_t base, phys_addr_t size); >>> +int memblock_clear_raw_pfn(phys_addr_t base, phys_addr_t size); >>> ulong choose_memblock_flags(void); >>> >>> /* Low level functions */ >>> diff --git a/mm/memblock.c b/mm/memblock.c >>> index 7608bc3..c103b94 100644 >>> --- a/mm/memblock.c >>> +++ b/mm/memblock.c >>> @@ -814,6 +814,30 @@ int __init_memblock memblock_mark_nomap(phys_addr_t >>> base, phys_addr_t size) >>> } >>> >>> /** >>> + * memblock_mark_raw_pfn - Mark raw pfn memory with flag >>> MEMBLOCK_RAW_PFN. >>> + * @base: the base phys addr of the region >>> + * @size: the size of the region >>> + * >>> + * Return 0 on succees, -errno on failure. >>> + */ >>> +int __init_memblock memblock_mark_raw_pfn(phys_addr_t base, phys_addr_t >>> size) >>> +{ >>> + return memblock_setclr_flag(base, size, 1, MEMBLOCK_RAW_PFN); >>> +} >>> + >>> +/** >>> + * memblock_clear_raw_pfn - Clear flag MEMBLOCK_RAW_PFN for a specified >>> region. >>> + * @base: the base phys addr of the region >>> + * @size: the size of the region >>> + * >>> + * Return 0 on succees, -errno on failure. >>> + */ >>> +int __init_memblock memblock_clear_raw_pfn(phys_addr_t base, phys_addr_t >>> size) >>> +{ >>> + return memblock_setclr_flag(base, size, 0, MEMBLOCK_RAW_PFN); >>> +} >>> + >>> +/** >>> * __next_reserved_mem_region - next function for >>> for_each_reserved_region() >>> * @idx: pointer to u64 loop variable >>> * @out_start: ptr to phys_addr_t for start address of the region, can >>> be %NULL >>> -- >>> 1.7.9.5 >>> >>> >>> _______________________________________________ >>> linux-arm-kernel mailing list >>> linux-arm-kernel@lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >> >> >> . >> >
On 2017/2/15 15:10, Ard Biesheuvel wrote: > On 15 February 2017 at 01:44, zhouxianrong <zhouxianrong@huawei.com> wrote: >> >> >> On 2017/2/14 17:03, Ard Biesheuvel wrote: >>> >>> On 14 February 2017 at 06:53, <zhouxianrong@huawei.com> wrote: >>>> >>>> From: zhouxianrong <zhouxianrong@huawei.com> >>>> >>>> just like freeing no-map area's memmap (gaps of memblock.memory) >>>> we could free reserved area's memmap (areas of memblock.reserved) >>>> as well only when user of reserved area indicate that we can do >>>> this in drivers. that is, user of reserved area know how to >>>> use the reserved area who could not memblock_free or free_reserved_xxx >>>> the reserved area and regard the area as raw pfn usage by kernel. >>>> the patch supply a way to users who want to utilize the memmap >>>> memory corresponding to raw pfn reserved areas as many as possible. >>>> users can do this by memblock_mark_raw_pfn interface which mark the >>>> reserved area as raw pfn and tell free_unused_memmap that this area's >>>> memmap could be freeed. >>>> >>> >>> Could you give an example how much memory we actually recover by doing >>> this? I understand it depends on the size of the reserved regions, but >>> I'm sure you have an actual example that inspired you to write this >>> patch. >> >> >> i did statistics in our platform, the memmap of reserved region that can be >> freed >> is about 6MB. it's fewer. >> > > So if you round up the start and round down the end to 8 MB alignment, > as you do in this patch, how much do you end up freeing? > yes, as you say, the size would get fewer if alignment to MAX_ORDER_NR_PAGES in fact, we could not benifit more from this patch because most reserved regions are cma (driver user want to multiplexing of memory with buddy) but cma regions could use struct page so we can not free whose memmap. in our platform only few reserved regions memmap could be freed. this patch aimed at some special big memory usages for reserved region. >> >>> >>>> Signed-off-by: zhouxianrong <zhouxianrong@huawei.com> >>>> --- >>>> arch/arm64/mm/init.c | 14 +++++++++++++- >>>> include/linux/memblock.h | 3 +++ >>>> mm/memblock.c | 24 ++++++++++++++++++++++++ >>>> 3 files changed, 40 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c >>>> index 380ebe7..7e62ef8 100644 >>>> --- a/arch/arm64/mm/init.c >>>> +++ b/arch/arm64/mm/init.c >>>> @@ -358,7 +358,7 @@ static inline void free_memmap(unsigned long >>>> start_pfn, unsigned long end_pfn) >>>> */ >>>> static void __init free_unused_memmap(void) >>>> { >>>> - unsigned long start, prev_end = 0; >>>> + unsigned long start, end, prev_end = 0; >>>> struct memblock_region *reg; >>>> >>>> for_each_memblock(memory, reg) { >>>> @@ -391,6 +391,18 @@ static void __init free_unused_memmap(void) >>>> if (!IS_ALIGNED(prev_end, PAGES_PER_SECTION)) >>>> free_memmap(prev_end, ALIGN(prev_end, >>>> PAGES_PER_SECTION)); >>>> #endif >>>> + >>>> + for_each_memblock(reserved, reg) { >>>> + if (!(reg->flags & MEMBLOCK_RAW_PFN)) >>>> + continue; >>>> + >>>> + start = memblock_region_memory_base_pfn(reg); >>>> + end = round_down(memblock_region_memory_end_pfn(reg), >>>> + MAX_ORDER_NR_PAGES); >>>> + >>> >>> >>> Why are you rounding down end only? Shouldn't you round up start and >>> round down end? Or does free_memmap() deal with that already? >> >> >> ok, i could round up start. >> > > Yes, but is that necessary? Could you explain > a) why you round down end, and > b) why the reason under a) does not apply to start? i made a mistake. for freeing memblock.memory gap, we need alignment by MAX_ORDER_NR_PAGES due to bank ending alignment requirement for freeing memblock.reserved area, because this is within bank, i think we do not need this alignment requirement. but if alignment, rounding up start and rounding down end are natural for memblock.reserved. > >>> >>> In any case, it is good to emphasize that on 4 KB pagesize kernels, we >>> will only free multiples of 8 MB that are 8 MB aligned, resulting in >>> 128 KB of memmap backing to be released. >> >> >> >>> >>> >>>> + if (start < end) >>>> + free_memmap(start, end); >>>> + } >>>> } >>>> #endif /* !CONFIG_SPARSEMEM_VMEMMAP */ >>>> >>>> diff --git a/include/linux/memblock.h b/include/linux/memblock.h >>>> index 5b759c9..9f8d277 100644 >>>> --- a/include/linux/memblock.h >>>> +++ b/include/linux/memblock.h >>>> @@ -26,6 +26,7 @@ enum { >>>> MEMBLOCK_HOTPLUG = 0x1, /* hotpluggable region */ >>>> MEMBLOCK_MIRROR = 0x2, /* mirrored region */ >>>> MEMBLOCK_NOMAP = 0x4, /* don't add to kernel direct >>>> mapping */ >>>> + MEMBLOCK_RAW_PFN = 0x8, /* region whose memmap never be >>>> used */ >>> >>> >>> I think we should be *very* careful about the combinatorial explosion >>> that results when combining all these flags, given that this is not a >>> proper enum but a bit field. >>> >>> In any case, the generic memblock change should be in a separate patch >>> from the arm64 change. >> >> >> MEMBLOCK_RAW_PFN and MEMBLOCK_NOMAP can not be set at the same time >> > > They should not. But if I call memblock_mark_raw_pfn() on a > MEMBLOCK_NOMAP region, it will have both flags set. > > In summary, I don't think we need this patch. And if you can convince > us otherwise, you should really be more methodical and explicit in > implementing this RAW_PFN flag, not add it as a byproduct of the arch > code that uses it. Also, you should explain how RAW_PFN relates to > NOMAP, and ensure that RAW_PFN and NOMAP regions don't intersect if > that is an unsupported combination. yes, setting both MEMBLOCK_RAW_PFN and MEMBLOCK_NOMAP could meet some problems when gaps of memblock.memory intersect memblock.reserved. if they do not intersect, that's ok. so as you said this should be carefully considered. as you think this patch is not needed because, i have showed my idea, it's enough, thanks! > > Regards, > Ard. > > >>> >>>> }; >>>> >>>> struct memblock_region { >>>> @@ -92,6 +93,8 @@ bool memblock_overlaps_region(struct memblock_type >>>> *type, >>>> int memblock_clear_hotplug(phys_addr_t base, phys_addr_t size); >>>> int memblock_mark_mirror(phys_addr_t base, phys_addr_t size); >>>> int memblock_mark_nomap(phys_addr_t base, phys_addr_t size); >>>> +int memblock_mark_raw_pfn(phys_addr_t base, phys_addr_t size); >>>> +int memblock_clear_raw_pfn(phys_addr_t base, phys_addr_t size); >>>> ulong choose_memblock_flags(void); >>>> >>>> /* Low level functions */ >>>> diff --git a/mm/memblock.c b/mm/memblock.c >>>> index 7608bc3..c103b94 100644 >>>> --- a/mm/memblock.c >>>> +++ b/mm/memblock.c >>>> @@ -814,6 +814,30 @@ int __init_memblock memblock_mark_nomap(phys_addr_t >>>> base, phys_addr_t size) >>>> } >>>> >>>> /** >>>> + * memblock_mark_raw_pfn - Mark raw pfn memory with flag >>>> MEMBLOCK_RAW_PFN. >>>> + * @base: the base phys addr of the region >>>> + * @size: the size of the region >>>> + * >>>> + * Return 0 on succees, -errno on failure. >>>> + */ >>>> +int __init_memblock memblock_mark_raw_pfn(phys_addr_t base, phys_addr_t >>>> size) >>>> +{ >>>> + return memblock_setclr_flag(base, size, 1, MEMBLOCK_RAW_PFN); >>>> +} >>>> + >>>> +/** >>>> + * memblock_clear_raw_pfn - Clear flag MEMBLOCK_RAW_PFN for a specified >>>> region. >>>> + * @base: the base phys addr of the region >>>> + * @size: the size of the region >>>> + * >>>> + * Return 0 on succees, -errno on failure. >>>> + */ >>>> +int __init_memblock memblock_clear_raw_pfn(phys_addr_t base, phys_addr_t >>>> size) >>>> +{ >>>> + return memblock_setclr_flag(base, size, 0, MEMBLOCK_RAW_PFN); >>>> +} >>>> + >>>> +/** >>>> * __next_reserved_mem_region - next function for >>>> for_each_reserved_region() >>>> * @idx: pointer to u64 loop variable >>>> * @out_start: ptr to phys_addr_t for start address of the region, can >>>> be %NULL >>>> -- >>>> 1.7.9.5 >>>> >>>> >>>> _______________________________________________ >>>> linux-arm-kernel mailing list >>>> linux-arm-kernel@lists.infradead.org >>>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel >>> >>> >>> . >>> >> > > . >
Add Chen, Catalin On Thu, 16 Feb 2017 09:11:29 +0800 zhouxianrong wrote: > > > On 2017/2/15 15:10, Ard Biesheuvel wrote: > > On 15 February 2017 at 01:44, zhouxianrong wrote: > >> > >> > >> On 2017/2/14 17:03, Ard Biesheuvel wrote: > >>> > >>> On 14 February 2017 at 06:53, <zhouxianrong@huawei.com> wrote: > >>>> > >>>> From: zhouxianrong <zhouxianrong@huawei.com> > >>>> > >>>> just like freeing no-map area's memmap (gaps of memblock.memory) > >>>> we could free reserved area's memmap (areas of memblock.reserved) > >>>> as well only when user of reserved area indicate that we can do > >>>> this in drivers. that is, user of reserved area know how to > >>>> use the reserved area who could not memblock_free or free_reserved_xxx > >>>> the reserved area and regard the area as raw pfn usage by kernel. > >>>> the patch supply a way to users who want to utilize the memmap > >>>> memory corresponding to raw pfn reserved areas as many as possible. > >>>> users can do this by memblock_mark_raw_pfn interface which mark the > >>>> reserved area as raw pfn and tell free_unused_memmap that this area's > >>>> memmap could be freeed. > >>>> > >>> > >>> Could you give an example how much memory we actually recover by doing > >>> this? I understand it depends on the size of the reserved regions, but > >>> I'm sure you have an actual example that inspired you to write this > >>> patch. > >> > >> > >> i did statistics in our platform, the memmap of reserved region that can be > >> freed > >> is about 6MB. it's fewer. <...> > >>> In any case, it is good to emphasize that on 4 KB pagesize kernels, we > >>> will only free multiples of 8 MB that are 8 MB aligned, resulting in > >>> 128 KB of memmap backing to be released. > >> > >> > >> > >>> > >>> > >>>> + if (start < end) > >>>> + free_memmap(start, end); > >>>> + } > >>>> } > >>>> #endif /* !CONFIG_SPARSEMEM_VMEMMAP */ > >>>> > >>>> diff --git a/include/linux/memblock.h b/include/linux/memblock.h > >>>> index 5b759c9..9f8d277 100644 > >>>> --- a/include/linux/memblock.h > >>>> +++ b/include/linux/memblock.h > >>>> @@ -26,6 +26,7 @@ enum { > >>>> MEMBLOCK_HOTPLUG = 0x1, /* hotpluggable region */ > >>>> MEMBLOCK_MIRROR = 0x2, /* mirrored region */ > >>>> MEMBLOCK_NOMAP = 0x4, /* don't add to kernel direct > >>>> mapping */ > >>>> + MEMBLOCK_RAW_PFN = 0x8, /* region whose memmap never be > >>>> used */ > >>> > >>> > >>> I think we should be *very* careful about the combinatorial explosion > >>> that results when combining all these flags, given that this is not a > >>> proper enum but a bit field. > >>> > >>> In any case, the generic memblock change should be in a separate patch > >>> from the arm64 change. > >> > >> > >> MEMBLOCK_RAW_PFN and MEMBLOCK_NOMAP can not be set at the same time > >> > > > > They should not. But if I call memblock_mark_raw_pfn() on a > > MEMBLOCK_NOMAP region, it will have both flags set. > > > > In summary, I don't think we need this patch. And if you can convince > > us otherwise, you should really be more methodical and explicit in > > implementing this RAW_PFN flag, not add it as a byproduct of the arch > > code that uses it. Also, you should explain how RAW_PFN relates to > > NOMAP, and ensure that RAW_PFN and NOMAP regions don't intersect if > > that is an unsupported combination. > > yes, setting both MEMBLOCK_RAW_PFN and MEMBLOCK_NOMAP could meet some problems > when gaps of memblock.memory intersect memblock.reserved. if they do not intersect, > that's ok. so as you said this should be carefully considered. > > as you think this patch is not needed because, i have showed my idea, it's enough, thanks! we are also interested in this area. Just curious, is this patch to "free the vmemmap holes" mentioned by by Catalin in [1]? [1]http://lkml.iu.edu/hypermail/linux/kernel/1604.1/03036.html
On 2017/3/1 18:41, Jisheng Zhang wrote: > Add Chen, Catalin > > On Thu, 16 Feb 2017 09:11:29 +0800 zhouxianrong wrote: >> >> >> On 2017/2/15 15:10, Ard Biesheuvel wrote: >>> On 15 February 2017 at 01:44, zhouxianrong wrote: >>>> >>>> >>>> On 2017/2/14 17:03, Ard Biesheuvel wrote: >>>>> >>>>> On 14 February 2017 at 06:53, <zhouxianrong@huawei.com> wrote: >>>>>> >>>>>> From: zhouxianrong <zhouxianrong@huawei.com> >>>>>> >>>>>> just like freeing no-map area's memmap (gaps of memblock.memory) >>>>>> we could free reserved area's memmap (areas of memblock.reserved) >>>>>> as well only when user of reserved area indicate that we can do >>>>>> this in drivers. that is, user of reserved area know how to >>>>>> use the reserved area who could not memblock_free or free_reserved_xxx >>>>>> the reserved area and regard the area as raw pfn usage by kernel. >>>>>> the patch supply a way to users who want to utilize the memmap >>>>>> memory corresponding to raw pfn reserved areas as many as possible. >>>>>> users can do this by memblock_mark_raw_pfn interface which mark the >>>>>> reserved area as raw pfn and tell free_unused_memmap that this area's >>>>>> memmap could be freeed. >>>>>> >>>>> >>>>> Could you give an example how much memory we actually recover by doing >>>>> this? I understand it depends on the size of the reserved regions, but >>>>> I'm sure you have an actual example that inspired you to write this >>>>> patch. >>>> >>>> >>>> i did statistics in our platform, the memmap of reserved region that can be >>>> freed >>>> is about 6MB. it's fewer. > > <...> > >>>>> In any case, it is good to emphasize that on 4 KB pagesize kernels, we >>>>> will only free multiples of 8 MB that are 8 MB aligned, resulting in >>>>> 128 KB of memmap backing to be released. >>>> >>>> >>>> >>>>> >>>>> >>>>>> + if (start < end) >>>>>> + free_memmap(start, end); >>>>>> + } >>>>>> } >>>>>> #endif /* !CONFIG_SPARSEMEM_VMEMMAP */ >>>>>> >>>>>> diff --git a/include/linux/memblock.h b/include/linux/memblock.h >>>>>> index 5b759c9..9f8d277 100644 >>>>>> --- a/include/linux/memblock.h >>>>>> +++ b/include/linux/memblock.h >>>>>> @@ -26,6 +26,7 @@ enum { >>>>>> MEMBLOCK_HOTPLUG = 0x1, /* hotpluggable region */ >>>>>> MEMBLOCK_MIRROR = 0x2, /* mirrored region */ >>>>>> MEMBLOCK_NOMAP = 0x4, /* don't add to kernel direct >>>>>> mapping */ >>>>>> + MEMBLOCK_RAW_PFN = 0x8, /* region whose memmap never be >>>>>> used */ >>>>> >>>>> >>>>> I think we should be *very* careful about the combinatorial explosion >>>>> that results when combining all these flags, given that this is not a >>>>> proper enum but a bit field. >>>>> >>>>> In any case, the generic memblock change should be in a separate patch >>>>> from the arm64 change. >>>> >>>> >>>> MEMBLOCK_RAW_PFN and MEMBLOCK_NOMAP can not be set at the same time >>>> >>> >>> They should not. But if I call memblock_mark_raw_pfn() on a >>> MEMBLOCK_NOMAP region, it will have both flags set. >>> >>> In summary, I don't think we need this patch. And if you can convince >>> us otherwise, you should really be more methodical and explicit in >>> implementing this RAW_PFN flag, not add it as a byproduct of the arch >>> code that uses it. Also, you should explain how RAW_PFN relates to >>> NOMAP, and ensure that RAW_PFN and NOMAP regions don't intersect if >>> that is an unsupported combination. >> >> yes, setting both MEMBLOCK_RAW_PFN and MEMBLOCK_NOMAP could meet some problems >> when gaps of memblock.memory intersect memblock.reserved. if they do not intersect, >> that's ok. so as you said this should be carefully considered. >> >> as you think this patch is not needed because, i have showed my idea, it's enough, thanks! > > we are also interested in this area. > > Just curious, is this patch to "free the vmemmap holes" mentioned by > by Catalin in [1]? free the vmemmap of reserved memblock (other than no-map regions) whose driver owner know it is never be used. > > [1]http://lkml.iu.edu/hypermail/linux/kernel/1604.1/03036.html > > . >
diff --git a/arch/arm64/mm/init.c b/arch/arm64/mm/init.c index 380ebe7..7e62ef8 100644 --- a/arch/arm64/mm/init.c +++ b/arch/arm64/mm/init.c @@ -358,7 +358,7 @@ static inline void free_memmap(unsigned long start_pfn, unsigned long end_pfn) */ static void __init free_unused_memmap(void) { - unsigned long start, prev_end = 0; + unsigned long start, end, prev_end = 0; struct memblock_region *reg; for_each_memblock(memory, reg) { @@ -391,6 +391,18 @@ static void __init free_unused_memmap(void) if (!IS_ALIGNED(prev_end, PAGES_PER_SECTION)) free_memmap(prev_end, ALIGN(prev_end, PAGES_PER_SECTION)); #endif + + for_each_memblock(reserved, reg) { + if (!(reg->flags & MEMBLOCK_RAW_PFN)) + continue; + + start = memblock_region_memory_base_pfn(reg); + end = round_down(memblock_region_memory_end_pfn(reg), + MAX_ORDER_NR_PAGES); + + if (start < end) + free_memmap(start, end); + } } #endif /* !CONFIG_SPARSEMEM_VMEMMAP */ diff --git a/include/linux/memblock.h b/include/linux/memblock.h index 5b759c9..9f8d277 100644 --- a/include/linux/memblock.h +++ b/include/linux/memblock.h @@ -26,6 +26,7 @@ enum { MEMBLOCK_HOTPLUG = 0x1, /* hotpluggable region */ MEMBLOCK_MIRROR = 0x2, /* mirrored region */ MEMBLOCK_NOMAP = 0x4, /* don't add to kernel direct mapping */ + MEMBLOCK_RAW_PFN = 0x8, /* region whose memmap never be used */ }; struct memblock_region { @@ -92,6 +93,8 @@ bool memblock_overlaps_region(struct memblock_type *type, int memblock_clear_hotplug(phys_addr_t base, phys_addr_t size); int memblock_mark_mirror(phys_addr_t base, phys_addr_t size); int memblock_mark_nomap(phys_addr_t base, phys_addr_t size); +int memblock_mark_raw_pfn(phys_addr_t base, phys_addr_t size); +int memblock_clear_raw_pfn(phys_addr_t base, phys_addr_t size); ulong choose_memblock_flags(void); /* Low level functions */ diff --git a/mm/memblock.c b/mm/memblock.c index 7608bc3..c103b94 100644 --- a/mm/memblock.c +++ b/mm/memblock.c @@ -814,6 +814,30 @@ int __init_memblock memblock_mark_nomap(phys_addr_t base, phys_addr_t size) } /** + * memblock_mark_raw_pfn - Mark raw pfn memory with flag MEMBLOCK_RAW_PFN. + * @base: the base phys addr of the region + * @size: the size of the region + * + * Return 0 on succees, -errno on failure. + */ +int __init_memblock memblock_mark_raw_pfn(phys_addr_t base, phys_addr_t size) +{ + return memblock_setclr_flag(base, size, 1, MEMBLOCK_RAW_PFN); +} + +/** + * memblock_clear_raw_pfn - Clear flag MEMBLOCK_RAW_PFN for a specified region. + * @base: the base phys addr of the region + * @size: the size of the region + * + * Return 0 on succees, -errno on failure. + */ +int __init_memblock memblock_clear_raw_pfn(phys_addr_t base, phys_addr_t size) +{ + return memblock_setclr_flag(base, size, 0, MEMBLOCK_RAW_PFN); +} + +/** * __next_reserved_mem_region - next function for for_each_reserved_region() * @idx: pointer to u64 loop variable * @out_start: ptr to phys_addr_t for start address of the region, can be %NULL