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@xxxxxxxxxx> > > > > > > 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@xxxxxxxxxx> > > > --- > > > 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!