| Message ID | 20230715032802.2508163-1-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/memcg: use get_page() for device private pages in mc_handle_swap_pte() | expand |
On Sat, Jul 15, 2023 at 11:28:02AM +0800, Miaohe Lin wrote: > When page table locked is held, the page can't be freed from under us. But the page isn't mapped into the page table ... there's a swap entry in the page table, so I don't think your logic holds. > So use get_page() to get the extra page reference to simplify the code. > No functional change intended. > > Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> > --- > mm/memcontrol.c | 5 +++-- > 1 file changed, 3 insertions(+), 2 deletions(-) > > diff --git a/mm/memcontrol.c b/mm/memcontrol.c > index 93e3cc581b51..4ca382efb1ca 100644 > --- a/mm/memcontrol.c > +++ b/mm/memcontrol.c > @@ -5670,8 +5670,9 @@ static struct page *mc_handle_swap_pte(struct vm_area_struct *vma, > */ > if (is_device_private_entry(ent)) { > page = pfn_swap_entry_to_page(ent); > - if (!get_page_unless_zero(page)) > - return NULL; > + /* Get a page reference while we know the page can't be freed. */ > + get_page(page); > + > return page; > } > > -- > 2.33.0 > >
On 2023/7/15 11:56, Matthew Wilcox wrote: > On Sat, Jul 15, 2023 at 11:28:02AM +0800, Miaohe Lin wrote: >> When page table locked is held, the page can't be freed from under us. > > But the page isn't mapped into the page table ... there's a swap entry > in the page table, so I don't think your logic holds. > IIUC, device_private_entry will hold one page refcnt when it's set to page table. And there's similar code in do_swap_page(): vm_fault_t do_swap_page(struct vm_fault *vmf) if (unlikely(non_swap_entry(entry))) { if (is_device_private_entry(entry)) /* * Get a page reference while we know the page can't be * freed. */ get_page(vmf->page); pte_unmap_unlock(vmf->pte, vmf->ptl); ret = vmf->page->pgmap->ops->migrate_to_ram(vmf); put_page(vmf->page); ... If my logic doesn't hold, do_swap_page() will need to fix the code. Or am I miss something? Thanks Matthew.
On 2023/7/17 10:28, Miaohe Lin wrote: > On 2023/7/15 11:56, Matthew Wilcox wrote: >> On Sat, Jul 15, 2023 at 11:28:02AM +0800, Miaohe Lin wrote: >>> When page table locked is held, the page can't be freed from under us. >> >> But the page isn't mapped into the page table ... there's a swap entry >> in the page table, so I don't think your logic holds. >> > > IIUC, device_private_entry will hold one page refcnt when it's set to page table. Take remove_migration_pte() as example, it will hold extra one page refcnt when set device private entry: remove_migration_pte() ... folio_get(folio); ... if (unlikely(is_device_private_page(new))) { make_[writable|readable]_device_private_entry(); } ... set_pte_at > And there's similar code in do_swap_page(): > > vm_fault_t do_swap_page(struct vm_fault *vmf) > if (unlikely(non_swap_entry(entry))) { > if (is_device_private_entry(entry)) > /* > * Get a page reference while we know the page can't be > * freed. > */ > get_page(vmf->page); > pte_unmap_unlock(vmf->pte, vmf->ptl); > ret = vmf->page->pgmap->ops->migrate_to_ram(vmf); > put_page(vmf->page); > ... > > If my logic doesn't hold, do_swap_page() will need to fix the code. Or am I miss something? Can I have your opinion? Thanks.
diff --git a/mm/memcontrol.c b/mm/memcontrol.c index 93e3cc581b51..4ca382efb1ca 100644 --- a/mm/memcontrol.c +++ b/mm/memcontrol.c @@ -5670,8 +5670,9 @@ static struct page *mc_handle_swap_pte(struct vm_area_struct *vma, */ if (is_device_private_entry(ent)) { page = pfn_swap_entry_to_page(ent); - if (!get_page_unless_zero(page)) - return NULL; + /* Get a page reference while we know the page can't be freed. */ + get_page(page); + return page; }
When page table locked is held, the page can't be freed from under us. So use get_page() to get the extra page reference to simplify the code. No functional change intended. Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- mm/memcontrol.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)