Re: [PATCH 1/2] mm/shmem: add shmem_insert_folio()

["Christian König" <christian.koenig@amd.com>]

Hi David & Thomas,

On 5/12/26 22:03, David Hildenbrand (Arm) wrote:
> On 5/12/26 13:31, Thomas Hellström wrote:
...
>>>> +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.

Exactly that is one of the major reasons why we aren't using a shmem as backing store for TTM buffers in the first place.

While HW today can usually work with everything down to 4k it needs higher order pages for optimal performance.

So for example for AMD GPUs you need 2M pages or otherwise the performance goes down by ~30% in quite a number of use cases.

Everything between 4k and 2M and above 2M is still preferred because it results in better L0/L1 reach, but if you can't get 2M the L2 reach goes down so rapidly that people start to complain immediately. 

And that stuff is very specific for each vendor and HW generation. Some have the sweet spot at 64k, some at 256k, most at 2M.

>    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.

Yeah but that is the requirement the HW has.

I mean we can keep torturing the buddy allocator to give us 2M pages, but essentially we want to get away from those specialized solutions and has more of the functionality necessary to driver the HW in the common Linux memory management code because that prevents vendors from re-implementing that stuff in their specific driver over and over again.

Regards,
Christian.

> 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?
>