| Message ID | 20201210035526.38938-4-songmuchun@bytedance.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Free some vmemmap pages of HugeTLB page | expand |
On Thu, Dec 10, 2020 at 11:55:17AM +0800, Muchun Song wrote: > Any memory allocated via the memblock allocator and not via the buddy > will be makred reserved already in the memmap. For those pages, we can marked > call free_bootmem_page() to free it to buddy allocator. > > Becasue we wan to free some vmemmap pages of the HugeTLB to the buddy Because want > allocator, we can use this helper to do that in the later patchs. patches To be honest, I think if would be best to introduce this along with patch#4, so we get to see where it gets used. > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > --- > include/linux/bootmem_info.h | 19 +++++++++++++++++++ > 1 file changed, 19 insertions(+) > > diff --git a/include/linux/bootmem_info.h b/include/linux/bootmem_info.h > index 4ed6dee1adc9..20a8b0df0c39 100644 > --- a/include/linux/bootmem_info.h > +++ b/include/linux/bootmem_info.h > @@ -3,6 +3,7 @@ > #define __LINUX_BOOTMEM_INFO_H > > #include <linux/mmzone.h> > +#include <linux/mm.h> <linux/mm.h> already includes <linux/mmzone.h> > +static inline void free_bootmem_page(struct page *page) > +{ > + unsigned long magic = (unsigned long)page->freelist; > + > + /* bootmem page has reserved flag in the reserve_bootmem_region */ reserve_bootmem_region sets the reserved flag on bootmem pages? > + VM_WARN_ON(!PageReserved(page) || page_ref_count(page) != 2); We do check for PageReserved in patch#4 before calling in here. Do we need yet another check here? IOW, do we need to be this paranoid? > + if (magic == SECTION_INFO || magic == MIX_SECTION_INFO) > + put_page_bootmem(page); > + else > + WARN_ON(1); Lately, some people have been complaining about using WARN_ON as some systems come with panic_on_warn set. I would say that in this case it does not matter much as if the vmemmap pages are not either SECTION_INFO or MIX_SECTION_INFO it means that a larger corruption happened elsewhere. But I think I would align the checks here. It does not make sense to me to only scream under DEBUG_VM if page's refcount differs from 2, and have a WARN_ON if the page we are trying to free was not used for the memmap array. Both things imply a corruption, so I would set the checks under the same configurations.
On Thu, Dec 10, 2020 at 10:16 PM Oscar Salvador <osalvador@suse.de> wrote: > > On Thu, Dec 10, 2020 at 11:55:17AM +0800, Muchun Song wrote: > > Any memory allocated via the memblock allocator and not via the buddy > > will be makred reserved already in the memmap. For those pages, we can > marked Thanks. > > call free_bootmem_page() to free it to buddy allocator. > > > > Becasue we wan to free some vmemmap pages of the HugeTLB to the buddy > Because want > > allocator, we can use this helper to do that in the later patchs. > patches > Thanks. > To be honest, I think if would be best to introduce this along with > patch#4, so we get to see where it gets used. > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > --- > > include/linux/bootmem_info.h | 19 +++++++++++++++++++ > > 1 file changed, 19 insertions(+) > > > > diff --git a/include/linux/bootmem_info.h b/include/linux/bootmem_info.h > > index 4ed6dee1adc9..20a8b0df0c39 100644 > > --- a/include/linux/bootmem_info.h > > +++ b/include/linux/bootmem_info.h > > @@ -3,6 +3,7 @@ > > #define __LINUX_BOOTMEM_INFO_H > > > > #include <linux/mmzone.h> > > +#include <linux/mm.h> > > <linux/mm.h> already includes <linux/mmzone.h> Yeah. Can remove this. > > > +static inline void free_bootmem_page(struct page *page) > > +{ > > + unsigned long magic = (unsigned long)page->freelist; > > + > > + /* bootmem page has reserved flag in the reserve_bootmem_region */ > reserve_bootmem_region sets the reserved flag on bootmem pages? Right. > > > + VM_WARN_ON(!PageReserved(page) || page_ref_count(page) != 2); > > We do check for PageReserved in patch#4 before calling in here. > Do we need yet another check here? IOW, do we need to be this paranoid? Yeah, do not need to check again. We can remove it. > > > + if (magic == SECTION_INFO || magic == MIX_SECTION_INFO) > > + put_page_bootmem(page); > > + else > > + WARN_ON(1); > > Lately, some people have been complaining about using WARN_ON as some > systems come with panic_on_warn set. > > I would say that in this case it does not matter much as if the vmemmap > pages are not either SECTION_INFO or MIX_SECTION_INFO it means that a > larger corruption happened elsewhere. > > But I think I would align the checks here. > It does not make sense to me to only scream under DEBUG_VM if page's > refcount differs from 2, and have a WARN_ON if the page we are trying > to free was not used for the memmap array. > Both things imply a corruption, so I would set the checks under the same > configurations. Do you suggest changing them all to VM_DEBUG_ON? > > -- > Oscar Salvador > SUSE L3
On Thu, Dec 10, 2020 at 11:22 PM Muchun Song <songmuchun@bytedance.com> wrote: > > On Thu, Dec 10, 2020 at 10:16 PM Oscar Salvador <osalvador@suse.de> wrote: > > > > On Thu, Dec 10, 2020 at 11:55:17AM +0800, Muchun Song wrote: > > > Any memory allocated via the memblock allocator and not via the buddy > > > will be makred reserved already in the memmap. For those pages, we can > > marked > > Thanks. > > > > call free_bootmem_page() to free it to buddy allocator. > > > > > > Becasue we wan to free some vmemmap pages of the HugeTLB to the buddy > > Because want > > > allocator, we can use this helper to do that in the later patchs. > > patches > > > > Thanks. > > > To be honest, I think if would be best to introduce this along with > > patch#4, so we get to see where it gets used. > > > > > Signed-off-by: Muchun Song <songmuchun@bytedance.com> > > > --- > > > include/linux/bootmem_info.h | 19 +++++++++++++++++++ > > > 1 file changed, 19 insertions(+) > > > > > > diff --git a/include/linux/bootmem_info.h b/include/linux/bootmem_info.h > > > index 4ed6dee1adc9..20a8b0df0c39 100644 > > > --- a/include/linux/bootmem_info.h > > > +++ b/include/linux/bootmem_info.h > > > @@ -3,6 +3,7 @@ > > > #define __LINUX_BOOTMEM_INFO_H > > > > > > #include <linux/mmzone.h> > > > +#include <linux/mm.h> > > > > <linux/mm.h> already includes <linux/mmzone.h> > > Yeah. Can remove this. > > > > > > +static inline void free_bootmem_page(struct page *page) > > > +{ > > > + unsigned long magic = (unsigned long)page->freelist; > > > + > > > + /* bootmem page has reserved flag in the reserve_bootmem_region */ > > reserve_bootmem_region sets the reserved flag on bootmem pages? > > Right. > > > > > > + VM_WARN_ON(!PageReserved(page) || page_ref_count(page) != 2); > > > > We do check for PageReserved in patch#4 before calling in here. > > Do we need yet another check here? IOW, do we need to be this paranoid? > > Yeah, do not need to check again. We can remove it. > > > > > > + if (magic == SECTION_INFO || magic == MIX_SECTION_INFO) > > > + put_page_bootmem(page); > > > + else > > > + WARN_ON(1); > > > > Lately, some people have been complaining about using WARN_ON as some > > systems come with panic_on_warn set. > > > > I would say that in this case it does not matter much as if the vmemmap > > pages are not either SECTION_INFO or MIX_SECTION_INFO it means that a > > larger corruption happened elsewhere. > > > > But I think I would align the checks here. > > It does not make sense to me to only scream under DEBUG_VM if page's > > refcount differs from 2, and have a WARN_ON if the page we are trying > > to free was not used for the memmap array. > > Both things imply a corruption, so I would set the checks under the same > > configurations. > > Do you suggest changing them all to VM_DEBUG_ON? Or VM_WARN_ON? > > > > > -- > > Oscar Salvador > > SUSE L3 > > > > -- > Yours, > Muchun
diff --git a/include/linux/bootmem_info.h b/include/linux/bootmem_info.h index 4ed6dee1adc9..20a8b0df0c39 100644 --- a/include/linux/bootmem_info.h +++ b/include/linux/bootmem_info.h @@ -3,6 +3,7 @@ #define __LINUX_BOOTMEM_INFO_H #include <linux/mmzone.h> +#include <linux/mm.h> /* * Types for free bootmem stored in page->lru.next. These have to be in @@ -22,6 +23,24 @@ void __init register_page_bootmem_info_node(struct pglist_data *pgdat); void get_page_bootmem(unsigned long info, struct page *page, unsigned long type); void put_page_bootmem(struct page *page); + +/* + * Any memory allocated via the memblock allocator and not via the + * buddy will be makred reserved already in the memmap. For those + * pages, we can call this function to free it to buddy allocator. + */ +static inline void free_bootmem_page(struct page *page) +{ + unsigned long magic = (unsigned long)page->freelist; + + /* bootmem page has reserved flag in the reserve_bootmem_region */ + VM_WARN_ON(!PageReserved(page) || page_ref_count(page) != 2); + + if (magic == SECTION_INFO || magic == MIX_SECTION_INFO) + put_page_bootmem(page); + else + WARN_ON(1); +} #else static inline void register_page_bootmem_info_node(struct pglist_data *pgdat) {
Any memory allocated via the memblock allocator and not via the buddy will be makred reserved already in the memmap. For those pages, we can call free_bootmem_page() to free it to buddy allocator. Becasue we wan to free some vmemmap pages of the HugeTLB to the buddy allocator, we can use this helper to do that in the later patchs. Signed-off-by: Muchun Song <songmuchun@bytedance.com> --- include/linux/bootmem_info.h | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+)