topology api confusion

[Nathan Lynch]

We need some clarity on how asm-generic/topology.h is intended to be
used.  I suspect that it's supposed to be unconditionally included at
the end of the architecture's topology.h so that any elements which
are undefined by the arch have sensible default definitions.  Looking
at 2.6.13-rc3, this is what ppc64, ia64, and x86_64 currently do,
however i386 does not (i386 pulls in the generic version only when
!CONFIG_NUMA).

The #ifndef guards around each element of the topology api
cannot serve their apparent intended purpose when the architecture
implements a given bit as a function instead of a macro
(e.g. cpu_to_node in ppc64):

----

asm-generic/topology.h:

#ifndef cpu_to_node
#define cpu_to_node(cpu)        (0)
#endif

----

asm-ppc64/topology.h:

static inline int cpu_to_node(int cpu)
{
        int node;

        node = numa_cpu_lookup_table[cpu];
	....

----

Since ppc64 unconditionally includes asm-generic/topology.h, all uses
of cpu_to_node are preprocessed to (0).  Similar damage occurs with
every other topology function which happens to be a real function
instead of a macro.  I'm surprised my ppc64 numa systems even boot ;)

If the intent is that the architecture is free to define only a subset
of the api and include the generic header for fallback definitions,
then we need to do the #ifndef __HAVE_ARCH_FOO thing, no?  That is,
the code above would look like:

----

asm-generic/topology.h:

#ifndef __HAVE_ARCH_CPU_TO_NODE
#define cpu_to_node(cpu)        (0)
#endif

----

asm-ppc64/topology.h:

#define __HAVE_ARCH_CPU_TO_NODE
static inline int cpu_to_node(int cpu)
{
        int node;

        node = numa_cpu_lookup_table[cpu];
	....

----

Thought I'd ask for input first since this would involve a sweep of
include/asm-*.


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