Re: [External] Re: [PATCH v8 03/12] mm/bootmem_info: Introduce free_bootmem_page helper
On Thu, Dec 10, 2020 at 11:22 PM Muchun Song wrote:
>
> On Thu, Dec 10, 2020 at 10:16 PM Oscar Salvador 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
> > > ---
> > > 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
> > > +#include
> >
> > already includes
>
> 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
--
Yours,
Muchun
Re: [External] Re: [PATCH v8 03/12] mm/bootmem_info: Introduce free_bootmem_page helper
On Thu, Dec 10, 2020 at 10:16 PM Oscar Salvador 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
> > ---
> > 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
> > +#include
>
> already includes
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
--
Yours,
Muchun
Re: [PATCH v8 03/12] mm/bootmem_info: Introduce free_bootmem_page helper
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
> ---
> 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
> +#include
already includes
> +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.
--
Oscar Salvador
SUSE L3
[PATCH v8 03/12] mm/bootmem_info: Introduce free_bootmem_page helper
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
---
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
+#include
/*
* 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)
{
--
2.11.0
