Messages in this thread | | | From | Linus Torvalds <> | Date | Sun, 4 Mar 2012 17:22:01 -0800 | Subject | Re: [PATCH] fix bug introduced in "mm: simplify find_vma_prev()" |
| |
On Sun, Mar 4, 2012 at 4:52 PM, Mikulas Patocka <mpatocka@redhat.com> 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
| |