lkml.org 
[lkml]   [2019]   [Jul]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v9 5/6] mm,thp: add read-only THP support for (non-shmem) FS
    Date


    > On Jul 10, 2019, at 11:48 AM, Johannes Weiner <hannes@cmpxchg.org> wrote:
    >
    > On Mon, Jun 24, 2019 at 05:12:45PM -0700, Song Liu wrote:
    >> This patch is (hopefully) the first step to enable THP for non-shmem
    >> filesystems.
    >>
    >> This patch enables an application to put part of its text sections to THP
    >> via madvise, for example:
    >>
    >> madvise((void *)0x600000, 0x200000, MADV_HUGEPAGE);
    >>
    >> We tried to reuse the logic for THP on tmpfs.
    >>
    >> Currently, write is not supported for non-shmem THP. khugepaged will only
    >> process vma with VM_DENYWRITE. sys_mmap() ignores VM_DENYWRITE requests
    >> (see ksys_mmap_pgoff). The only way to create vma with VM_DENYWRITE is
    >> execve(). This requirement limits non-shmem THP to text sections.
    >>
    >> The next patch will handle writes, which would only happen when the all
    >> the vmas with VM_DENYWRITE are unmapped.
    >>
    >> An EXPERIMENTAL config, READ_ONLY_THP_FOR_FS, is added to gate this
    >> feature.
    >>
    >> Acked-by: Rik van Riel <riel@surriel.com>
    >> Signed-off-by: Song Liu <songliubraving@fb.com>
    >
    > This is really cool, and less invasive than I anticipated. Nice work.
    >
    > I only have one concern and one question:
    >
    >> @@ -1392,6 +1401,29 @@ static void collapse_file(struct mm_struct *mm,
    >> result = SCAN_FAIL;
    >> goto xa_unlocked;
    >> }
    >> + } else if (!page || xa_is_value(page)) {
    >> + xas_unlock_irq(&xas);
    >> + page_cache_sync_readahead(mapping, &file->f_ra, file,
    >> + index, PAGE_SIZE);
    >> + /* drain pagevecs to help isolate_lru_page() */
    >> + lru_add_drain();
    >> + page = find_lock_page(mapping, index);
    >> + if (unlikely(page == NULL)) {
    >> + result = SCAN_FAIL;
    >> + goto xa_unlocked;
    >> + }
    >> + } else if (!PageUptodate(page)) {
    >> + VM_BUG_ON(is_shmem);
    >> + xas_unlock_irq(&xas);
    >> + wait_on_page_locked(page);
    >> + if (!trylock_page(page)) {
    >> + result = SCAN_PAGE_LOCK;
    >> + goto xa_unlocked;
    >> + }
    >> + get_page(page);
    >> + } else if (!is_shmem && PageDirty(page)) {
    >> + result = SCAN_FAIL;
    >> + goto xa_locked;
    >> } else if (trylock_page(page)) {
    >> get_page(page);
    >> xas_unlock_irq(&xas);
    >
    > The many else ifs here check fairly complex page state and are hard to
    > follow and verify mentally. In fact, it's a bit easier now in the
    > patch when you see how it *used* to work with just shmem, but the end
    > result is fragile from a maintenance POV.
    >
    > The shmem and file cases have little in common - basically only the
    > trylock_page(). Can you please make one big 'if (is_shmem) {} {}'
    > structure instead that keeps those two scenarios separate?

    Good point! Will fix in next version.

    >
    >> @@ -1426,6 +1458,12 @@ static void collapse_file(struct mm_struct *mm,
    >> goto out_unlock;
    >> }
    >>
    >> + if (page_has_private(page) &&
    >> + !try_to_release_page(page, GFP_KERNEL)) {
    >> + result = SCAN_PAGE_HAS_PRIVATE;
    >> + break;
    >> + }
    >> +
    >> if (page_mapped(page))
    >> unmap_mapping_pages(mapping, index, 1, false);
    >
    >> @@ -1607,6 +1658,17 @@ static void khugepaged_scan_file(struct mm_struct *mm,
    >> break;
    >> }
    >>
    >> + if (page_has_private(page) && trylock_page(page)) {
    >> + int ret;
    >> +
    >> + ret = try_to_release_page(page, GFP_KERNEL);
    >> + unlock_page(page);
    >> + if (!ret) {
    >> + result = SCAN_PAGE_HAS_PRIVATE;
    >> + break;
    >> + }
    >> + }
    >> +
    >> if (page_count(page) != 1 + page_mapcount(page)) {
    >> result = SCAN_PAGE_COUNT;
    >> break;
    >
    > There is already a try_to_release() inside the page lock section in
    > collapse_file(). I'm assuming you added this one because private data
    > affects the refcount. But it seems a bit overkill just for that; we
    > could also still fail the check, in which case we'd have dropped the
    > buffers in vain. Can you fix the check instead?
    >
    > There is an is_page_cache_freeable() function in vmscan.c that handles
    > private fs references:
    >
    > static inline int is_page_cache_freeable(struct page *page)
    > {
    > /*
    > * A freeable page cache page is referenced only by the caller
    > * that isolated the page, the page cache and optional buffer
    > * heads at page->private.
    > */
    > int page_cache_pins = PageTransHuge(page) && PageSwapCache(page) ?
    > HPAGE_PMD_NR : 1;
    > return page_count(page) - page_has_private(page) == 1 + page_cache_pins;
    > }
    >
    > Wouldn't this work here as well?

    Good point! Let me try fix this.

    Thanks,
    Song

    \
     
     \ /
      Last update: 2019-07-23 01:44    [W:2.217 / U:0.072 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site