lkml.org 
[lkml]   [2010]   [Mar]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] proc: pagemap: Hold mmap_sem during page walk


    On Wed, 31 Mar 2010, Matt Mackall wrote:
    >
    > Linus, I must say your charm has really worn thin. I've just stuck a
    > post-it on my monitor saying "don't be Linus" to remind me not to be
    > rude to my contributors.

    You didn't actually answer the problem, though.

    I'm rude, because I think the code is buggy. I pointed out how, and why I
    think it's pretty fundamental. You quoted it, but you didn't answer it.

    > If I recall correctly, the get_user_pages is there to force all the
    > output pages to be guaranteed present before we later fill them so that
    > the output needn't be double-buffered and the locking and recursion on
    > the page tables is significantly simpler and faster. put_user is then
    > actually easier than "writing to the physical pages through the array".

    Umm. Why would get_user_pages() guarantee that the pages don't get swapped
    out in the meantime, and you end up with a page fault _anyway_?

    Yes, it makes those page faults much rarer, but that just makes things
    much worse. Now you have a nasty subtle fundamental bug that is hard to
    trigger too.

    NOTE! The fact that we mark the page dirty (and increment the page count)
    in get_user_pages() (not in the later loop, which does indeed look
    pointless) should mean that the page doesn't go away physically, and if it
    gets mapped out the same page will be mapped back in on access. That's how
    things like direct-IO work (modulo a concurrent fork(), when they don't
    work, but that's a separate issue).

    But the problem with the deadlock is not that "same physical page" issue,
    it's simply the problem of the page fault code itself wanting to do a
    down_read() on the mmap_sem. So the fact that the physical page is
    reliable in the array doesn't actually solve the bug I was pointing out.

    See?

    So the

    nr = get_user_pages(.. 1, 0, ..)

    ...

    for_each_page()
    mark_it_dirty();

    pattern is a valid pattern, but it is _only_ valid if you then do the
    write to the physical page you looked up. If you do the accesses through
    the virtual addresses, it makes zero sense.

    > You're right that the SetPageDirty() at cleanup is redundant (but
    > harmless), and I probably copied that pattern from another user of
    > get_user_pages.

    Fine. So then you'd do get_user_pages() and do _nothing_ with the array?
    Please explain what the point of that is, again?

    See above: there are valid reasons for things like direct-IO to do that
    exact "get_user_pages()" pattern - they mark the pages dirty both early
    _and_ late, and both page dirtying is required:

    - the first one (done by get_user_pages() itself) is to make sure that an
    anonymous page doesn't get switched around to another anonymous page
    that just happens to have the same contents (ie if the page gets
    swapped out, the swapout code will turn it into a swap-cache page).

    Once it's a swap-out page, the page refcount will then guarantee that
    it doesn't get replaced with anything else, so now you can trust the
    physical page.

    - the second "mark it dirty afterwards" is required in case swapout did
    happen, and the page got marked clean before the direct-IO actually
    wrote to it. So you mark it dirty again - this time not to force any
    swap cache things, but simply because there might have been a race
    between cleaning the page and the thing that wrote to it through the
    physical address.

    So there is a very real reason for that pattern existing. It's just that
    that reason has nothing to do with locking the thing into the page tables.
    That is absolutely NOT guaranteed by the get_user_pages() physical page
    pinning (munmap() is an extreme example of this, but I think swapout will
    clear it too in try_to_unmap_one().

    In fact, even without swapping the page out, the accessed and dirty bit
    stuff may end up being done by software and have that page fault happen.

    What I'm trying to explain is simply that all these VM rules mean that you
    must never do a virtual user space access while holding mmap_sem. And
    doing get_user_pages() in no way changes that fundamental rule.

    Now, I will _happily_ admit that the above is all very complicated, and
    very easy to get wrong. There is a real reason why I hate direct-IO. It's
    a total nightmare from a VM standpoint. We've had bugs here. So I can well
    see that people don't always understand all the rules.

    Quite frankly, the only reason that /proc/self/pagemap code got merged is
    because it came through Andrew. Andrew knows the VM layer. Probably way
    better than I do by now. So when I get patches that touch things like this
    through the -mm tree, I tend to apply them.

    And looking now at the code again, I really think it was a mistake.

    > Earlier versions of the pagewalk code studiously avoided calling
    > find_vma and only looked at the page tables (with either caller doing
    > locking or accepting the non-atomicity) to avoid the sort of issue that
    > San has run into, but it looks like I forgot about that and let it sneak
    > back in when other folks added more hugepage support.

    Ok, that would fix the problem. The page tables can be accessed even
    without holding mmap_sem. And once you don't need mmap_sem, all the
    deadlocks go away. The get_user_pages() pattern still doesn't make sense,
    but at that point it's a _harmless_ problem, and doesn't really hurt.

    See what I'm saying?

    > The deeper problem is that hugepages have various nasty layering
    > violations associated with them like being tied to vmas so until some
    > hugepage genius shows up and tells us how to do this cleanly, we'll
    > probably have to deal with the associated mmap_sem.
    >
    > San, your patch is touching the wrong mm_sem, I think. The interesting
    > races are going to happen on the target mm, not current's (generally not
    > the same).

    The thing is, you need to hold the mmap_sem over the whole loop in
    pagemap_pte_range (since you're using the vma inside the loop). And that
    means that you'd hold mmap sem over the add_to_pagemap(), and the
    put_user() reference. Which has deadlock bug I pointed out, and that you
    totally ignored.

    The fact that it's a separate mm doesn't change the deadlock in _any_
    shape or form. It _could_ be the same mm, but even if it was another VM
    entirely, it would just make the deadlock possible as an ABBA thing across
    two different VM's instead of through a single VM.

    > Holding the mm_sem across the entire walk is also prohibitive, it
    > probably needs to be localized to individual find_vma calls.

    You'd need to do it for each individual page, as far as I can tell, and
    move the find_vma() inside the loop itself in order to avoid the whole
    "hold mmap-sem while potentially doing a page fault" case.

    At that point, it looks like it wouldn't be buggy any more, it would just
    suck horribly from a performance standpoint.

    So Matt, please actually address the _bug_ I pointed out rather than talk
    about other things. And yes, getting rid of the vma accesses sounds like
    it would fix it best. If that means that it doesn't work for hugepages, so
    be it.

    Linus


    \
     
     \ /
      Last update: 2010-04-01 03:41    [W:4.950 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site