linux-kernel - Re: [RFC][PATCH 3/7] break up __remove

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: <20130514152247.GU11497@suse.de>
Date:	Tue, 14 May 2013 16:22:47 +0100
From:	Mel Gorman <mgorman@...e.de>
To:	Dave Hansen <dave@...1.net>
Cc:	linux-mm@...ck.org, linux-kernel@...r.kernel.org,
	akpm@...ux-foundation.org, tim.c.chen@...ux.intel.com
Subject: Re: [RFC][PATCH 3/7] break up __remove_mapping()

On Tue, May 07, 2013 at 02:19:58PM -0700, Dave Hansen wrote:
> 
> From: Dave Hansen <dave.hansen@...ux.intel.com>
> 
> 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@...ux.intel.com>

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@...e.de>

-- 
Mel Gorman
SUSE Labs
--
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/