| Message ID | 20201111204008.21332-4-zi.yan@sent.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Split huge pages to any lower order pages. | expand |
On 11/11/20 12:40 PM, Zi Yan wrote: > From: Zi Yan <ziy@nvidia.com> > > It adds a new_order parameter to set new page order in page owner. > It prepares for upcoming changes to support split huge page to any lower > order. > > Signed-off-by: Zi Yan <ziy@nvidia.com> Except for a minor fix below, you can add: Reviewed-by: Ralph Campbell <rcampbell@nvidia.com> > --- > include/linux/page_owner.h | 7 ++++--- > mm/huge_memory.c | 2 +- > mm/page_alloc.c | 2 +- > mm/page_owner.c | 6 +++--- > 4 files changed, 9 insertions(+), 8 deletions(-) > > diff --git a/include/linux/page_owner.h b/include/linux/page_owner.h > index 3468794f83d2..215cbb159568 100644 > --- a/include/linux/page_owner.h > +++ b/include/linux/page_owner.h > @@ -31,10 +31,11 @@ static inline void set_page_owner(struct page *page, > __set_page_owner(page, order, gfp_mask); > } > > -static inline void split_page_owner(struct page *page, unsigned int nr) > +static inline void split_page_owner(struct page *page, unsigned int nr, > + unsigned int new_order) > { > if (static_branch_unlikely(&page_owner_inited)) > - __split_page_owner(page, nr); > + __split_page_owner(page, nr, new_order); > } > static inline void copy_page_owner(struct page *oldpage, struct page *newpage) > { > @@ -60,7 +61,7 @@ static inline void set_page_owner(struct page *page, > { > } > static inline void split_page_owner(struct page *page, > - unsigned int order) > + unsigned int nr, unsigned int new_order) > { > } > static inline void copy_page_owner(struct page *oldpage, struct page *newpage) > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > index f599f5b9bf7f..8b7d771ee962 100644 > --- a/mm/huge_memory.c > +++ b/mm/huge_memory.c > @@ -2459,7 +2459,7 @@ static void __split_huge_page(struct page *page, struct list_head *list, > > ClearPageCompound(head); > > - split_page_owner(head, nr); > + split_page_owner(head, nr, 1); Shouldn't this be 0, not 1? (new_order not new_nr). > /* See comment in __split_huge_page_tail() */ > if (PageAnon(head)) { > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index d77220615fd5..a9eead0e091a 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -3284,7 +3284,7 @@ void split_page(struct page *page, unsigned int order) > > for (i = 1; i < (1 << order); i++) > set_page_refcounted(page + i); > - split_page_owner(page, 1 << order); > + split_page_owner(page, 1 << order, 1); Ditto, 0. > } > EXPORT_SYMBOL_GPL(split_page); > diff --git a/mm/page_owner.c b/mm/page_owner.c > index b735a8eafcdb..2b7f7e9056dc 100644 > --- a/mm/page_owner.c > +++ b/mm/page_owner.c > @@ -204,7 +204,7 @@ void __set_page_owner_migrate_reason(struct page *page, int reason) > page_owner->last_migrate_reason = reason; > } > > -void __split_page_owner(struct page *page, unsigned int nr) > +void __split_page_owner(struct page *page, unsigned int nr, unsigned int new_order) > { > int i; > struct page_ext *page_ext = lookup_page_ext(page); > @@ -213,9 +213,9 @@ void __split_page_owner(struct page *page, unsigned int nr) > if (unlikely(!page_ext)) > return; > > - for (i = 0; i < nr; i++) { > + for (i = 0; i < nr; i += (1 << new_order)) { > page_owner = get_page_owner(page_ext); > - page_owner->order = 0; > + page_owner->order = new_order; > page_ext = page_ext_next(page_ext); > } > } >
On 12 Nov 2020, at 12:57, Ralph Campbell wrote: > On 11/11/20 12:40 PM, Zi Yan wrote: >> From: Zi Yan <ziy@nvidia.com> >> >> It adds a new_order parameter to set new page order in page owner. >> It prepares for upcoming changes to support split huge page to any lower >> order. >> >> Signed-off-by: Zi Yan <ziy@nvidia.com> > > Except for a minor fix below, you can add: > Reviewed-by: Ralph Campbell <rcampbell@nvidia.com> Thanks. > >> --- >> include/linux/page_owner.h | 7 ++++--- >> mm/huge_memory.c | 2 +- >> mm/page_alloc.c | 2 +- >> mm/page_owner.c | 6 +++--- >> 4 files changed, 9 insertions(+), 8 deletions(-) >> >> diff --git a/include/linux/page_owner.h b/include/linux/page_owner.h >> index 3468794f83d2..215cbb159568 100644 >> --- a/include/linux/page_owner.h >> +++ b/include/linux/page_owner.h >> @@ -31,10 +31,11 @@ static inline void set_page_owner(struct page *page, >> __set_page_owner(page, order, gfp_mask); >> } >> -static inline void split_page_owner(struct page *page, unsigned int nr) >> +static inline void split_page_owner(struct page *page, unsigned int nr, >> + unsigned int new_order) >> { >> if (static_branch_unlikely(&page_owner_inited)) >> - __split_page_owner(page, nr); >> + __split_page_owner(page, nr, new_order); >> } >> static inline void copy_page_owner(struct page *oldpage, struct page *newpage) >> { >> @@ -60,7 +61,7 @@ static inline void set_page_owner(struct page *page, >> { >> } >> static inline void split_page_owner(struct page *page, >> - unsigned int order) >> + unsigned int nr, unsigned int new_order) >> { >> } >> static inline void copy_page_owner(struct page *oldpage, struct page *newpage) >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >> index f599f5b9bf7f..8b7d771ee962 100644 >> --- a/mm/huge_memory.c >> +++ b/mm/huge_memory.c >> @@ -2459,7 +2459,7 @@ static void __split_huge_page(struct page *page, struct list_head *list, >> ClearPageCompound(head); >> - split_page_owner(head, nr); >> + split_page_owner(head, nr, 1); > > Shouldn't this be 0, not 1? > (new_order not new_nr). > Yes, I forgot to fix the call site after I change the function signature. Thanks. >> /* See comment in __split_huge_page_tail() */ >> if (PageAnon(head)) { >> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >> index d77220615fd5..a9eead0e091a 100644 >> --- a/mm/page_alloc.c >> +++ b/mm/page_alloc.c >> @@ -3284,7 +3284,7 @@ void split_page(struct page *page, unsigned int order) >> for (i = 1; i < (1 << order); i++) >> set_page_refcounted(page + i); >> - split_page_owner(page, 1 << order); >> + split_page_owner(page, 1 << order, 1); > > Ditto, 0. > Sure, will fix this too. — Best Regards, Yan Zi
On Wed, Nov 11, 2020 at 03:40:05PM -0500, Zi Yan wrote: > From: Zi Yan <ziy@nvidia.com> > > It adds a new_order parameter to set new page order in page owner. > It prepares for upcoming changes to support split huge page to any lower > order. > > Signed-off-by: Zi Yan <ziy@nvidia.com> > --- > include/linux/page_owner.h | 7 ++++--- > mm/huge_memory.c | 2 +- > mm/page_alloc.c | 2 +- > mm/page_owner.c | 6 +++--- > 4 files changed, 9 insertions(+), 8 deletions(-) > > diff --git a/include/linux/page_owner.h b/include/linux/page_owner.h > index 3468794f83d2..215cbb159568 100644 > --- a/include/linux/page_owner.h > +++ b/include/linux/page_owner.h > @@ -31,10 +31,11 @@ static inline void set_page_owner(struct page *page, > __set_page_owner(page, order, gfp_mask); > } > > -static inline void split_page_owner(struct page *page, unsigned int nr) > +static inline void split_page_owner(struct page *page, unsigned int nr, > + unsigned int new_order) > { > if (static_branch_unlikely(&page_owner_inited)) > - __split_page_owner(page, nr); > + __split_page_owner(page, nr, new_order); > } > static inline void copy_page_owner(struct page *oldpage, struct page *newpage) > { > @@ -60,7 +61,7 @@ static inline void set_page_owner(struct page *page, > { > } > static inline void split_page_owner(struct page *page, > - unsigned int order) > + unsigned int nr, unsigned int new_order) With the addition of the new argument it's a bit hard to understand what the function is supposed to do. It seems like nr == page_order(page), is it right? Maybe we can pass old_order and new_order? Or just the page and the new order? > { > } > static inline void copy_page_owner(struct page *oldpage, struct page *newpage) > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > index f599f5b9bf7f..8b7d771ee962 100644 > --- a/mm/huge_memory.c > +++ b/mm/huge_memory.c > @@ -2459,7 +2459,7 @@ static void __split_huge_page(struct page *page, struct list_head *list, > > ClearPageCompound(head); > > - split_page_owner(head, nr); > + split_page_owner(head, nr, 1); > > /* See comment in __split_huge_page_tail() */ > if (PageAnon(head)) { > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index d77220615fd5..a9eead0e091a 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -3284,7 +3284,7 @@ void split_page(struct page *page, unsigned int order) > > for (i = 1; i < (1 << order); i++) > set_page_refcounted(page + i); > - split_page_owner(page, 1 << order); > + split_page_owner(page, 1 << order, 1); > } > EXPORT_SYMBOL_GPL(split_page); > > diff --git a/mm/page_owner.c b/mm/page_owner.c > index b735a8eafcdb..2b7f7e9056dc 100644 > --- a/mm/page_owner.c > +++ b/mm/page_owner.c > @@ -204,7 +204,7 @@ void __set_page_owner_migrate_reason(struct page *page, int reason) > page_owner->last_migrate_reason = reason; > } > > -void __split_page_owner(struct page *page, unsigned int nr) > +void __split_page_owner(struct page *page, unsigned int nr, unsigned int new_order) > { > int i; > struct page_ext *page_ext = lookup_page_ext(page); > @@ -213,9 +213,9 @@ void __split_page_owner(struct page *page, unsigned int nr) > if (unlikely(!page_ext)) > return; > > - for (i = 0; i < nr; i++) { > + for (i = 0; i < nr; i += (1 << new_order)) { > page_owner = get_page_owner(page_ext); > - page_owner->order = 0; > + page_owner->order = new_order; > page_ext = page_ext_next(page_ext); I believe there cannot be any leftovers because nr is always a power of 2. Is it true? Converting nr argument to order (if it's possible) will make it obvious. Other than that the patch looks good to me. Thanks!
On 13 Nov 2020, at 19:15, Roman Gushchin wrote: > On Wed, Nov 11, 2020 at 03:40:05PM -0500, Zi Yan wrote: >> From: Zi Yan <ziy@nvidia.com> >> >> It adds a new_order parameter to set new page order in page owner. >> It prepares for upcoming changes to support split huge page to any >> lower >> order. >> >> Signed-off-by: Zi Yan <ziy@nvidia.com> >> --- >> include/linux/page_owner.h | 7 ++++--- >> mm/huge_memory.c | 2 +- >> mm/page_alloc.c | 2 +- >> mm/page_owner.c | 6 +++--- >> 4 files changed, 9 insertions(+), 8 deletions(-) >> >> diff --git a/include/linux/page_owner.h b/include/linux/page_owner.h >> index 3468794f83d2..215cbb159568 100644 >> --- a/include/linux/page_owner.h >> +++ b/include/linux/page_owner.h >> @@ -31,10 +31,11 @@ static inline void set_page_owner(struct page >> *page, >> __set_page_owner(page, order, gfp_mask); >> } >> >> -static inline void split_page_owner(struct page *page, unsigned int >> nr) >> +static inline void split_page_owner(struct page *page, unsigned int >> nr, >> + unsigned int new_order) >> { >> if (static_branch_unlikely(&page_owner_inited)) >> - __split_page_owner(page, nr); >> + __split_page_owner(page, nr, new_order); >> } >> static inline void copy_page_owner(struct page *oldpage, struct page >> *newpage) >> { >> @@ -60,7 +61,7 @@ static inline void set_page_owner(struct page >> *page, >> { >> } >> static inline void split_page_owner(struct page *page, >> - unsigned int order) >> + unsigned int nr, unsigned int new_order) > > With the addition of the new argument it's a bit hard to understand > what the function is supposed to do. It seems like nr == > page_order(page), > is it right? Maybe we can pass old_order and new_order? Or just the > page > and the new order? Yeah, it is a bit confusing. Please see more below. > >> { >> } >> static inline void copy_page_owner(struct page *oldpage, struct page >> *newpage) >> diff --git a/mm/huge_memory.c b/mm/huge_memory.c >> index f599f5b9bf7f..8b7d771ee962 100644 >> --- a/mm/huge_memory.c >> +++ b/mm/huge_memory.c >> @@ -2459,7 +2459,7 @@ static void __split_huge_page(struct page >> *page, struct list_head *list, >> >> ClearPageCompound(head); >> >> - split_page_owner(head, nr); >> + split_page_owner(head, nr, 1); >> >> /* See comment in __split_huge_page_tail() */ >> if (PageAnon(head)) { >> diff --git a/mm/page_alloc.c b/mm/page_alloc.c >> index d77220615fd5..a9eead0e091a 100644 >> --- a/mm/page_alloc.c >> +++ b/mm/page_alloc.c >> @@ -3284,7 +3284,7 @@ void split_page(struct page *page, unsigned int >> order) >> >> for (i = 1; i < (1 << order); i++) >> set_page_refcounted(page + i); >> - split_page_owner(page, 1 << order); >> + split_page_owner(page, 1 << order, 1); >> } >> EXPORT_SYMBOL_GPL(split_page); >> >> diff --git a/mm/page_owner.c b/mm/page_owner.c >> index b735a8eafcdb..2b7f7e9056dc 100644 >> --- a/mm/page_owner.c >> +++ b/mm/page_owner.c >> @@ -204,7 +204,7 @@ void __set_page_owner_migrate_reason(struct page >> *page, int reason) >> page_owner->last_migrate_reason = reason; >> } >> >> -void __split_page_owner(struct page *page, unsigned int nr) >> +void __split_page_owner(struct page *page, unsigned int nr, unsigned >> int new_order) >> { >> int i; >> struct page_ext *page_ext = lookup_page_ext(page); >> @@ -213,9 +213,9 @@ void __split_page_owner(struct page *page, >> unsigned int nr) >> if (unlikely(!page_ext)) >> return; >> >> - for (i = 0; i < nr; i++) { >> + for (i = 0; i < nr; i += (1 << new_order)) { >> page_owner = get_page_owner(page_ext); >> - page_owner->order = 0; >> + page_owner->order = new_order; >> page_ext = page_ext_next(page_ext); > > I believe there cannot be any leftovers because nr is always a power > of 2. > Is it true? Converting nr argument to order (if it's possible) will > make it obvious. Right. nr = thp_nr_pages(head), which is a power of 2. There would not be any leftover. Matthew recently converted split_page_owner to take nr instead of order.[1] But I am not sure why, since it seems to me that two call sites (__split_huge_page in mm/huge_memory.c and split_page in mm/page_alloc.c) can pass the order information. [1]https://lore.kernel.org/linux-mm/20200908195539.25896-4-willy@infradead.org/ — Best Regards, Yan Zi
On Fri, Nov 13, 2020 at 08:08:58PM -0500, Zi Yan wrote: > On 13 Nov 2020, at 19:15, Roman Gushchin wrote: > > > On Wed, Nov 11, 2020 at 03:40:05PM -0500, Zi Yan wrote: > > > From: Zi Yan <ziy@nvidia.com> > > > > > > It adds a new_order parameter to set new page order in page owner. > > > It prepares for upcoming changes to support split huge page to any > > > lower > > > order. > > > > > > Signed-off-by: Zi Yan <ziy@nvidia.com> > > > --- > > > include/linux/page_owner.h | 7 ++++--- > > > mm/huge_memory.c | 2 +- > > > mm/page_alloc.c | 2 +- > > > mm/page_owner.c | 6 +++--- > > > 4 files changed, 9 insertions(+), 8 deletions(-) > > > > > > diff --git a/include/linux/page_owner.h b/include/linux/page_owner.h > > > index 3468794f83d2..215cbb159568 100644 > > > --- a/include/linux/page_owner.h > > > +++ b/include/linux/page_owner.h > > > @@ -31,10 +31,11 @@ static inline void set_page_owner(struct page > > > *page, > > > __set_page_owner(page, order, gfp_mask); > > > } > > > > > > -static inline void split_page_owner(struct page *page, unsigned int > > > nr) > > > +static inline void split_page_owner(struct page *page, unsigned int > > > nr, > > > + unsigned int new_order) > > > { > > > if (static_branch_unlikely(&page_owner_inited)) > > > - __split_page_owner(page, nr); > > > + __split_page_owner(page, nr, new_order); > > > } > > > static inline void copy_page_owner(struct page *oldpage, struct > > > page *newpage) > > > { > > > @@ -60,7 +61,7 @@ static inline void set_page_owner(struct page > > > *page, > > > { > > > } > > > static inline void split_page_owner(struct page *page, > > > - unsigned int order) > > > + unsigned int nr, unsigned int new_order) > > > > With the addition of the new argument it's a bit hard to understand > > what the function is supposed to do. It seems like nr == > > page_order(page), > > is it right? Maybe we can pass old_order and new_order? Or just the page > > and the new order? > > Yeah, it is a bit confusing. Please see more below. > > > > > > { > > > } > > > static inline void copy_page_owner(struct page *oldpage, struct > > > page *newpage) > > > diff --git a/mm/huge_memory.c b/mm/huge_memory.c > > > index f599f5b9bf7f..8b7d771ee962 100644 > > > --- a/mm/huge_memory.c > > > +++ b/mm/huge_memory.c > > > @@ -2459,7 +2459,7 @@ static void __split_huge_page(struct page > > > *page, struct list_head *list, > > > > > > ClearPageCompound(head); > > > > > > - split_page_owner(head, nr); > > > + split_page_owner(head, nr, 1); > > > > > > /* See comment in __split_huge_page_tail() */ > > > if (PageAnon(head)) { > > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > > > index d77220615fd5..a9eead0e091a 100644 > > > --- a/mm/page_alloc.c > > > +++ b/mm/page_alloc.c > > > @@ -3284,7 +3284,7 @@ void split_page(struct page *page, unsigned > > > int order) > > > > > > for (i = 1; i < (1 << order); i++) > > > set_page_refcounted(page + i); > > > - split_page_owner(page, 1 << order); > > > + split_page_owner(page, 1 << order, 1); > > > } > > > EXPORT_SYMBOL_GPL(split_page); > > > > > > diff --git a/mm/page_owner.c b/mm/page_owner.c > > > index b735a8eafcdb..2b7f7e9056dc 100644 > > > --- a/mm/page_owner.c > > > +++ b/mm/page_owner.c > > > @@ -204,7 +204,7 @@ void __set_page_owner_migrate_reason(struct page > > > *page, int reason) > > > page_owner->last_migrate_reason = reason; > > > } > > > > > > -void __split_page_owner(struct page *page, unsigned int nr) > > > +void __split_page_owner(struct page *page, unsigned int nr, > > > unsigned int new_order) > > > { > > > int i; > > > struct page_ext *page_ext = lookup_page_ext(page); > > > @@ -213,9 +213,9 @@ void __split_page_owner(struct page *page, > > > unsigned int nr) > > > if (unlikely(!page_ext)) > > > return; > > > > > > - for (i = 0; i < nr; i++) { > > > + for (i = 0; i < nr; i += (1 << new_order)) { > > > page_owner = get_page_owner(page_ext); > > > - page_owner->order = 0; > > > + page_owner->order = new_order; > > > page_ext = page_ext_next(page_ext); > > > > I believe there cannot be any leftovers because nr is always a power of > > 2. > > Is it true? Converting nr argument to order (if it's possible) will make > > it obvious. > > Right. nr = thp_nr_pages(head), which is a power of 2. There would not be > any > leftover. > > Matthew recently converted split_page_owner to take nr instead of order.[1] > But I am not > sure why, since it seems to me that two call sites (__split_huge_page in > mm/huge_memory.c and split_page in mm/page_alloc.c) can pass the order > information. Yeah, I'm not sure why too. Maybe Matthew has some input here? You can also pass new_nr, but IMO orders look so much better here. Thanks!
On Wed, Nov 11, 2020 at 03:40:05PM -0500, Zi Yan wrote: > From: Zi Yan <ziy@nvidia.com> > > It adds a new_order parameter to set new page order in page owner. > It prepares for upcoming changes to support split huge page to any lower > order. > > Signed-off-by: Zi Yan <ziy@nvidia.com> > --- > include/linux/page_owner.h | 7 ++++--- > mm/huge_memory.c | 2 +- > mm/page_alloc.c | 2 +- > mm/page_owner.c | 6 +++--- > 4 files changed, 9 insertions(+), 8 deletions(-) > > diff --git a/include/linux/page_owner.h b/include/linux/page_owner.h > index 3468794f83d2..215cbb159568 100644 > --- a/include/linux/page_owner.h > +++ b/include/linux/page_owner.h > @@ -31,10 +31,11 @@ static inline void set_page_owner(struct page *page, > __set_page_owner(page, order, gfp_mask); > } > > -static inline void split_page_owner(struct page *page, unsigned int nr) > +static inline void split_page_owner(struct page *page, unsigned int nr, > + unsigned int new_order) > { > if (static_branch_unlikely(&page_owner_inited)) > - __split_page_owner(page, nr); > + __split_page_owner(page, nr, new_order); Hm. Where do you correct __split_page_owner() declaration. I don't see it.
On 16 Nov 2020, at 11:25, Kirill A. Shutemov wrote: > On Wed, Nov 11, 2020 at 03:40:05PM -0500, Zi Yan wrote: >> From: Zi Yan <ziy@nvidia.com> >> >> It adds a new_order parameter to set new page order in page owner. >> It prepares for upcoming changes to support split huge page to any lower >> order. >> >> Signed-off-by: Zi Yan <ziy@nvidia.com> >> --- >> include/linux/page_owner.h | 7 ++++--- >> mm/huge_memory.c | 2 +- >> mm/page_alloc.c | 2 +- >> mm/page_owner.c | 6 +++--- >> 4 files changed, 9 insertions(+), 8 deletions(-) >> >> diff --git a/include/linux/page_owner.h b/include/linux/page_owner.h >> index 3468794f83d2..215cbb159568 100644 >> --- a/include/linux/page_owner.h >> +++ b/include/linux/page_owner.h >> @@ -31,10 +31,11 @@ static inline void set_page_owner(struct page *page, >> __set_page_owner(page, order, gfp_mask); >> } >> >> -static inline void split_page_owner(struct page *page, unsigned int nr) >> +static inline void split_page_owner(struct page *page, unsigned int nr, >> + unsigned int new_order) >> { >> if (static_branch_unlikely(&page_owner_inited)) >> - __split_page_owner(page, nr); >> + __split_page_owner(page, nr, new_order); > > Hm. Where do you correct __split_page_owner() declaration. I don't see it. I missed it. Will add it. Thanks. — Best Regards, Yan Zi
On Fri, Nov 13, 2020 at 05:38:01PM -0800, Roman Gushchin wrote: > On Fri, Nov 13, 2020 at 08:08:58PM -0500, Zi Yan wrote: > > Matthew recently converted split_page_owner to take nr instead of order.[1] > > But I am not > > sure why, since it seems to me that two call sites (__split_huge_page in > > mm/huge_memory.c and split_page in mm/page_alloc.c) can pass the order > > information. > > Yeah, I'm not sure why too. Maybe Matthew has some input here? > You can also pass new_nr, but IMO orders look so much better here. If only I'd written that information in the changelog ... oh wait, I did! mm/page_owner: change split_page_owner to take a count The implementation of split_page_owner() prefers a count rather than the old order of the page. When we support a variable size THP, we won't have the order at this point, but we will have the number of pages. So change the interface to what the caller and callee would prefer.
On Wed, Nov 11, 2020 at 03:40:05PM -0500, Zi Yan wrote: > - for (i = 0; i < nr; i++) { > + for (i = 0; i < nr; i += (1 << new_order)) { > page_owner = get_page_owner(page_ext); > - page_owner->order = 0; > + page_owner->order = new_order; > page_ext = page_ext_next(page_ext); > } This doesn't do what you're hoping it will. It's going to set ->order to new_order for the first N pages instead of every 1/N pages. You'll need to do something like page_ext = lookup_page_ext(page + i); or add a new page_ext_add(page_ext, 1 << new_order);
On 17 Nov 2020, at 16:05, Matthew Wilcox wrote: > On Fri, Nov 13, 2020 at 05:38:01PM -0800, Roman Gushchin wrote: >> On Fri, Nov 13, 2020 at 08:08:58PM -0500, Zi Yan wrote: >>> Matthew recently converted split_page_owner to take nr instead of order.[1] >>> But I am not >>> sure why, since it seems to me that two call sites (__split_huge_page in >>> mm/huge_memory.c and split_page in mm/page_alloc.c) can pass the order >>> information. >> >> Yeah, I'm not sure why too. Maybe Matthew has some input here? >> You can also pass new_nr, but IMO orders look so much better here. > > If only I'd written that information in the changelog ... oh wait, I did! > > mm/page_owner: change split_page_owner to take a count > > The implementation of split_page_owner() prefers a count rather than the > old order of the page. When we support a variable size THP, we won't > have the order at this point, but we will have the number of pages. > So change the interface to what the caller and callee would prefer. There are two callers, split_page in mm/page_alloc.c and __split_huge_page in mm/huge_memory.c. The former has the page order. The latter has the page order information before __split_huge_page_tail is called, so we can do old_order = thp_order(head) instead of nr = thp_nr_page(head) and use old_order. What am I missing there? Thanks. — Best Regards, Yan Zi
On 17 Nov 2020, at 16:10, Matthew Wilcox wrote: > On Wed, Nov 11, 2020 at 03:40:05PM -0500, Zi Yan wrote: >> - for (i = 0; i < nr; i++) { >> + for (i = 0; i < nr; i += (1 << new_order)) { >> page_owner = get_page_owner(page_ext); >> - page_owner->order = 0; >> + page_owner->order = new_order; >> page_ext = page_ext_next(page_ext); >> } > > This doesn't do what you're hoping it will. It's going to set ->order to > new_order for the first N pages instead of every 1/N pages. > > You'll need to do something like > > page_ext = lookup_page_ext(page + i); Will use this. Thanks. > > or add a new page_ext_add(page_ext, 1 << new_order); — Best Regards, Yan Zi
On Tue, Nov 17, 2020 at 04:12:03PM -0500, Zi Yan wrote: > On 17 Nov 2020, at 16:05, Matthew Wilcox wrote: > > > On Fri, Nov 13, 2020 at 05:38:01PM -0800, Roman Gushchin wrote: > >> On Fri, Nov 13, 2020 at 08:08:58PM -0500, Zi Yan wrote: > >>> Matthew recently converted split_page_owner to take nr instead of order.[1] > >>> But I am not > >>> sure why, since it seems to me that two call sites (__split_huge_page in > >>> mm/huge_memory.c and split_page in mm/page_alloc.c) can pass the order > >>> information. > >> > >> Yeah, I'm not sure why too. Maybe Matthew has some input here? > >> You can also pass new_nr, but IMO orders look so much better here. > > > > If only I'd written that information in the changelog ... oh wait, I did! > > > > mm/page_owner: change split_page_owner to take a count > > > > The implementation of split_page_owner() prefers a count rather than the > > old order of the page. When we support a variable size THP, we won't > > have the order at this point, but we will have the number of pages. > > So change the interface to what the caller and callee would prefer. > > There are two callers, split_page in mm/page_alloc.c and __split_huge_page in > mm/huge_memory.c. The former has the page order. The latter has the page order > information before __split_huge_page_tail is called, so we can do > old_order = thp_order(head) instead of nr = thp_nr_page(head) and use old_order. > What am I missing there? Sure, we could also do that. But what I wrote was true at the time I wrote it.
On 17 Nov 2020, at 16:22, Matthew Wilcox wrote: > On Tue, Nov 17, 2020 at 04:12:03PM -0500, Zi Yan wrote: >> On 17 Nov 2020, at 16:05, Matthew Wilcox wrote: >> >>> On Fri, Nov 13, 2020 at 05:38:01PM -0800, Roman Gushchin wrote: >>>> On Fri, Nov 13, 2020 at 08:08:58PM -0500, Zi Yan wrote: >>>>> Matthew recently converted split_page_owner to take nr instead of order.[1] >>>>> But I am not >>>>> sure why, since it seems to me that two call sites (__split_huge_page in >>>>> mm/huge_memory.c and split_page in mm/page_alloc.c) can pass the order >>>>> information. >>>> >>>> Yeah, I'm not sure why too. Maybe Matthew has some input here? >>>> You can also pass new_nr, but IMO orders look so much better here. >>> >>> If only I'd written that information in the changelog ... oh wait, I did! >>> >>> mm/page_owner: change split_page_owner to take a count >>> >>> The implementation of split_page_owner() prefers a count rather than the >>> old order of the page. When we support a variable size THP, we won't >>> have the order at this point, but we will have the number of pages. >>> So change the interface to what the caller and callee would prefer. >> >> There are two callers, split_page in mm/page_alloc.c and __split_huge_page in >> mm/huge_memory.c. The former has the page order. The latter has the page order >> information before __split_huge_page_tail is called, so we can do >> old_order = thp_order(head) instead of nr = thp_nr_page(head) and use old_order. >> What am I missing there? > > Sure, we could also do that. But what I wrote was true at the time I > wrote it. Got it. Thanks. Will change it to use old_order to make split_page_owner parameters look more consistent. — Best Regards, Yan Zi
On Tue, Nov 17, 2020 at 09:22:55PM +0000, Matthew Wilcox wrote: > On Tue, Nov 17, 2020 at 04:12:03PM -0500, Zi Yan wrote: > > On 17 Nov 2020, at 16:05, Matthew Wilcox wrote: > > > > > On Fri, Nov 13, 2020 at 05:38:01PM -0800, Roman Gushchin wrote: > > >> On Fri, Nov 13, 2020 at 08:08:58PM -0500, Zi Yan wrote: > > >>> Matthew recently converted split_page_owner to take nr instead of order.[1] > > >>> But I am not > > >>> sure why, since it seems to me that two call sites (__split_huge_page in > > >>> mm/huge_memory.c and split_page in mm/page_alloc.c) can pass the order > > >>> information. > > >> > > >> Yeah, I'm not sure why too. Maybe Matthew has some input here? > > >> You can also pass new_nr, but IMO orders look so much better here. > > > > > > If only I'd written that information in the changelog ... oh wait, I did! > > > > > > mm/page_owner: change split_page_owner to take a count > > > > > > The implementation of split_page_owner() prefers a count rather than the > > > old order of the page. When we support a variable size THP, we won't > > > have the order at this point, but we will have the number of pages. > > > So change the interface to what the caller and callee would prefer. > > > > There are two callers, split_page in mm/page_alloc.c and __split_huge_page in > > mm/huge_memory.c. The former has the page order. The latter has the page order > > information before __split_huge_page_tail is called, so we can do > > old_order = thp_order(head) instead of nr = thp_nr_page(head) and use old_order. > > What am I missing there? > > Sure, we could also do that. But what I wrote was true at the time I > wrote it. Sure, I was asking about if you're ok with going back to orders or there are better ideas. I'm sorry if it wasn't clear and sounded differently. It just seems to me than a function is taking nr and order (as in Zi's last version), I'd expect that it's a number of pages of given order, or something like this. So I'd avoid mixing them. Orders are slightly better if nr is always a power of two, it's just more obvious from looking at the code. Thanks!
On Tue, Nov 17, 2020 at 01:35:37PM -0800, Roman Gushchin wrote: > On Tue, Nov 17, 2020 at 09:22:55PM +0000, Matthew Wilcox wrote: > > On Tue, Nov 17, 2020 at 04:12:03PM -0500, Zi Yan wrote: > > > On 17 Nov 2020, at 16:05, Matthew Wilcox wrote: > > > > > > > On Fri, Nov 13, 2020 at 05:38:01PM -0800, Roman Gushchin wrote: > > > >> On Fri, Nov 13, 2020 at 08:08:58PM -0500, Zi Yan wrote: > > > >>> Matthew recently converted split_page_owner to take nr instead of order.[1] > > > >>> But I am not > > > >>> sure why, since it seems to me that two call sites (__split_huge_page in > > > >>> mm/huge_memory.c and split_page in mm/page_alloc.c) can pass the order > > > >>> information. > > > >> > > > >> Yeah, I'm not sure why too. Maybe Matthew has some input here? > > > >> You can also pass new_nr, but IMO orders look so much better here. > > > > > > > > If only I'd written that information in the changelog ... oh wait, I did! > > > > > > > > mm/page_owner: change split_page_owner to take a count > > > > > > > > The implementation of split_page_owner() prefers a count rather than the > > > > old order of the page. When we support a variable size THP, we won't > > > > have the order at this point, but we will have the number of pages. > > > > So change the interface to what the caller and callee would prefer. > > > > > > There are two callers, split_page in mm/page_alloc.c and __split_huge_page in > > > mm/huge_memory.c. The former has the page order. The latter has the page order > > > information before __split_huge_page_tail is called, so we can do > > > old_order = thp_order(head) instead of nr = thp_nr_page(head) and use old_order. > > > What am I missing there? > > > > Sure, we could also do that. But what I wrote was true at the time I > > wrote it. > > Sure, I was asking about if you're ok with going back to orders or there are better > ideas. I'm sorry if it wasn't clear and sounded differently. > > It just seems to me than a function is taking nr and order (as in Zi's last version), > I'd expect that it's a number of pages of given order, or something like this. > So I'd avoid mixing them. Orders are slightly better if nr is always a power of two, > it's just more obvious from looking at the code. I think it's awkward no matter which way round we do it. If we pass old_order, new_order then we create extra work for both caller and callee. If we pass old_nr, new_order, it looks weird for humans. At the end of the day, I'm not that invested in which we do.
diff --git a/include/linux/page_owner.h b/include/linux/page_owner.h index 3468794f83d2..215cbb159568 100644 --- a/include/linux/page_owner.h +++ b/include/linux/page_owner.h @@ -31,10 +31,11 @@ static inline void set_page_owner(struct page *page, __set_page_owner(page, order, gfp_mask); } -static inline void split_page_owner(struct page *page, unsigned int nr) +static inline void split_page_owner(struct page *page, unsigned int nr, + unsigned int new_order) { if (static_branch_unlikely(&page_owner_inited)) - __split_page_owner(page, nr); + __split_page_owner(page, nr, new_order); } static inline void copy_page_owner(struct page *oldpage, struct page *newpage) { @@ -60,7 +61,7 @@ static inline void set_page_owner(struct page *page, { } static inline void split_page_owner(struct page *page, - unsigned int order) + unsigned int nr, unsigned int new_order) { } static inline void copy_page_owner(struct page *oldpage, struct page *newpage) diff --git a/mm/huge_memory.c b/mm/huge_memory.c index f599f5b9bf7f..8b7d771ee962 100644 --- a/mm/huge_memory.c +++ b/mm/huge_memory.c @@ -2459,7 +2459,7 @@ static void __split_huge_page(struct page *page, struct list_head *list, ClearPageCompound(head); - split_page_owner(head, nr); + split_page_owner(head, nr, 1); /* See comment in __split_huge_page_tail() */ if (PageAnon(head)) { diff --git a/mm/page_alloc.c b/mm/page_alloc.c index d77220615fd5..a9eead0e091a 100644 --- a/mm/page_alloc.c +++ b/mm/page_alloc.c @@ -3284,7 +3284,7 @@ void split_page(struct page *page, unsigned int order) for (i = 1; i < (1 << order); i++) set_page_refcounted(page + i); - split_page_owner(page, 1 << order); + split_page_owner(page, 1 << order, 1); } EXPORT_SYMBOL_GPL(split_page); diff --git a/mm/page_owner.c b/mm/page_owner.c index b735a8eafcdb..2b7f7e9056dc 100644 --- a/mm/page_owner.c +++ b/mm/page_owner.c @@ -204,7 +204,7 @@ void __set_page_owner_migrate_reason(struct page *page, int reason) page_owner->last_migrate_reason = reason; } -void __split_page_owner(struct page *page, unsigned int nr) +void __split_page_owner(struct page *page, unsigned int nr, unsigned int new_order) { int i; struct page_ext *page_ext = lookup_page_ext(page); @@ -213,9 +213,9 @@ void __split_page_owner(struct page *page, unsigned int nr) if (unlikely(!page_ext)) return; - for (i = 0; i < nr; i++) { + for (i = 0; i < nr; i += (1 << new_order)) { page_owner = get_page_owner(page_ext); - page_owner->order = 0; + page_owner->order = new_order; page_ext = page_ext_next(page_ext); } }