linux-kernel - Re: [PATCH 4/5] mm/rmap: share the i_mmap

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1401137322.12982.5.camel@buesod1.americas.hpqcorp.net>
Date:	Mon, 26 May 2014 13:48:42 -0700
From:	Davidlohr Bueso <davidlohr@...com>
To:	Hugh Dickins <hughd@...gle.com>
Cc:	akpm@...ux-foundation.org, mingo@...nel.org, peterz@...radead.org,
	riel@...hat.com, mgorman@...e.de, aswin@...com, linux-mm@...ck.org,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 4/5] mm/rmap: share the i_mmap_rwsem

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@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/