On Tue, May 07, 2013 at 02:19:58PM -0700, Dave Hansen wrote: > > From: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx> > > Our goal here is to eventually reduce the number of repetitive > acquire/release operations on mapping->tree_lock. > > To start out, we make a version of __remove_mapping() called > __remove_mapping_nolock(). This actually makes the locking > _much_ more straighforward. > > One non-obvious part of this patch: the > > freepage = mapping->a_ops->freepage; > > used to happen under the mapping->tree_lock, but this patch > moves it to outside of the lock. All of the other > a_ops->freepage users do it outside the lock, and we only > assign it when we create inodes, so that makes it safe. > > Signed-off-by: Dave Hansen <dave.hansen@xxxxxxxxxxxxxxx> It's a stupid nit, but more often than not, foo and __foo refer to the locked and unlocked versions of a function. Other times it refers to functions with internal helpers. In this patch, it looks like there are two helpers "locked" and "really, I mean it, it's locked this time". The term "nolock" is ambiguous because it could mean either "no lock is acquired" or "no lock needs to be acquired". It's all in one file so it's hardly a major problem but I would suggest different names. Maybe remove_mapping lock_remove_mapping __remove_mapping instead of remove_mapping __remove_mapping __remove_mapping_nolock Whether you do that or not Acked-by: Mel Gorman <mgorman@xxxxxxx> -- Mel Gorman SUSE Labs -- To unsubscribe, send a message with 'unsubscribe linux-mm' in the body to majordomo@xxxxxxxxx. For more info on Linux MM, see: http://www.linux-mm.org/ . Don't email: <a href=mailto:"dont@xxxxxxxxx";> email@xxxxxxxxx </a>