On 5/12/26 13:31, Thomas Hellström wrote:
> Hi David,
>
> Thanks for having a look.
>
> On Tue, 2026-05-12 at 13:07 +0200, David Hildenbrand (Arm) wrote:
>>
>>>
>>> +/**
>>> + * undo_compound_page() - Reverse the effect of
>>> prep_compound_page().
>>> + * @page: The head page of a compound page to demote.
>>> + *
>>> + * Returns the pages to non-compound state as if
>>> prep_compound_page()
>>> + * had never been called. split_page() must NOT have been called
>>> on
>>> + * the compound page; tail refcounts must be 0. The caller must
>>> ensure
>>> + * no other users hold references to the compound page.
>>> + */
>>> +void undo_compound_page(struct page *page)
>>> +{
>>> + unsigned int i, nr = 1U << compound_order(page);
>>> +
>>> + page[1].flags.f &= ~PAGE_FLAGS_SECOND;
>>> + for (i = 1; i < nr; i++) {
>>> + page[i].mapping = NULL;
>>> + clear_compound_head(&page[i]);
>>> + }
>>> + ClearPageHead(page);
>>> +}
>>> +
>>> static inline void set_buddy_order(struct page *page, unsigned int
>>> order)
>>> {
>>> set_page_private(page, order);
>>> diff --git a/mm/shmem.c b/mm/shmem.c
>>> index 3b5dc21b323c..45e80a74f77c 100644
>>> --- a/mm/shmem.c
>>> +++ b/mm/shmem.c
>>> @@ -937,6 +937,111 @@ int shmem_add_to_page_cache(struct folio
>>> *folio,
>>> return 0;
>>> }
>>>
>>> +/**
>>> + * shmem_insert_folio() - Insert an isolated folio into a shmem
>>> file.
>>> + * @file: The shmem file created with shmem_file_setup().
>>> + * @folio: The folio to insert. Must be isolated (not on LRU),
>>> unlocked,
>>> + * have exactly one reference (the caller's), have no
>>> page-table
>>> + * mappings, and have folio->mapping == NULL.
>>> + * @order: The allocation order of @folio. If @order > 0 and
>>> @folio is
>>> + * not already a large (compound) folio, it will be
>>> promoted to a
>>> + * compound folio of this order inside this function.
>>> This requires
>>> + * the standard post-alloc state: head refcount == 1, tail
>>> + * refcounts == 0 (i.e. split_page() must NOT have been
>>> called).
>>> + * On failure the promotion is reversed and the folio is
>>> returned
>>> + * to its original non-compound state.
>>> + * @index: Page-cache index at which to insert. Must be aligned to
>>> + * (1 << @order) and within the file's size.
>>> + * @writeback: If true, attempt immediate writeback to swap after
>>> insertion.
>>> + * Best-effort; failure is silently ignored.
>>> + * @folio_gfp: The GFP flags to use for memory-cgroup charging.
>>> + *
>>> + * The folio is inserted zero-copy into the shmem page cache and
>>> placed on
>>> + * the anon LRU, where it participates in normal kernel reclaim
>>> (written to
>>> + * swap under memory pressure). Any previous content at @index is
>>> discarded.
>>> + * On success the caller should release their reference with
>>> folio_put() and
>>> + * track the (@file, @index) pair for later recovery via
>>> shmem_read_folio()
>>> + * and release via shmem_truncate_range().
>>> + *
>>> + * Return: 0 on success. On failure the folio is returned to its
>>> original
>>> + * state and the caller retains ownership.
>>> + */
>>> +int shmem_insert_folio(struct file *file, struct folio *folio,
>>> unsigned int order,
>>> + pgoff_t index, bool writeback, gfp_t
>>> folio_gfp)
>>> +{
>>> + struct address_space *mapping = file->f_mapping;
>>> + struct inode *inode = mapping->host;
>>> + bool promoted;
>>> + long nr_pages;
>>> + int ret;
>>> +
>>> + promoted = order > 0 && !folio_test_large(folio);
>>> + if (promoted)
>>> + prep_compound_page(&folio->page, order);
>>> + nr_pages = folio_nr_pages(folio);
>>> +
>>> + VM_BUG_ON_FOLIO(folio_test_lru(folio), folio);
>>> + VM_BUG_ON_FOLIO(folio_mapped(folio), folio);
>>> + VM_BUG_ON_FOLIO(folio_test_swapcache(folio), folio);
>>> + VM_BUG_ON_FOLIO(folio->mapping, folio);
>>> + VM_BUG_ON(index != round_down(index, nr_pages));
>>
>> No new VM_BUG_ON_FOLIO etc.
>
> OK, can eliminate those. Is VM_WARN_ON_FOLIO() preferred,
> or any other type of assert?
VM_WARN_ON_FOLIO() is usually what you want, or VM_WARN_ON_ONCE().
>
>>
>> But in general, pushing in random allocated pages into shmem,
>> converting them to
>> folios is not something I particularly enjoy seeing.
>>
>
> OK, let me understand the concern. The pages are allocated as multi-
> page folios using alloc_pages(gfp, order), but typically not promoted
> to compound pages, until inserted here. Is it that promotion that is of
> concern or inserting pages of unknown origin into shmem? Anything we
> can do to alleviate that concern?
It's all rather questionable.
A couple of points:
a) The pages are allocated to be unmovable, but adding them to shmem effectively
turns them movable. Now you interfere with the page allocator logic of
placing movable and unmovable pages a reasonable way into
pageblocks that group allocations of similar types.
b) A driver is not supposed to decide which folio size will be allocated for
shmem. I am not even sure if there is a fencing on
CONFIG_TRANSPARENT_HUGEPAGE somewhere when ending up with large folios. order
> PMD_ORDER is currently essentially unsupported, and I suspect your code
would even allow for that (looking at ttm_pool_alloc_find_order).
We also have some problems with the pagecache not actually supporting all
MAX_PAGE_ORDER orders (see MAX_PAGECACHE_ORDER).
You are bypassing shmem logic to decide on that completely.
While these things might not actually cause harm for you today (although I
suspect some of them might in shmem swapout code), we don't want drivers to
make our life harder by doing completely unexpected things.
c) You pass folio + order, which is just the red flag that you are doing
something extremely dodgy.
You just cast something that is not a folio, and was not allocated to be a
folio to a folio through page_folio(page). That will stop working completely
in the future once we decouple struct page from struct folio.
If it's not a folio with a proper set order, you should be passing page +
order.
d) We are once more open-coding creation of a folio, by hand-crafting it
ourselves.
We have folio_alloc() and friends for a reason. Where we, for example, do a
page_rmappable_folio().
I am pretty sure that you are missing a call to page_rmappable_folio(),
resulting in the large folios not getting folio_set_large_rmappable() set.
e) undo_compound_page(). No words.
*maybe* it would be a little less bad if you would just allocate a compound page
in your driver and use page_rmappable_folio() in there.
That wouldn't change a) or b), though.
>
> Given the problem statement in the cover-letter, would there be a
> better direction to take here? We could, for example, bypass shmem and
> insert the folios directly into the swap-cache, (although there is an
> issue with the swap-cache when the number of swap_entries are close to
> being depleted).
Good question.
We'd have to keep swapoff and all of that working. For example, in
try_to_unuse(), we special-case shmem_unuse() to handle non-anonymous pages.
But then, the whole swapcache operates on folios ... so I am not sure if there
is a lot to be won by re-implementing what shmem already does?
--
Cheers,
David
