lkml.org 
[lkml]   [2008]   [Jan]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 1/5] x86: Change size of node ids from u8 to u16 fixup
On Sat, 19 Jan 2008, Mike Travis wrote:

> >> Excuse my ignorance but why wouldn't this work:
> >>
> >> static numanode_t pxm_to_node_map[MAX_PXM_DOMAINS]
> >> = { [0 ... MAX_PXM_DOMAINS - 1] = NUMA_NO_NODE };
> >> ...
> >>>> int acpi_map_pxm_to_node(int pxm)
> >>>> {
> >>> int node = pxm_to_node_map[pxm];
> >>>
> >>> if (node < 0)
> >> numanode_t node = pxm_to_node_map[pxm];
> >>
> >
> > Because NUMA_NO_NODE is 0xff on x86. That's a valid node id for
> > configurations with CONFIG_NODES_SHIFT equal to or greater than 8.
>
> Perhaps numanode_t should be set to u16 if MAX_NUMNODES > 255 to
> allow for an invalid value of 255?
>

Throughout the NUMA code you need a way to distinguish between an invalid
mapping and an actual node id. NID_INVAL is used to say there are no
additional node ids available for this system, the pxm-to-node mapping
doesn't yet exist, etc.

You can't get away with using a magic positive integer with those
semantics because CONFIG_NODES_SHIFT determines MAX_NUMNODES. All
nodemasks will have that many bits in their struct. So 255 will always be
a valid node id for shifts of 8 or larger. It isn't feasible to say for
these types of systems (or ia64 where the default shift is 10) that 255 is
some magic node id that means its invalid.

NID_INVAL is the only way to signify an invalid node id and that has
always been done with -1. So objects that store node ids that have the
possibility of being invalid must be signed. The only time you can use
unsigned objects are when you are guaranteed to have valid node ids.

> > Additionally, Linux has always discouraged typedefs when they do not
> > define an architecture-specific size. The savings from your patch for
> > CONFIG_NODES_SHIFT == 7 would be 256 bytes for this mapping.
> >
> > It's simply not worth it.
>
> So are you saying that I should just use u16 for all node ids whether
> CONFIG_NODES_SHIFT > 7 or not? Othersise, I would think that defining a
> typedef is a fairly clean solution.
>

You're assuming that CONFIG_NODES_SHIFT will never been larger than that
and you still wouldn't be able to use your new numanode_t for anything
that could return NID_INVAL.

> A quick grep shows that there are 35 arrays defined by MAX_NUMNODES in
> x86_64, 38 in X86_32 (not verified.) So it's not exactly a trivial
> amount of memory.
>

You're spinning the argument. Most of those arrays are not simply
returning node ids; most are returning structs or are arrays of unsigned
long type that return addresses. Those are unaffected by your change.
Others are initdata and is freed after boot anyway.

The handful of arrays thoughout the source that return node ids and are
not initdata would only save MAX_NUMNODES number of bytes with your
change for 8-bytes instead of 16-bytes. You're right, you might save a
KB of memory with the change. It's not worth it, especially since they
need to be signed anyway.

David


\
 
 \ /
  Last update: 2008-01-20 07:25    [from the cache]
©2003-2014 Jasper Spaans. Advertise on this site