| 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: <c7e82a13-aa93-4eb3-8679-29cd32692bd0@redhat.com>
Date: Fri, 11 Jul 2025 08:41:46 +0200
From: David Hildenbrand <david@...hat.com>
To: Zi Yan <ziy@...dia.com>, Balbir Singh <balbirs@...dia.com>,
linux-mm@...ck.org
Cc: Andrew Morton <akpm@...ux-foundation.org>,
Lorenzo Stoakes <lorenzo.stoakes@...cle.com>,
Baolin Wang <baolin.wang@...ux.alibaba.com>,
"Liam R. Howlett" <Liam.Howlett@...cle.com>, Nico Pache <npache@...hat.com>,
Ryan Roberts <ryan.roberts@....com>, Dev Jain <dev.jain@....com>,
Barry Song <baohua@...nel.org>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] mm/huge_memory: move unrelated code out of
__split_unmapped_folio()
On 11.07.25 05:02, Zi Yan wrote:
> remap(), folio_ref_unfreeze(), lru_add_split_folio() are not related to
> splitting unmapped folio operations. Move them out to the caller, so that
> __split_unmapped_folio() only splits unmapped folios. This makes
> __split_unmapped_folio() reusable.
>
> Convert VM_BUG_ON(mapping) to use VM_WARN_ON_ONCE_FOLIO().
>
> Signed-off-by: Zi Yan <ziy@...dia.com>
> ---
> Based on the prior discussion[1], this patch makes
> __split_unmapped_folio() reusable for splitting unmapped folios without
> adding a new boolean unmapped parameter to guard mapping related code.
>
> Another potential benefit is that __split_unmapped_folio() could be
> called on after-split folios by __folio_split() to perform new split
> methods. For example, at deferred split time, unmapped subpages can
> scatter arbitrarily within a large folio, neither uniform nor non-uniform
> split can maximize after-split folio orders for mapped subpages.
> Hopefully, performing __split_unmapped_folio() multiple times can
> achieve the optimal split result.
>
> It passed mm selftests.
>
> [1] https://lore.kernel.org/linux-mm/94D8C1A4-780C-4BEC-A336-7D3613B54845@nvidia.com/
> ---
>
> mm/huge_memory.c | 275 ++++++++++++++++++++++++-----------------------
> 1 file changed, 139 insertions(+), 136 deletions(-)
>
> diff --git a/mm/huge_memory.c b/mm/huge_memory.c
> index 3eb1c34be601..d97145dfa6c8 100644
> --- a/mm/huge_memory.c
> +++ b/mm/huge_memory.c
> @@ -3396,10 +3396,6 @@ static void __split_folio_to_order(struct folio *folio, int old_order,
> * order - 1 to new_order).
> * @split_at: in buddy allocator like split, the folio containing @split_at
> * will be split until its order becomes @new_order.
> - * @lock_at: the folio containing @lock_at is left locked for caller.
> - * @list: the after split folios will be added to @list if it is not NULL,
> - * otherwise to LRU lists.
> - * @end: the end of the file @folio maps to. -1 if @folio is anonymous memory.
> * @xas: xa_state pointing to folio->mapping->i_pages and locked by caller
> * @mapping: @folio->mapping
> * @uniform_split: if the split is uniform or not (buddy allocator like split)
> @@ -3425,51 +3421,27 @@ static void __split_folio_to_order(struct folio *folio, int old_order,
> * @page, which is split in next for loop.
> *
> * After splitting, the caller's folio reference will be transferred to the
> - * folio containing @page. The other folios may be freed if they are not mapped.
> - *
> - * In terms of locking, after splitting,
> - * 1. uniform split leaves @page (or the folio contains it) locked;
> - * 2. buddy allocator like (non-uniform) split leaves @folio locked.
> - *
> + * folio containing @page. The caller needs to unlock and/or free after-split
> + * folios if necessary.
> *
> * For !uniform_split, when -ENOMEM is returned, the original folio might be
> * split. The caller needs to check the input folio.
> */
> static int __split_unmapped_folio(struct folio *folio, int new_order,
> - struct page *split_at, struct page *lock_at,
> - struct list_head *list, pgoff_t end,
> - struct xa_state *xas, struct address_space *mapping,
> - bool uniform_split)
> + struct page *split_at, struct xa_state *xas,
> + struct address_space *mapping,
> + bool uniform_split)
Use two-tabs indent please (like we already do, I assume).
[...]
> @@ -3706,11 +3599,14 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
> {
> struct deferred_split *ds_queue = get_deferred_split_queue(folio);
> XA_STATE(xas, &folio->mapping->i_pages, folio->index);
> + struct folio *next_folio = folio_next(folio);
> bool is_anon = folio_test_anon(folio);
> struct address_space *mapping = NULL;
> struct anon_vma *anon_vma = NULL;
> int order = folio_order(folio);
> + struct folio *new_folio, *next;
> int extra_pins, ret;
> + int nr_shmem_dropped = 0;
> pgoff_t end;
> bool is_hzp;
>
> @@ -3833,13 +3729,18 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
> */
> xas_lock(&xas);
> xas_reset(&xas);
> - if (xas_load(&xas) != folio)
> + if (xas_load(&xas) != folio) {
> + ret = -EAGAIN;
> goto fail;
> + }
> }
>
> /* Prevent deferred_split_scan() touching ->_refcount */
> spin_lock(&ds_queue->split_queue_lock);
> if (folio_ref_freeze(folio, 1 + extra_pins)) {
> + struct address_space *swap_cache = NULL;
> + struct lruvec *lruvec;
> +
> if (folio_order(folio) > 1 &&
> !list_empty(&folio->_deferred_list)) {
> ds_queue->split_queue_len--;
> @@ -3873,18 +3774,120 @@ static int __folio_split(struct folio *folio, unsigned int new_order,
> }
> }
>
> - ret = __split_unmapped_folio(folio, new_order,
> - split_at, lock_at, list, end, &xas, mapping,
> - uniform_split);
> + if (folio_test_swapcache(folio)) {
> + if (mapping) {
> + VM_WARN_ON_ONCE_FOLIO(mapping, folio);
> + ret = -EINVAL;
> + goto fail;
> + }
> +
> + /*
> + * a swapcache folio can only be uniformly split to
> + * order-0
> + */
> + if (!uniform_split || new_order != 0) {
> + ret = -EINVAL;
> + goto fail;
> + }
> +
> + swap_cache = swap_address_space(folio->swap);
> + xa_lock(&swap_cache->i_pages);
> + }
> +
> + /* lock lru list/PageCompound, ref frozen by page_ref_freeze */
> + lruvec = folio_lruvec_lock(folio);
> +
> + ret = __split_unmapped_folio(folio, new_order, split_at, &xas,
> + mapping, uniform_split);
> +
> + /*
> + * Unfreeze after-split folios and put them back to the right
> + * list. @folio should be kept frozon until page cache entries
> + * are updated with all the other after-split folios to prevent
> + * others seeing stale page cache entries.
> + */
> + for (new_folio = folio_next(folio); new_folio != next_folio;
> + new_folio = next) {
> + next = folio_next(new_folio);
> +
> + folio_ref_unfreeze(
> + new_folio,
> + 1 + ((mapping || swap_cache) ?
> + folio_nr_pages(new_folio) :
> + 0));
While we are at it, is a way to make this look less than an artistic
masterpiece? :)
expected_refs = ...
folio_ref_unfreeze(new_folio, expected_refs).
Can we already make use of folio_expected_ref_count() at that point?
Mapcount should be 0 and the folio should be properly setup (e.g., anon,
swapcache) IIRC.
So maybe
expected_refs = folio_expected_ref_count(new_folio) + 1;
folio_ref_unfreeze(new_folio, expected_refs).
Would do?
> +
> + lru_add_split_folio(folio, new_folio, lruvec, list);
> +
> + /* Some pages can be beyond EOF: drop them from cache */
> + if (new_folio->index >= end) {
> + if (shmem_mapping(mapping))
> + nr_shmem_dropped +=
> + folio_nr_pages(new_folio);
Keep that on a single line.
> + else if (folio_test_clear_dirty(new_folio))
> + folio_account_cleaned(
> + new_folio,
> + inode_to_wb(mapping->host));
> + __filemap_remove_folio(new_folio, NULL);
> + folio_put_refs(new_folio,
> + folio_nr_pages(new_folio));
> + } else if (mapping) {
> + __xa_store(&mapping->i_pages, new_folio->index,
> + new_folio, 0);
> + } else if (swap_cache) {
> + __xa_store(&swap_cache->i_pages,
> + swap_cache_index(new_folio->swap),
> + new_folio, 0);
> + }
> + }
> + /*
> + * Unfreeze @folio only after all page cache entries, which
> + * used to point to it, have been updated with new folios.
> + * Otherwise, a parallel folio_try_get() can grab origin_folio
> + * and its caller can see stale page cache entries.
> + */
> + folio_ref_unfreeze(folio, 1 +
> + ((mapping || swap_cache) ? folio_nr_pages(folio) : 0));
Same as above probably.
--
Cheers,
David / dhildenb
Powered by blists - more mailing lists