Re: [PATCH] fix bug introduced in "mm: simplify find_vma_prev()"

[Linus Torvalds]

On Sun, Mar 4, 2012 at 4:52 PM, Mikulas Patocka <mpatocka@xxxxxxxxxx> wrote:
>
> This patch fixes a bug introduced in "mm: simplify find_vma_prev()". You
> can apply this, or alternatively revert the original patch.

Hmm.

I realize the patch is a bug-fix, but I do wonder if we shouldn't look
at alternatives.

In particular, how about we just change the rule to be clearer, and
make the rule be:

 - no more find_vma_prev() AT ALL.

 - callers get turned into "find_vma()" and then we have a
"vma_prev()", which basically does your thing.

 - many of the callers seem to be interested in "prev" *only* if they
found a vma. For example, the do_mlock() kind of logic seems to be
common:

    vma = find_vma_prev(current->mm, start, &prev);
    if (!vma || vma->vm_start > start)
        return -ENOMEM;

   which implies that the current find_vma_prev() semantics are really
wrong, because they force us to do extra work in this case where we
really really don't care about the result.

So we could do an entirely mechanical conversion of

   vma = find_vma_prev(mm, address, &prev_vma);

into

   vma = find_vma(mm, address);
   prev_vma = vma_prev(vma);

and then after we've done that conversion, it looks like the bulk of
them will not care about "prev_vma" if vma is NULL, so they can then
be replaced with a pure

   prev_vma = vma->vm_prev;

instead. Leaving just the (few) cases that may care about the
"previous vma may be the last vma of the VM" case.

Hmm? And we'd get away from that horrible "find_vma_prev()" interface
that uses pointers to vma pointers for return values. Ugh. There only
seems to be nine callers in the whole kernel.

                 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/