Date: Fri, 14 Sep 2007 17:07:33 -0700
From: Andrew Morton <akpm@linux-foundation.org>
To: "Satyam Sharma" <satyam.sharma@gmail.com>
Cc: "Ethan Solomita" <solo@google.com>, linux-mm@kvack.org,
       LKML <linux-kernel@vger.kernel.org>,
       "Christoph Lameter" <clameter@sgi.com>
Subject: Re: [PATCH 1/6] cpuset write dirty map
Message-Id: <20070914170733.dbe89493.akpm@linux-foundation.org>
In-Reply-To: <a781481a0709141647q3d019423s388c64bf6bed871a@mail.gmail.com>
References: <469D3342.3080405@google.com>
	<46E741B1.4030100@google.com>
	<46E742A2.9040006@google.com>
	<20070914161536.3ec5c533.akpm@linux-foundation.org>
	<a781481a0709141647q3d019423s388c64bf6bed871a@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1985
Lines: 43

On Sat, 15 Sep 2007 05:17:48 +0530
"Satyam Sharma" <satyam.sharma@gmail.com> wrote:

> > It's unobvious why the break point is at MAX_NUMNODES = BITS_PER_LONG and
> > we might want to tweak that in the future.  Yet another argument for
> > centralising this comparison.
> 
> Looks like just an optimization to me ... Ethan wants to economize and not bloat
> struct address_space too much.
> 
> So, if sizeof(nodemask_t) == sizeof(long), i.e. when:
> MAX_NUMNODES <= BITS_PER_LONG, then we'll be adding only sizeof(long)
> extra bytes to the struct (by plonking the object itself into it).
> 
> But even when MAX_NUMNODES > BITS_PER_LONG, because we're storing
> a pointer, and because sizeof(void *) == sizeof(long), so again the maximum
> bloat addition to struct address_space would only be sizeof(long) bytes.

yup.

Note that "It's unobvious" != "It's unobvious to me".  I review code for
understandability-by-others, not for understandability-by-me.

> I didn't see the original mail, but if the #ifdeffery for this
> conditional is too much
> as a result of this optimization, Ethan should probably just do away
> with all of it
> entirely, and simply put a full nodemask_t object (irrespective of MAX_NUMNODES)
> into the struct. After all, struct task_struct does the same unconditionally ...
> but admittedly, there are several times more address_space struct's resident in
> memory at any given time than there are task_struct's, so this optimization does
> make sense too ...

I think the optimisation is (probably) desirable, but it would be best to
describe the tradeoff in the changelog and to add some suitable
code-commentary for those who read the code in a year's time and to avoid
sprinkling the logic all over the tree.

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/