Re: [PATCH 4/5] mm/rmap: share the i_mmap_rwsem

[Davidlohr Bueso]

On Mon, 2014-05-26 at 12:35 -0700, Hugh Dickins wrote:
> On Thu, 22 May 2014, Davidlohr Bueso wrote:
> 
> > Similarly to rmap_walk_anon() and collect_procs_anon(),
> > there is opportunity to share the lock in rmap_walk_file()
> > and collect_procs_file() for file backed pages.
> 
> And lots of other places, no?  I welcome i_mmap_rwsem, but I think
> you're approaching it wrongly to separate this off from 2/5, then
> follow anon_vma for the places that can be converted to lock_read().

Sure, but as you can imagine, the reasoning behind it is simplicity and
bisectability. 2/5 is easy to commit typo-like errors, and end up
locking instead of unlocking and vice versa. I ran into a few while
testing and wanted to make life easier for reviewers.

> If you go back through 2/5 and study the context of each, I think
> you'll find most make no modification to the tree, and can well
> use the lock_read() rather than the lock_write().

I was planning on revisiting some of that. I have no concrete examples
yet, but I agree, there could very well be further opportunity to share
the lock in read-only paths. This 4/5 is just the first, and most
obvious, step towards improving the usage of the i_mmap lock.

> I could be wrong, but I don't think there are any hidden gotchas.
> There certainly are in the anon_vma case (where THP makes special
> use of the anon_vma lock), and used to be in the i_mmap_lock case
> (when invalidation had to be single-threaded across cond_rescheds),
> but I think i_mmap_rwsem should be straightforward.
> 
> Sure, it's safe to use the lock_write() variant, but please don't
> prefer it to lock_read() without good reason.

I will dig deeper (probably for 3.17 now), but I really believe this is
the correct way of splitting the patches for this particular series.

Thanks,
Davidlohr

--
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/