lkml.org 
[lkml]   [2018]   [Nov]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC PATCH 3/3] mm, fault_around: do not take a reference to a locked page
    On Wed 21-11-18 18:27:11, Hugh Dickins wrote:
    > On Wed, 21 Nov 2018, Michal Hocko wrote:
    > > On Tue 20-11-18 17:47:21, Hugh Dickins wrote:
    > > > On Tue, 20 Nov 2018, Michal Hocko wrote:
    > > >
    > > > > From: Michal Hocko <mhocko@suse.com>
    > > > >
    > > > > filemap_map_pages takes a speculative reference to each page in the
    > > > > range before it tries to lock that page. While this is correct it
    > > > > also can influence page migration which will bail out when seeing
    > > > > an elevated reference count. The faultaround code would bail on
    > > > > seeing a locked page so we can pro-actively check the PageLocked
    > > > > bit before page_cache_get_speculative and prevent from pointless
    > > > > reference count churn.
    > > > >
    > > > > Cc: "Kirill A. Shutemov" <kirill@shutemov.name>
    > > > > Suggested-by: Jan Kara <jack@suse.cz>
    > > > > Signed-off-by: Michal Hocko <mhocko@suse.com>
    > > >
    > > > Acked-by: Hugh Dickins <hughd@google.com>
    > >
    > > Thanks!
    > >
    > > > though I think this patch is more useful to the avoid atomic ops,
    > > > and unnecessary dirtying of the cacheline, than to avoid the very
    > > > transient elevation of refcount, which will not affect page migration
    > > > very much.
    > >
    > > Are you sure it would really be transient? In other words is it possible
    > > that the fault around can block migration repeatedly under refault heavy
    > > workload? I just couldn't convince myself, to be honest.
    >
    > I don't deny that it is possible: I expect that, using fork() (which does
    > not copy the ptes in a shared file vma), you can construct a test case
    > where each child faults one or another page near a page of no interest,
    > and that page of no interest is a target of migration perpetually
    > frustrated by filemap_map_pages()'s briefly raised refcount.

    The other issue I am debugging and which very likely has the same
    underlying issue in the end has shown
    [ 883.930477] rac1 kernel: page:ffffea2084bf5cc0 count:1889 mapcount:1887 mapping:ffff8833c82c9ad8 index:0x6b
    [ 883.930485] rac1 kernel: ext4_da_aops [ext4]
    [ 883.930497] rac1 kernel: name:"libc-2.22.so"
    [ 883.931241] rac1 kernel: do_migrate_range done ret=23

    pattern. After we have disabled the faultaround the failure has moved to
    a different page but libc hasn't shown up again. This might be a matter
    of (bad)luck and timing. But we thought that it is not too unlikely for
    faultaround on such a shared page to strike in.

    > But I suggest that's a third-order effect: well worth fixing because
    > it's easily and uncontroversially dealt with, as you have; but not of
    > great importance.
    >
    > The first-order effect is migration conspiring to defeat itself: that's
    > what my put_and_wait_on_page_locked() patch, in other thread, is about.

    yes. That is obviously a much more effective fix.

    > The second order effect is when a page that is really wanted is waited
    > on - the target of a fault, for which page refcount is raised maybe
    > long before it finally gets into the page table (whereupon it becomes
    > visible to try_to_unmap(), and its mapcount matches refcount so that
    > migration can fully account for the page). One class of that can be
    > well dealt with by using put_and_wait_on_page_locked_killable() in
    > lock_page_or_retry(), but I was keeping that as a future instalment.
    >
    > But I shouldn't denigrate the transient case by referring so lightly
    > to migrate_pages()' 10 attempts: each of those failed attempts can
    > be very expensive, unmapping and TLB flushing (including IPIs) and
    > remapping. It may well be that 2 or 3 would be a more cost-effective
    > number of attempts, at least when the page is mapped.

    If you want some update to the comment in this function or to the
    changelog, I am open of course. Right now I have
    + * Check for a locked page first, as a speculative
    + * reference may adversely influence page migration.
    as suggested by William.
    --
    Michal Hocko
    SUSE Labs

    \
     
     \ /
      Last update: 2018-11-22 10:07    [W:2.312 / U:0.028 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site