| 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: <5f0fe985-7512-4a94-aa5b-ddae176838a1@redhat.com>
Date: Wed, 9 Apr 2025 11:03:26 +0200
From: David Hildenbrand <david@...hat.com>
To: Matthew Wilcox <willy@...radead.org>, nifan.cxl@...il.com
Cc: muchun.song@...ux.dev, mcgrof@...nel.org, a.manzanares@...sung.com,
dave@...olabs.net, akpm@...ux-foundation.org, linux-mm@...ck.org,
linux-kernel@...r.kernel.org, Fan Ni <fan.ni@...sung.com>
Subject: Re: [PATCH] mm/hugetlb: Convert &folio->page to folio_page(folio, 0)
On 09.04.25 05:15, Matthew Wilcox wrote:
> On Tue, Apr 08, 2025 at 05:49:10PM -0700, nifan.cxl@...il.com wrote:
>> From: Fan Ni <fan.ni@...sung.com>
>>
>> Convert the use of &folio->page to folio_page(folio, 0) where struct
>> filio fits in. This is part of the efforts to move some fields out of
>> struct page to reduce its size.
>
> Thanks for sending the patch. You've mixed together quite a few things;
> I'd suggest focusing on one API at a time.
Agreed.
>
>> folio_get(folio);
>> - folio_add_file_rmap_pmd(folio, &folio->page, vma);
>> + folio_add_file_rmap_pmd(folio, folio_page(folio, 0), vma);
>> add_mm_counter(mm, mm_counter_file(folio), HPAGE_PMD_NR);
>
> I think this is fine, but would defer to David Hildenbrand.
For now this should be fine. We want a pointer at the actual first page.
In some future (with folios spanning multiple PMDs), this will not be
correct.
But the THP changes should *absolutely* not be included in this hugetlb
patch. I was severly confused staring at the usage of
folio_add_file_rmap_pmd() in hugetlb context/
Actually, having to go back to my comments below to fix them up now
that I see that this is
mm/huge_memory.c | 30 +++++++++++++++++-------------
mm/hugetlb.c | 20 +++++++++++---------
mm/hugetlb_vmemmap.c | 12 ++++++------
Makes me angry.
>
>> folio_get(folio);
>> - folio_add_file_rmap_pud(folio, &folio->page, vma);
>> + folio_add_file_rmap_pud(folio, folio_page(folio, 0), vma);
>> add_mm_counter(mm, mm_counter_file(folio), HPAGE_PUD_NR);
>
> If that is fine, then so is this (put them in the same patchset).
>
>> spin_unlock(ptl);
>> - if (flush_needed)
>> - tlb_remove_page_size(tlb, &folio->page, HPAGE_PMD_SIZE);
>> + if (flush_needed) {
>> + tlb_remove_page_size(tlb, folio_page(folio, 0),
>> + HPAGE_PMD_SIZE);
>> + }
>
> You don't need to add the extra braces here. I haven't looked into this
> family of APIs; not sure if we should be passing the folio here or
> if it should be taking a folio argument.
>
>> if (folio_maybe_dma_pinned(src_folio) ||
>> - !PageAnonExclusive(&src_folio->page)) {
>> + !PageAnonExclusive(folio_page(src_folio, 0))) {
>> err = -EBUSY;
>
> mmm. Another David question.
For now this should be correct. (first page mapped by the PMD stores the
flag)
>
>> for (i = new_nr_pages; i < nr_pages; i += new_nr_pages) {
>> - struct page *new_head = &folio->page + i;
>> + struct page *new_head = folio_page(folio, i);
>>
>
> This is definitely the right thing to do.
>
>> @@ -3403,7 +3405,7 @@ static void __split_folio_to_order(struct folio *folio, int old_order,
>> if (new_order)
>> folio_set_order(folio, new_order);
>> else
>> - ClearPageCompound(&folio->page);
>> + ClearPageCompound(folio_page(folio, 0));
>> }
>
> I might be inclined to leave this one alone; this whole function needs
> to be rewritten as part of the folio split.
>
>> folio_split_memcg_refs(folio, old_order, split_order);
>> - split_page_owner(&folio->page, old_order, split_order);
>> + split_page_owner(folio_page(folio, 0), old_order, split_order);
>> pgalloc_tag_split(folio, old_order, split_order);
>
> Not sure if split_folio_owner is something that should exist. Haven't
> looked into it.
>
>> */
>> - free_page_and_swap_cache(&new_folio->page);
>> + free_page_and_swap_cache(folio_page(new_folio, 0));
>> }
>
> free_page_and_swap_cache() should be converted to be
> free_folio_and_swap_cache().
>
>>
>> - return __folio_split(folio, new_order, &folio->page, page, list, true);
>> + return __folio_split(folio, new_order, folio_page(folio, 0), page,
>> + list, true);
>> }
>
> Probably right.
>
>> {
>> - return __folio_split(folio, new_order, split_at, &folio->page, list,
>> - false);
>> + return __folio_split(folio, new_order, split_at, folio_page(folio, 0),
>> + list, false);
>> }
>
> Ditto.
>
>>
>> - return split_huge_page_to_list_to_order(&folio->page, list, ret);
>> + return split_huge_page_to_list_to_order(folio_page(folio, 0), list,
>> + ret);
>> }
>
> Ditto.
>
>>
>> - if (is_migrate_isolate_page(&folio->page))
>> + if (is_migrate_isolate_page(folio_page(folio, 0)))
>> continue;
>
> I think we need an is_migrate_isolate_folio() instead of this.
>
>> if (folio_test_anon(folio))
>> - __ClearPageAnonExclusive(&folio->page);
>> + __ClearPageAnonExclusive(folio_page(folio, 0));
>> folio->mapping = NULL;
>
> ... David.
See above.
>
>>
>> - split_page_owner(&folio->page, huge_page_order(src), huge_page_order(dst));
>> + split_page_owner(folio_page(folio, 0), huge_page_order(src),
>> + huge_page_order(dst));
>
> See earlier.
>
>> if (folio_mapcount(old_folio) == 1 && folio_test_anon(old_folio)) {
>> - if (!PageAnonExclusive(&old_folio->page)) {
>> + if (!PageAnonExclusive(folio_page(old_folio, 0))) {
>> folio_move_anon_rmap(old_folio, vma);
>> - SetPageAnonExclusive(&old_folio->page);
>> + SetPageAnonExclusive(folio_page(old_folio, 0));
>> }
>
> David.
See above.
>
>> }
>> VM_BUG_ON_PAGE(folio_test_anon(old_folio) &&
>> - PageAnonExclusive(&old_folio->page), &old_folio->page);
>> + PageAnonExclusive(folio_page(old_folio, 0)),
>> + folio_page(old_folio, 0));
>
> The PageAnonExclusive() part of this change is for David to comment on,
> but this should be a VM_BUG_ON_FOLIO() instead of calling folio_page()
> to keep this a VM_BUG_ON_PAGE().
Agreed.
--
Cheers,
David / dhildenb
Powered by blists - more mailing lists