Re: [PATCH] mm/mlock: fix BUG_ON unlocked page for nolinear VMAs

[Linus Torvalds]

On Fri, Jan 3, 2014 at 3:36 PM, Vlastimil Babka <vbabka@xxxxxxx> wrote:
>
> I'm for going with the removal of BUG_ON. The TestSetPageMlocked should provide enough
> race protection.

Maybe. But dammit, that's subtle, and I don't think you're even right.

It basically depends on mlock_vma_page() and munlock_vma_page() being
able to run CONCURRENTLY on the same page. In particular, you could
have a mlock_vma_page() set the bit on one CPU, and munlock_vma_page()
immediately clearing it on another, and then the rest of those
functions could run with a totally arbitrary interleaving when working
with the exact same page.

They both do basically

    if (!isolate_lru_page(page))
        putback_lru_page(page);

but one or the other would randomly win the race (it's internally
protected by the lru lock), and *if* the munlock_vma_page() wins it,
it would also do

    try_to_munlock(page);

but if mlock_vma_page() wins it, that wouldn't happen. That looks
entirely broken - you end up with the PageMlocked bit clear, but
try_to_munlock() was never called on that page, because
mlock_vma_page() got to the page isolation before the "subsequent"
munlock_vma_page().

And this is very much what the page lock serialization would prevent.
So no, the PageMlocked in *no* way gives serialization. It's an atomic
bit op, yes, but that only "serializes" in one direction, not when you
can have a mix of bit setting and clearing.

So quite frankly, I think you're wrong. The BUG_ON() is correct, or at
least enforces some kind of ordering. And try_to_unmap_cluster() is
just broken in calling that without the page being locked. That's my
opinion. There may be some *other* reason why it all happens to work,
but no, "TestSetPageMlocked should provide enough race protection" is
simply not true, and even if it were, it's way too subtle and odd to
be a good rule.

So I really object to just removing the BUG_ON(). Not with a *lot*
more explanation as to why these kinds of issues wouldn't matter.

                 Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/