Quite frankly, I think it's totally insane to walk a list of all
anon_vma's that are associated with one vma, just to lock them all.
Tell me why you just don't put the lock in the vma itself then? Walking a
list in order to lock multiple things is something we should _never_ do
under any normal circumstances.
I can see why you'd want to do this in theory (the "other side" of the
locker might have to lock just the _one_ anon_vma), but if your argument
is that the list is usually not very deep ("one"?), then there is no
advantage, because putting the lock in the anon_vma vs the vma will get
the same kind of contention.
And if the list _is_ deep, then walking the list to lock them all is a
crime against humanity.
As for patch 2/2, Mel has an alternative approach for that:
http://lkml.org/lkml/2010/4/30/198
Does Mel's patch seem more reasonable to you?
Well, I certainly think that seems to be a lot more targeted,
In particular, why don't we just make rmap_walk() be the one that locks
all the anon_vma's? Instead of locking just one? THAT is the function that
cares. THAT is the function that should do proper locking and not expect
others to do extra unnecessary locking.
So again, my gut feel is that if the lock just were in the vma itself,
then the "normal" users would have just one natural lock, while the
special case users (rmap_walk_anon) would have to lock each vma it
traverses. That would seem to be the more natural way to lock things.
Btw, Mel's patch doesn't really match the description of 2/2. 2/2 says
that all pages must always be findable in rmap. Mel's patch seems to
explicitly say "we want to ignore that thing that is busy for execve". Are
we just avoiding a BUG_ON()? Is perhaps the BUG_ON() buggy?