| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <7b8a62f1-3e81-4012-8b44-219498f37152@lucifer.local>
Date: Tue, 1 Jul 2025 10:01:06 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: David Hildenbrand <david@...hat.com>
Cc: linux-kernel@...r.kernel.org, linux-mm@...ck.org,
linux-doc@...r.kernel.org, linuxppc-dev@...ts.ozlabs.org,
virtualization@...ts.linux.dev, linux-fsdevel@...r.kernel.org,
Andrew Morton <akpm@...ux-foundation.org>,
Jonathan Corbet <corbet@....net>,
Madhavan Srinivasan <maddy@...ux.ibm.com>,
Michael Ellerman <mpe@...erman.id.au>,
Nicholas Piggin <npiggin@...il.com>,
Christophe Leroy <christophe.leroy@...roup.eu>,
Jerrin Shaji George <jerrin.shaji-george@...adcom.com>,
Arnd Bergmann <arnd@...db.de>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
"Michael S. Tsirkin" <mst@...hat.com>,
Jason Wang <jasowang@...hat.com>,
Xuan Zhuo <xuanzhuo@...ux.alibaba.com>,
Eugenio Pérez <eperezma@...hat.com>,
Alexander Viro <viro@...iv.linux.org.uk>,
Christian Brauner <brauner@...nel.org>, Jan Kara <jack@...e.cz>,
Zi Yan <ziy@...dia.com>, Matthew Brost <matthew.brost@...el.com>,
Joshua Hahn <joshua.hahnjy@...il.com>, Rakie Kim <rakie.kim@...com>,
Byungchul Park <byungchul@...com>, Gregory Price <gourry@...rry.net>,
Ying Huang <ying.huang@...ux.alibaba.com>,
Alistair Popple <apopple@...dia.com>,
"Liam R. Howlett" <Liam.Howlett@...cle.com>,
Vlastimil Babka <vbabka@...e.cz>, Mike Rapoport <rppt@...nel.org>,
Suren Baghdasaryan <surenb@...gle.com>, Michal Hocko <mhocko@...e.com>,
"Matthew Wilcox (Oracle)" <willy@...radead.org>,
Minchan Kim <minchan@...nel.org>,
Sergey Senozhatsky <senozhatsky@...omium.org>,
Brendan Jackman <jackmanb@...gle.com>,
Johannes Weiner <hannes@...xchg.org>, Jason Gunthorpe <jgg@...pe.ca>,
John Hubbard <jhubbard@...dia.com>, Peter Xu <peterx@...hat.com>,
Xu Xin <xu.xin16@....com.cn>,
Chengming Zhou <chengming.zhou@...ux.dev>,
Miaohe Lin <linmiaohe@...wei.com>,
Naoya Horiguchi <nao.horiguchi@...il.com>,
Oscar Salvador <osalvador@...e.de>, Rik van Riel <riel@...riel.com>,
Harry Yoo <harry.yoo@...cle.com>,
Qi Zheng <zhengqi.arch@...edance.com>,
Shakeel Butt <shakeel.butt@...ux.dev>
Subject: Re: [PATCH v1 02/29] mm/balloon_compaction: convert
balloon_page_delete() to balloon_page_finalize()
On Tue, Jul 01, 2025 at 09:58:09AM +0200, David Hildenbrand wrote:
> On 30.06.25 17:15, Lorenzo Stoakes wrote:
> > 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 <david@...hat.com>
Based on below this LGTM, so:
Reviewed-by: Lorenzo Stoakes <lorenzo.stoakes@...cle.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);
> >
>
> Hi Lorenzo,
>
> as always, thanks for the detailed review!
You're welcome :>)
>
> > We seem to just be removing this and not replacing with finalize, is this right?
>
> See below.
>
> >
> > > 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);
>
> ^ here it is, next to the put_page() just like for the other cases.
OK so it's just moved to a different place for consistency.
>
> Or did you mean something else?
No this is what I meant :)
>
> > > /* 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);
> > >
>
>
> [...]
>
> > > -/*
> > > - * 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_delete() was used on two paths
>
> 1) Removing a page from the balloon for deflation through
> balloon_page_list_dequeue()
>
> 2) Removing an isolated page from the balloon for migration in the
> per-driver migration handlers. Isolated pages were already removed from the
> balloon list during ... isolation.
>
> With this change, 1) does the list_del(&page->lru) manually and 2) only
> calls balloon_page_finalize().
>
> During 1) the same reasoning as in 1/xx applies: isolated pages cannot be in
> the balloon list.
Right yeah this is what I thought, thanks!
>
> >
> > > -}
> > > -
> > > /*
> > > * 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?
>
> We could, but I'm planning on removing the page lock next (see patch
> description), so not too keen to create more code around that.
>
> Maybe mention that the balloon lock should not be held?
>
> Not a limitation. It could be called with it, just not a requirement today.
>
> I suspect that once we remove the page lock, that we might use the balloon
> lock and rework balloon_page_migrate() to take the lock. TBD.
OK fair enough!
>
> > >> + */
> > > +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?
>
> Sure. balloon_page_list_dequeue() is called from balloon_page_dequeue(),
> which resides outside the CONFIG_BALLOON_COMPACTION ifdef in
> mm/balloon_compaction.c.
>
> At some point (not in this series) we should probably rename
>
> balloon_compaction.c -> balloon.c
>
> To match CONFIG_MEMORY_BALLOON.
>
> Because the compaction part is just one extra bit in there. (an important
> one, but still, you can use the balloon infrastructure without
> compaction/page migration)
Yeah this would be nice!
Powered by blists - more mailing lists