| 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: <B9C05B4D-2777-45DC-A229-B4E119B0E9A3@nvidia.com>
Date: Fri, 11 Jul 2025 11:40:13 -0400
From: Zi Yan <ziy@...dia.com>
To: David Hildenbrand <david@...hat.com>
Cc: Balbir Singh <balbirs@...dia.com>, linux-mm@...ck.org,
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 Jul 2025, at 10:40, David Hildenbrand wrote:
> On 11.07.25 16:37, Zi Yan wrote:
>> On 11 Jul 2025, at 2:41, David Hildenbrand wrote:
>>
>>> 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).
>>
>> OK. I was using clang-format. It gave me this indentation.
>>>
>>> [...]
>>>
>>>> @@ -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?
>>
>> I think so. Even further, I think we probably can get rid of can_split_folio()’s
>> pextra_pins and use folio_expected_ref_count() too.
>
> That will only do the right think if we know that the folio is not mapped and that there is no way it can get mapped concurrently.
>
> Otherwise, when freezing, we might ignore a mapping (where we should fail freezing).
You mean between unmap_folio() and folio_ref_freeze(), a concurrent mapping
happens? So that what folio_expected_ref_count() returns has
folio_mapcount() != 0. You are right. Thanks.
I could pull the ref_count code in folio_expected_ref_count() into a
new helper function and reuse it in can_split_folio(). That might be
out of scope. I might back to this later.
Best Regards,
Yan, Zi
Powered by blists - more mailing lists