| Message ID | 20240326063036.6242-4-osalvador@suse.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | page_owner: Fix refcount imbalance | expand |
On 3/26/24 7:30 AM, Oscar Salvador wrote: > Upon migration, new allocated pages are being given the handle of the old > pages. This is problematic because it means that for the stack which > allocated the old page, we will be substracting the old page + the new one > when that page is freed, creating an accounting imbalance. > > There is an interest in keeping it that way, as otherwise the output will > biased towards migration stacks should those operations occur often, but > that is not really helpful. > The link from the new page to the old stack is being performed by calling > __update_page_owner_handle() in __folio_copy_owner(). > The only thing that is left is to link the migrate stack to the old > page, so the old page will be subtracted from the migrate stack, > avoiding by doing so any possible imbalance. > > Fixes: 217b2119b9e2 ("mm,page_owner: implement the tracking of the stacks count") > Signed-off-by: Oscar Salvador <osalvador@suse.de> > --- > mm/page_owner.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/mm/page_owner.c b/mm/page_owner.c > index 5df0d6892bdc..b4476f45b376 100644 > --- a/mm/page_owner.c > +++ b/mm/page_owner.c > @@ -366,9 +366,12 @@ void __split_page_owner(struct page *page, int old_order, int new_order) > > void __folio_copy_owner(struct folio *newfolio, struct folio *old) > { > + int i; > struct page_ext *old_ext; > struct page_ext *new_ext; > struct page_owner *old_page_owner; > + struct page_owner *new_page_owner; > + depot_stack_handle_t migrate_handle; > > old_ext = page_ext_get(&old->page); > if (unlikely(!old_ext)) > @@ -381,6 +384,8 @@ void __folio_copy_owner(struct folio *newfolio, struct folio *old) > } > > old_page_owner = get_page_owner(old_ext); > + new_page_owner = get_page_owner(new_ext); > + migrate_handle = new_page_owner->handle; > __update_page_owner_handle(new_ext, old_page_owner->handle, > old_page_owner->order, old_page_owner->gfp_mask, > old_page_owner->last_migrate_reason, > @@ -395,6 +400,16 @@ void __folio_copy_owner(struct folio *newfolio, struct folio *old) > old_page_owner->free_pid, > old_page_owner->free_tgid, > old_page_owner->free_ts_nsec); > + /* > + * We linked the original stack to the new folio, we need to do the same > + * for the new one and the old folio otherwise there will be an imbalance > + * when subtracting those pages from the stack. > + */ > + for (i = 0; i < (1 << new_page_owner->order); i++) { > + old_page_owner->handle = migrate_handle; > + old_ext = page_ext_next(old_ext); > + old_page_owner = get_page_owner(old_ext); > + } Can the migration still fail after __folio_copy_owner()? That goes again to the comment you changed in patch 1/3. If it can, this will kinda create a mess with the old folio's handles not reflecting reality? (although refcounts will be ok) So if that case is possible, could we instead just dec_stack_record_count() for the handle that allocated the new folio (IIUC "migrate_handle" here) and inc_stack_record_count() for the original handle that we duplicated from the old to new. Then if either old is freed (successful migration) or new is freed (failed migration), we'll have the correct refcounts. > > page_ext_put(new_ext); > page_ext_put(old_ext);
On Tue, Apr 02, 2024 at 12:26:51PM +0200, Vlastimil Babka wrote: > Can the migration still fail after __folio_copy_owner()? That goes again to > the comment you changed in patch 1/3. If it can, this will kinda create a > mess with the old folio's handles not reflecting reality? (although > refcounts will be ok) According to my research (I replied in patch#1), no, migration cannot fail after __folio_copy_owner(), so we are safe here (Tm).
On 4/2/24 1:22 PM, Oscar Salvador wrote: > On Tue, Apr 02, 2024 at 12:26:51PM +0200, Vlastimil Babka wrote: >> Can the migration still fail after __folio_copy_owner()? That goes again to >> the comment you changed in patch 1/3. If it can, this will kinda create a >> mess with the old folio's handles not reflecting reality? (although >> refcounts will be ok) > > According to my research (I replied in patch#1), no, migration cannot > fail after __folio_copy_owner(), so we are safe here (Tm). OK then, Reviewed-by: Vlastimil Babka <vbabka@suse.cz>
diff --git a/mm/page_owner.c b/mm/page_owner.c index 5df0d6892bdc..b4476f45b376 100644 --- a/mm/page_owner.c +++ b/mm/page_owner.c @@ -366,9 +366,12 @@ void __split_page_owner(struct page *page, int old_order, int new_order) void __folio_copy_owner(struct folio *newfolio, struct folio *old) { + int i; struct page_ext *old_ext; struct page_ext *new_ext; struct page_owner *old_page_owner; + struct page_owner *new_page_owner; + depot_stack_handle_t migrate_handle; old_ext = page_ext_get(&old->page); if (unlikely(!old_ext)) @@ -381,6 +384,8 @@ void __folio_copy_owner(struct folio *newfolio, struct folio *old) } old_page_owner = get_page_owner(old_ext); + new_page_owner = get_page_owner(new_ext); + migrate_handle = new_page_owner->handle; __update_page_owner_handle(new_ext, old_page_owner->handle, old_page_owner->order, old_page_owner->gfp_mask, old_page_owner->last_migrate_reason, @@ -395,6 +400,16 @@ void __folio_copy_owner(struct folio *newfolio, struct folio *old) old_page_owner->free_pid, old_page_owner->free_tgid, old_page_owner->free_ts_nsec); + /* + * We linked the original stack to the new folio, we need to do the same + * for the new one and the old folio otherwise there will be an imbalance + * when subtracting those pages from the stack. + */ + for (i = 0; i < (1 << new_page_owner->order); i++) { + old_page_owner->handle = migrate_handle; + old_ext = page_ext_next(old_ext); + old_page_owner = get_page_owner(old_ext); + } page_ext_put(new_ext); page_ext_put(old_ext);
Upon migration, new allocated pages are being given the handle of the old pages. This is problematic because it means that for the stack which allocated the old page, we will be substracting the old page + the new one when that page is freed, creating an accounting imbalance. There is an interest in keeping it that way, as otherwise the output will biased towards migration stacks should those operations occur often, but that is not really helpful. The link from the new page to the old stack is being performed by calling __update_page_owner_handle() in __folio_copy_owner(). The only thing that is left is to link the migrate stack to the old page, so the old page will be subtracted from the migrate stack, avoiding by doing so any possible imbalance. Fixes: 217b2119b9e2 ("mm,page_owner: implement the tracking of the stacks count") Signed-off-by: Oscar Salvador <osalvador@suse.de> --- mm/page_owner.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)