On Mon, Jun 30, 2025 at 02:59:43PM +0200, David Hildenbrand wrote: > Let's move the removal of the page from the balloon list into the single > caller, to remove the dependency on the PG_isolated flag and clarify > locking requirements. > > We'll shuffle the operations a bit such that they logically make more sense > (e.g., remove from the list before clearing flags). > > In balloon migration functions we can now move the balloon_page_finalize() > out of the balloon lock and perform the finalization just before dropping > the balloon reference. > > Document that the page lock is currently required when modifying the > movability aspects of a page; hopefully we can soon decouple this from the > page lock. > > Signed-off-by: David Hildenbrand <da...@redhat.com> > --- > arch/powerpc/platforms/pseries/cmm.c | 2 +- > drivers/misc/vmw_balloon.c | 3 +- > drivers/virtio/virtio_balloon.c | 4 +-- > include/linux/balloon_compaction.h | 43 +++++++++++----------------- > mm/balloon_compaction.c | 3 +- > 5 files changed, 21 insertions(+), 34 deletions(-) > > diff --git a/arch/powerpc/platforms/pseries/cmm.c > b/arch/powerpc/platforms/pseries/cmm.c > index 5f4037c1d7fe8..5e0a718d1be7b 100644 > --- a/arch/powerpc/platforms/pseries/cmm.c > +++ b/arch/powerpc/platforms/pseries/cmm.c > @@ -532,7 +532,6 @@ static int cmm_migratepage(struct balloon_dev_info > *b_dev_info, > > spin_lock_irqsave(&b_dev_info->pages_lock, flags); > balloon_page_insert(b_dev_info, newpage); > - balloon_page_delete(page);
We seem to just be removing this and not replacing with finalize, is this right? > b_dev_info->isolated_pages--; > spin_unlock_irqrestore(&b_dev_info->pages_lock, flags); > > @@ -542,6 +541,7 @@ static int cmm_migratepage(struct balloon_dev_info > *b_dev_info, > */ > plpar_page_set_active(page); > > + balloon_page_finalize(page); > /* balloon page list reference */ > put_page(page); > > diff --git a/drivers/misc/vmw_balloon.c b/drivers/misc/vmw_balloon.c > index c817d8c216413..6653fc53c951c 100644 > --- a/drivers/misc/vmw_balloon.c > +++ b/drivers/misc/vmw_balloon.c > @@ -1778,8 +1778,7 @@ static int vmballoon_migratepage(struct > balloon_dev_info *b_dev_info, > * @pages_lock . We keep holding @comm_lock since we will need it in a > * second. > */ > - balloon_page_delete(page); > - > + balloon_page_finalize(page); > put_page(page); > > /* Inflate */ > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c > index 89da052f4f687..e299e18346a30 100644 > --- a/drivers/virtio/virtio_balloon.c > +++ b/drivers/virtio/virtio_balloon.c > @@ -866,15 +866,13 @@ static int virtballoon_migratepage(struct > balloon_dev_info *vb_dev_info, > tell_host(vb, vb->inflate_vq); > > /* balloon's page migration 2nd step -- deflate "page" */ > - spin_lock_irqsave(&vb_dev_info->pages_lock, flags); > - balloon_page_delete(page); > - spin_unlock_irqrestore(&vb_dev_info->pages_lock, flags); > vb->num_pfns = VIRTIO_BALLOON_PAGES_PER_PAGE; > set_page_pfns(vb, vb->pfns, page); > tell_host(vb, vb->deflate_vq); > > mutex_unlock(&vb->balloon_lock); > > + balloon_page_finalize(page); > put_page(page); /* balloon reference */ > > return MIGRATEPAGE_SUCCESS; > diff --git a/include/linux/balloon_compaction.h > b/include/linux/balloon_compaction.h > index 5ca2d56996201..b9f19da37b089 100644 > --- a/include/linux/balloon_compaction.h > +++ b/include/linux/balloon_compaction.h > @@ -97,27 +97,6 @@ static inline void balloon_page_insert(struct > balloon_dev_info *balloon, > list_add(&page->lru, &balloon->pages); > } > > -/* > - * balloon_page_delete - delete a page from balloon's page list and clear > - * the page->private assignement accordingly. > - * @page : page to be released from balloon's page list > - * > - * Caller must ensure the page is locked and the spin_lock protecting balloon > - * pages list is held before deleting a page from the balloon device. > - */ > -static inline void balloon_page_delete(struct page *page) > -{ > - __ClearPageOffline(page); > - __ClearPageMovable(page); > - set_page_private(page, 0); > - /* > - * No touch page.lru field once @page has been isolated > - * because VM is using the field. > - */ > - if (!PageIsolated(page)) > - list_del(&page->lru); I don't see this check elsewhere, is it because, as per the 1/xx of this series, because by the time we do the finalize > -} > - > /* > * balloon_page_device - get the b_dev_info descriptor for the balloon device > * that enqueues the given page. > @@ -141,12 +120,6 @@ static inline void balloon_page_insert(struct > balloon_dev_info *balloon, > list_add(&page->lru, &balloon->pages); > } > > -static inline void balloon_page_delete(struct page *page) > -{ > - __ClearPageOffline(page); > - list_del(&page->lru); > -} > - > static inline gfp_t balloon_mapping_gfp_mask(void) > { > return GFP_HIGHUSER; > @@ -154,6 +127,22 @@ static inline gfp_t balloon_mapping_gfp_mask(void) > > #endif /* CONFIG_BALLOON_COMPACTION */ > > +/* > + * balloon_page_finalize - prepare a balloon page that was removed from the > + * balloon list for release to the page allocator > + * @page: page to be released to the page allocator > + * > + * Caller must ensure that the page is locked. Can we assert this? Maybe mention that the balloon lock should not be held? > + */ > +static inline void balloon_page_finalize(struct page *page) > +{ > + if (IS_ENABLED(CONFIG_BALLOON_COMPACTION)) { > + __ClearPageMovable(page); > + set_page_private(page, 0); > + } Why do we check this? Is this function called from anywhere where that config won't be set? > + __ClearPageOffline(page); > +} > + > /* > * balloon_page_push - insert a page into a page list. > * @head : pointer to list > diff --git a/mm/balloon_compaction.c b/mm/balloon_compaction.c > index fcb60233aa35d..ec176bdb8a78b 100644 > --- a/mm/balloon_compaction.c > +++ b/mm/balloon_compaction.c > @@ -94,7 +94,8 @@ size_t balloon_page_list_dequeue(struct balloon_dev_info > *b_dev_info, > if (!trylock_page(page)) > continue; > > - balloon_page_delete(page); > + list_del(&page->lru); > + balloon_page_finalize(page); > __count_vm_event(BALLOON_DEFLATE); > list_add(&page->lru, pages); > unlock_page(page); > -- > 2.49.0 >