On 9/15/07, Andrew Morton <[EMAIL PROTECTED]> wrote: > On Sat, 15 Sep 2007 05:17:48 +0530 > "Satyam Sharma" <[EMAIL PROTECTED]> 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 [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
