Re: [PATCH v3 RESEND] mm: shmem: implement POSIX_FADV_[WILL|DONT]NEED for shmem

[Charan Teja Kalla]

Thanks Mark!!

On 1/12/2022 5:08 PM, Mark Hemment wrote:
>>>> +static int shmem_fadvise_dontneed(struct address_space *mapping, loff_t start,
>>>> +                               loff_t end)
>>>> +{
>>>> +       int ret;
>>>> +       struct page *page;
>>>> +       LIST_HEAD(list);
>>>> +       struct writeback_control wbc = {
>>>> +               .sync_mode = WB_SYNC_NONE,
>>>> +               .nr_to_write = LONG_MAX,
>>>> +               .range_start = 0,
>>>> +               .range_end = LLONG_MAX,
>>>> +               .for_reclaim = 1,
>>>> +       };
>>>> +
>>>> +       if (!shmem_mapping(mapping))
>>>> +               return -EINVAL;
>>>> +
>>>> +       if (!total_swap_pages)
>>>> +               return 0;
>>>> +
>>>> +       lru_add_drain();
>>>> +       shmem_isolate_pages_range(mapping, start, end, &list);
>>>> +
>>>> +       while (!list_empty(&list)) {
>>>> +               page = lru_to_page(&list);
>>>> +               list_del(&page->lru);
>>>> +               if (page_mapped(page))
>>>> +                       goto keep;
>>>> +               if (!trylock_page(page))
>>>> +                       goto keep;
>>>> +               if (unlikely(PageTransHuge(page))) {
>>>> +                       if (split_huge_page_to_list(page, &list))
>>>> +                               goto keep;
>>>> +               }
>>> I don't know the shmem code and the lifecycle of a shm-page, so
>>> genuine questions;
>>> When the try-lock succeeds, should there be a test for PageWriteback()
>>> (page skipped if true)?  Also, does page->mapping need to be tested
>>> for NULL to prevent races with deletion from the page-cache?
>> I failed to envisage it. I should have considered both these conditions
>> here. BTW, I am just thinking about why we shouldn't use
>> reclaim_pages(page_list) function here with an extra set_page_dirty() on
>> a page that is isolated? It just call the shrink_page_list() where all
>> these conditions are properly handled. What is your opinion here?
> Should be possible to use reclaim_pages() (I haven't look closely).
> It might actually be good to use this function, as will do some
> congestion throttling.  Although it will always try to unmap
> pages (note: your page_mapped() test is 'unstable' as done without the
> page locked), so might give behaviour you want to avoid.

page_mapped can be true between isolate and then asking for reclaim of
it through reclaim_pages(), and then can be unmapped there. Thanks for
pointing it out.

BTW, the posix_fadvise man pages[1] doesn't talk about any restrictions
with the mapped pages. If so, Am I allowed to even unmap the pages when
called FADV_DONTNEED on the file (agree for mapped, we can rely on
MADV_DONTNEED too)?

[1]https://man7.org/linux/man-pages/man2/posix_fadvise.2.html

> Note: reclaim_pages() is already used for madvise(PAGEOUT).  The shmem
> code would need to prepare page(s) to help shrink_page_list() to make
> progress (see madvise.c:madvise_cold_or_pageout_pte_range()).
> 
> Taking a step back; is fadvise(DONTNEED) really needed/wanted?  Yes,
> you gave a usecase (which I cut from this thread in my earlier reply),
> but I'm not familiar with various shmem uses to know if this feature
> is needed.  Someone else will need to answer this.

Actually I needed this for the use case mentioned. And regarding the
various use cases, I encountered that GEM buffers for display/graphics
are using the shmem buffers.
drivers/gpu/drm/i915/gem/i915_gem_shmem.c