lkml.org 
[lkml]   [2022]   [Jan]   [13]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] mm: reuse the unshared swapcache page in do_wp_page
    On 13.01.22 18:14, Linus Torvalds wrote:
    > On Thu, Jan 13, 2022 at 8:48 AM David Hildenbrand <david@redhat.com> wrote:
    >>
    >> I'm wondering if we can get rid of the mapcount checks in
    >> reuse_swap_page() and instead check for page_count() and swapcount only.
    >
    > Honestly, I think even checking page_count() is pointless.
    >
    > If the page has users, then that's fine. That's irrelevant for whether
    > it's a swap page or not, no?
    >
    > So if you want to remove it from the swap cache, all that matters is that
    >
    > (a) you have it locked so that there can be no new users trying to mix it up
    >
    > (b) there are no swapcount() users of this page (which don't have a
    > ref to the page itself, they only have a swap entry), so that you
    > can't have somebody trying to look it up (whether some racy
    > "concurrent" lookup _or_ any later one, since we're about to remove
    > the swap cache association).
    >
    > Why would "map_count()" matter - it's now many times the *page* is
    > mapped, it's irrelevant to swap cache status? And for the same reason,
    > what difference does "page_count()" have?
    >
    > One big reason I did the COW rewrite was literally that the rules were
    > pure voodoo and made no sense at all. There was no underlying logic,
    > it was just a random collection of random tests that didn't have any
    > logical architecture to them.
    >
    > Our VM is really really complicated already, so I really want our code
    > to make sense.
    >
    > So even if I'm entirely wrong in my swap/map-count arguments above,
    > I'd like whatever patches in this area to be really well commented and
    > have some fundamental rules and logic to them so that people can read
    > the code and go "Ok, makes sense".
    >
    > Please?

    I might be missing something, but it's not only about whether we can remove
    the page from the swap cache, it's about whether we can reuse the page
    exclusively in a process with write access, avoiding a COW. And for that we
    have to check if it's mapped somewhere else already (readable).

    Here is a sketch (buggy, untested, uncompiled) of how I think reuse_swap_page()
    could look like, removing any mapcount logic and incorporating the
    refount, leaving the page_trans_huge_swapcount() changes etc. out of the picture.

    Would that make any sense?


    bool reuse_swap_page(struct page *page, bool mapped)
    {
    int swapcount = 0, total_swapcount;

    VM_BUG_ON_PAGE(!PageLocked(page), page);
    if (unlikely(PageKsm(page)))
    return false;

    if (PageSwapCache(page)) {
    swapcount = page_trans_huge_swapcount(page, &total_swapcount);

    if (swapcount == 1 && !mapped &&
    (likely(!PageTransCompound(page)) ||
    /* The remaining swap count will be freed soon */
    total_swapcount == page_swapcount(page))) {
    if (!PageWriteback(page)) {
    page = compound_head(page);
    delete_from_swap_cache(page);
    SetPageDirty(page);
    } else {
    swp_entry_t entry;
    struct swap_info_struct *p;

    entry.val = page_private(page);
    p = swap_info_get(entry);
    if (p->flags & SWP_STABLE_WRITES) {
    spin_unlock(&p->lock);
    return false;
    }
    spin_unlock(&p->lock);
    }
    }
    }

    /*
    * We expect exactly one ref from our mapped page (if already mapped)
    * and one ref from the swapcache (if in the swapcache).
    */
    if (!mapped)
    return swapcount == 1 && page_count(page) == !!PageSwapCache(page)
    return swapcount == 0 && page_count(page) == 1 + !!PageSwapCache(page)
    }


    --
    Thanks,

    David / dhildenb

    \
     
     \ /
      Last update: 2022-01-13 18:26    [W:3.624 / U:0.148 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site