Re: [PATCH] anobjrmap 9 priority mjb tree

[Andrea Arcangeli <andrea@suse.de>]

On Wed, Apr 14, 2004 at 11:40:52PM -0400, Rajesh Venkatasubramanian wrote:
> 
> > >   2) However, vmas can be added and removed from a vm_set list
> > >      by just holding the read lock and a bit lock (vm_set_lock)
> > >      in the corresponding prio_tree node.
> >
> > no way, you cannot bitflip vm_flags unless you own the mmap_sem, this
> > patch seems very broken to me, it should randomly corrupt memory in
> > vma->vm_flags while racing against mprotect etc.. or am I missing
> > something?
> 
> I don't know why bit_spin_lock with vma->vm_flags should be a problem
> if it is used without mmap_sem. Can you explain ?

you seem not to know all rules about the atomic operations in smp, you
cannot just set_bit on one side and use non-atomic operations on the
other side, and expect the set_bit not to invalidate the non-atomic
operations.

The effect of the mprotect may be deleted by your new concurrent
set_bit and stuff like that.

> If it is really racy to use bit_spin_lock on vm_flags without mmap_sem

it __definitely__ is racy.

> Well. In that case, we can use rwsem as you mentioned below: take
> down_write on all modifications and take down_read on pageout. Here, you

exactly, this also avoids the more complex (and racy, but the racy would
be easy to fix by adding another 4 atomic bytes to the vma) double
locking that you introduced.

> allow multiple parallel page_referenced and try_to_unmap on the same
> inode, however with only one modification at a time.

exactly.

> Wherease my solution will allow multiple modifications at the same
> time (if possible) with only one pageout routine at a time. I chose
> this solution because Martin's SDET took big hit in common cases of
> adding and removing vmas from the i_mmap{_shared} data structure.

you can still fix the smp race condition by trivially adding 4 bytes to
the vma (i.e. a vma->vm_flags_atomic), but I'd be surprised if your
double locking actually improve things, Martin is running on a very
parallel old-numa where cacheline bouncing across nodes pass through a
fibre channel IIRC, and the cacheline bouncing that the semaphore
generates is huge, it's not necessairly huge contention, it's just the
bouncing that hurts, and the down_read won't help at all for the
cacheline trashing, it'll still bounce like before. Though you may gain
something minor, but I doubt it'd be huge.

I'd suggest Martin to give a try to the racy code, it's just good enough
for a pratical experiment (the race shouldn't easily trigger so it
probably passes one run of SDET safely).