linux-kernel - Re: [PATCH 1/6] cpuset write dirty map

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]

Date:	Sat, 15 Sep 2007 05:46:07 +0530
From:	"Satyam Sharma" <satyam.sharma@...il.com>
To:	"Andrew Morton" <akpm@...ux-foundation.org>
Cc:	"Ethan Solomita" <solo@...gle.com>, linux-mm@...ck.org,
	LKML <linux-kernel@...r.kernel.org>,
	"Christoph Lameter" <clameter@....com>
Subject: Re: [PATCH 1/6] cpuset write dirty map

On 9/15/07, Andrew Morton <akpm@...ux-foundation.org> wrote:
> On Sat, 15 Sep 2007 05:17:48 +0530
> "Satyam Sharma" <satyam.sharma@...il.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.

True, the other option could be to put the /pointer/ in there unconditionally,
but that would slow down the MAX_NUMNODES <= BITS_PER_LONG case,
which (after grepping through defconfigs) appears to be the common case on
all archs other than ia64. So I think your idea of making that conditional
centralized in the code with an accompanying comment is the way to go here ...


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