[PATCH 1/1] x86: Add check for node passed to node_to_cpumask - Kernel

This is a discussion on [PATCH 1/1] x86: Add check for node passed to node_to_cpumask - Kernel ; Subject: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask * When CONFIG_DEBUG_PER_CPU_MAPS is set, the node passed to node_to_cpumask and node_to_cpumask_ptr should be validated. Based on "Thu Jun 26 09:23:55 PDT 2008" tip/master... ;-) Signed-off-by: Mike Travis --- ...

+ Reply to Thread
Page 1 of 2 1 2 LastLast
Results 1 to 20 of 21

Thread: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask

  1. [PATCH 1/1] x86: Add check for node passed to node_to_cpumask

    Subject: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask

    * When CONFIG_DEBUG_PER_CPU_MAPS is set, the node passed to
    node_to_cpumask and node_to_cpumask_ptr should be validated.

    Based on "Thu Jun 26 09:23:55 PDT 2008" tip/master... ;-)

    Signed-off-by: Mike Travis
    ---
    arch/x86/kernel/setup.c | 26 +++++++++++++++++++++++---
    include/asm-x86/topology.h | 7 ++++++-
    2 files changed, 29 insertions(+), 4 deletions(-)

    --- linux-2.6.tip.orig/arch/x86/kernel/setup.c
    +++ linux-2.6.tip/arch/x86/kernel/setup.c
    @@ -377,6 +377,10 @@ int early_cpu_to_node(int cpu)
    return per_cpu(x86_cpu_to_node_map, cpu);
    }

    +
    +/* empty cpumask */
    +static const cpumask_t cpu_mask_none;
    +
    /*
    * Returns a pointer to the bitmask of CPUs on Node 'node'.
    */
    @@ -389,13 +393,23 @@ cpumask_t *_node_to_cpumask_ptr(int node
    dump_stack();
    return &cpu_online_map;
    }
    - BUG_ON(node >= nr_node_ids);
    - return &node_to_cpumask_map[node];
    + if (node >= nr_node_ids) {
    + printk(KERN_WARNING
    + "_node_to_cpumask_ptr(%d): node > nr_node_ids(%d)\n",
    + node, nr_node_ids);
    + dump_stack();
    + return &cpu_mask_none;
    + }
    + return (cpumask_t *)&node_to_cpumask_map[node];
    }
    EXPORT_SYMBOL(_node_to_cpumask_ptr);

    /*
    * Returns a bitmask of CPUs on Node 'node'.
    + *
    + * Side note: this function creates the returned cpumask on the stack
    + * so with a high NR_CPUS count, excessive stack space is used. The
    + * node_to_cpumask_ptr function should be used whenever possible.
    */
    cpumask_t node_to_cpumask(int node)
    {
    @@ -405,7 +419,13 @@ cpumask_t node_to_cpumask(int node)
    dump_stack();
    return cpu_online_map;
    }
    - BUG_ON(node >= nr_node_ids);
    + if (node >= nr_node_ids) {
    + printk(KERN_WARNING
    + "node_to_cpumask(%d): node > nr_node_ids(%d)\n",
    + node, nr_node_ids);
    + dump_stack();
    + return cpu_mask_none;
    + }
    return node_to_cpumask_map[node];
    }
    EXPORT_SYMBOL(node_to_cpumask);
    --- linux-2.6.tip.orig/include/asm-x86/topology.h
    +++ linux-2.6.tip/include/asm-x86/topology.h
    @@ -57,7 +57,12 @@ static inline int cpu_to_node(int cpu)
    }
    #define early_cpu_to_node(cpu) cpu_to_node(cpu)

    -/* Returns a bitmask of CPUs on Node 'node'. */
    +/* Returns a bitmask of CPUs on Node 'node'.
    + *
    + * Side note: this function creates the returned cpumask on the stack
    + * so with a high NR_CPUS count, excessive stack space is used. The
    + * node_to_cpumask_ptr function should be used whenever possible.
    + */
    static inline cpumask_t node_to_cpumask(int node)
    {
    return node_to_cpumask_map[node];
    --
    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/

  2. Re: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V2

    Subject: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V2

    V2: Slightly different version to remove a compiler warning.

    * When CONFIG_DEBUG_PER_CPU_MAPS is set, the node passed to
    node_to_cpumask and node_to_cpumask_ptr should be validated.

    Based on "Thu Jun 26 09:23:55 PDT 2008" tip/master... ;-)

    Signed-off-by: Mike Travis
    ---
    arch/x86/kernel/setup.c | 26 +++++++++++++++++++++++---
    include/asm-x86/topology.h | 7 ++++++-
    2 files changed, 29 insertions(+), 4 deletions(-)

    --- linux-2.6.tip.orig/arch/x86/kernel/setup.c
    +++ linux-2.6.tip/arch/x86/kernel/setup.c
    @@ -377,6 +377,10 @@ int early_cpu_to_node(int cpu)
    return per_cpu(x86_cpu_to_node_map, cpu);
    }

    +
    +/* empty cpumask */
    +static const cpumask_t cpu_mask_none;
    +
    /*
    * Returns a pointer to the bitmask of CPUs on Node 'node'.
    */
    @@ -389,13 +393,23 @@ cpumask_t *_node_to_cpumask_ptr(int node
    dump_stack();
    return &cpu_online_map;
    }
    - BUG_ON(node >= nr_node_ids);
    - return &node_to_cpumask_map[node];
    + if (node >= nr_node_ids) {
    + printk(KERN_WARNING
    + "_node_to_cpumask_ptr(%d): node > nr_node_ids(%d)\n",
    + node, nr_node_ids);
    + dump_stack();
    + return (cpumask_t *)&cpu_mask_none;
    + }
    + return (cpumask_t *)&node_to_cpumask_map[node];
    }
    EXPORT_SYMBOL(_node_to_cpumask_ptr);

    /*
    * Returns a bitmask of CPUs on Node 'node'.
    + *
    + * Side note: this function creates the returned cpumask on the stack
    + * so with a high NR_CPUS count, excessive stack space is used. The
    + * node_to_cpumask_ptr function should be used whenever possible.
    */
    cpumask_t node_to_cpumask(int node)
    {
    @@ -405,7 +419,13 @@ cpumask_t node_to_cpumask(int node)
    dump_stack();
    return cpu_online_map;
    }
    - BUG_ON(node >= nr_node_ids);
    + if (node >= nr_node_ids) {
    + printk(KERN_WARNING
    + "node_to_cpumask(%d): node > nr_node_ids(%d)\n",
    + node, nr_node_ids);
    + dump_stack();
    + return cpu_mask_none;
    + }
    return node_to_cpumask_map[node];
    }
    EXPORT_SYMBOL(node_to_cpumask);
    --- linux-2.6.tip.orig/include/asm-x86/topology.h
    +++ linux-2.6.tip/include/asm-x86/topology.h
    @@ -57,7 +57,12 @@ static inline int cpu_to_node(int cpu)
    }
    #define early_cpu_to_node(cpu) cpu_to_node(cpu)

    -/* Returns a bitmask of CPUs on Node 'node'. */
    +/* Returns a bitmask of CPUs on Node 'node'.
    + *
    + * Side note: this function creates the returned cpumask on the stack
    + * so with a high NR_CPUS count, excessive stack space is used. The
    + * node_to_cpumask_ptr function should be used whenever possible.
    + */
    static inline cpumask_t node_to_cpumask(int node)
    {
    return node_to_cpumask_map[node];

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

  3. Re: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3

    Subject: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3

    * When CONFIG_DEBUG_PER_CPU_MAPS is set, the node passed to
    node_to_cpumask and node_to_cpumask_ptr should be validated.
    If invalid, then a dump_stack is performed and a zero cpumask
    is returned.

    Based on "Fri Jun 27 10:06:06 PDT 2008" tip/master... ;-)

    Signed-off-by: Mike Travis
    ---
    V2: Slightly different version to remove a compiler warning.
    V3: Redone to reflect moving setup.c -> setup_percpu.c
    ---
    arch/x86/kernel/setup_percpu.c | 26 +++++++++++++++++++++++---
    include/asm-x86/topology.h | 7 ++++++-
    2 files changed, 29 insertions(+), 4 deletions(-)

    --- linux-2.6.tip.orig/arch/x86/kernel/setup_percpu.c
    +++ linux-2.6.tip/arch/x86/kernel/setup_percpu.c
    @@ -346,6 +346,10 @@ int early_cpu_to_node(int cpu)
    return per_cpu(x86_cpu_to_node_map, cpu);
    }

    +
    +/* empty cpumask */
    +static const cpumask_t cpu_mask_none;
    +
    /*
    * Returns a pointer to the bitmask of CPUs on Node 'node'.
    */
    @@ -358,13 +362,23 @@ cpumask_t *_node_to_cpumask_ptr(int node
    dump_stack();
    return &cpu_online_map;
    }
    - BUG_ON(node >= nr_node_ids);
    - return &node_to_cpumask_map[node];
    + if (node >= nr_node_ids) {
    + printk(KERN_WARNING
    + "_node_to_cpumask_ptr(%d): node > nr_node_ids(%d)\n",
    + node, nr_node_ids);
    + dump_stack();
    + return (cpumask_t *)&cpu_mask_none;
    + }
    + return (cpumask_t *)&node_to_cpumask_map[node];
    }
    EXPORT_SYMBOL(_node_to_cpumask_ptr);

    /*
    * Returns a bitmask of CPUs on Node 'node'.
    + *
    + * Side note: this function creates the returned cpumask on the stack
    + * so with a high NR_CPUS count, excessive stack space is used. The
    + * node_to_cpumask_ptr function should be used whenever possible.
    */
    cpumask_t node_to_cpumask(int node)
    {
    @@ -374,7 +388,13 @@ cpumask_t node_to_cpumask(int node)
    dump_stack();
    return cpu_online_map;
    }
    - BUG_ON(node >= nr_node_ids);
    + if (node >= nr_node_ids) {
    + printk(KERN_WARNING
    + "node_to_cpumask(%d): node > nr_node_ids(%d)\n",
    + node, nr_node_ids);
    + dump_stack();
    + return cpu_mask_none;
    + }
    return node_to_cpumask_map[node];
    }
    EXPORT_SYMBOL(node_to_cpumask);
    --- linux-2.6.tip.orig/include/asm-x86/topology.h
    +++ linux-2.6.tip/include/asm-x86/topology.h
    @@ -57,7 +57,12 @@ static inline int cpu_to_node(int cpu)
    }
    #define early_cpu_to_node(cpu) cpu_to_node(cpu)

    -/* Returns a bitmask of CPUs on Node 'node'. */
    +/* Returns a bitmask of CPUs on Node 'node'.
    + *
    + * Side note: this function creates the returned cpumask on the stack
    + * so with a high NR_CPUS count, excessive stack space is used. The
    + * node_to_cpumask_ptr function should be used whenever possible.
    + */
    static inline cpumask_t node_to_cpumask(int node)
    {
    return node_to_cpumask_map[node];

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

  4. Re: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3

    On Fri, Jun 27, 2008 at 7:10 PM, Mike Travis wrote:
    > Subject: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3
    >
    > * When CONFIG_DEBUG_PER_CPU_MAPS is set, the node passed to
    > node_to_cpumask and node_to_cpumask_ptr should be validated.
    > If invalid, then a dump_stack is performed and a zero cpumask
    > is returned.
    >
    > Based on "Fri Jun 27 10:06:06 PDT 2008" tip/master... ;-)
    >
    > Signed-off-by: Mike Travis
    > ---
    > V2: Slightly different version to remove a compiler warning.
    > V3: Redone to reflect moving setup.c -> setup_percpu.c
    > ---
    > arch/x86/kernel/setup_percpu.c | 26 +++++++++++++++++++++++---
    > include/asm-x86/topology.h | 7 ++++++-
    > 2 files changed, 29 insertions(+), 4 deletions(-)
    >
    > --- linux-2.6.tip.orig/arch/x86/kernel/setup_percpu.c
    > +++ linux-2.6.tip/arch/x86/kernel/setup_percpu.c
    > @@ -346,6 +346,10 @@ int early_cpu_to_node(int cpu)
    > return per_cpu(x86_cpu_to_node_map, cpu);
    > }
    >
    > +
    > +/* empty cpumask */
    > +static const cpumask_t cpu_mask_none;
    > +
    > /*
    > * Returns a pointer to the bitmask of CPUs on Node 'node'.
    > */
    > @@ -358,13 +362,23 @@ cpumask_t *_node_to_cpumask_ptr(int node
    > dump_stack();
    > return &cpu_online_map;
    > }
    > - BUG_ON(node >= nr_node_ids);
    > - return &node_to_cpumask_map[node];
    > + if (node >= nr_node_ids) {
    > + printk(KERN_WARNING
    > + "_node_to_cpumask_ptr(%d): node > nr_node_ids(%d)\n",
    > + node, nr_node_ids);
    > + dump_stack();
    > + return (cpumask_t *)&cpu_mask_none;


    Hm, I am wondering about this.

    There exists an option DEBUG_RODATA ("Write protect kernel read-only
    data structures"). I'm guessing this RO protections means that we'll
    get page faults (and thus BUG/panic) if this "none" mask is ever
    attempted to be changed.

    So if CONFIG_DEBUG_RODATA=y, I believe we'll see first this warning,
    then a panic (after all). Would it be better to make cpu_mask_none
    non-const, in spirit of trying to continue as far as possible? I don't
    really know if it matters, though. It seems that fedora kernels at
    least ship with a default of DEBUG_RODATA=y.

    What you could also do is to make it non-const and then zero it each
    time it is requested. Again, this is an error situation in either
    case, so maybe it's not worth fussing about.


    Vegard

    --
    "The animistic metaphor of the bug that maliciously sneaked in while
    the programmer was not looking is intellectually dishonest as it
    disguises that the error is the programmer's own creation."
    -- E. W. Dijkstra, EWD1036
    --
    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/

  5. Re: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3

    Vegard Nossum wrote:
    > On Fri, Jun 27, 2008 at 7:10 PM, Mike Travis wrote:
    >> Subject: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3
    >>
    >> * When CONFIG_DEBUG_PER_CPU_MAPS is set, the node passed to
    >> node_to_cpumask and node_to_cpumask_ptr should be validated.
    >> If invalid, then a dump_stack is performed and a zero cpumask
    >> is returned.
    >>
    >> Based on "Fri Jun 27 10:06:06 PDT 2008" tip/master... ;-)
    >>
    >> Signed-off-by: Mike Travis
    >> ---
    >> V2: Slightly different version to remove a compiler warning.
    >> V3: Redone to reflect moving setup.c -> setup_percpu.c
    >> ---
    >> arch/x86/kernel/setup_percpu.c | 26 +++++++++++++++++++++++---
    >> include/asm-x86/topology.h | 7 ++++++-
    >> 2 files changed, 29 insertions(+), 4 deletions(-)
    >>
    >> --- linux-2.6.tip.orig/arch/x86/kernel/setup_percpu.c
    >> +++ linux-2.6.tip/arch/x86/kernel/setup_percpu.c
    >> @@ -346,6 +346,10 @@ int early_cpu_to_node(int cpu)
    >> return per_cpu(x86_cpu_to_node_map, cpu);
    >> }
    >>
    >> +
    >> +/* empty cpumask */
    >> +static const cpumask_t cpu_mask_none;
    >> +
    >> /*
    >> * Returns a pointer to the bitmask of CPUs on Node 'node'.
    >> */
    >> @@ -358,13 +362,23 @@ cpumask_t *_node_to_cpumask_ptr(int node
    >> dump_stack();
    >> return &cpu_online_map;
    >> }
    >> - BUG_ON(node >= nr_node_ids);
    >> - return &node_to_cpumask_map[node];
    >> + if (node >= nr_node_ids) {
    >> + printk(KERN_WARNING
    >> + "_node_to_cpumask_ptr(%d): node > nr_node_ids(%d)\n",
    >> + node, nr_node_ids);
    >> + dump_stack();
    >> + return (cpumask_t *)&cpu_mask_none;

    >
    > Hm, I am wondering about this.
    >
    > There exists an option DEBUG_RODATA ("Write protect kernel read-only
    > data structures"). I'm guessing this RO protections means that we'll
    > get page faults (and thus BUG/panic) if this "none" mask is ever
    > attempted to be changed.
    >
    > So if CONFIG_DEBUG_RODATA=y, I believe we'll see first this warning,
    > then a panic (after all). Would it be better to make cpu_mask_none
    > non-const, in spirit of trying to continue as far as possible? I don't
    > really know if it matters, though. It seems that fedora kernels at
    > least ship with a default of DEBUG_RODATA=y.
    >
    > What you could also do is to make it non-const and then zero it each
    > time it is requested. Again, this is an error situation in either
    > case, so maybe it's not worth fussing about.
    >
    >
    > Vegard
    >


    Ingo asked me to add the "const". But it would be a mistake to use
    node_to_cpumask_ptr to modify the map... numa_set/clear_node is the
    proper way to modify the maps. Hmm, maybe I should add "const" to
    the real node_to_cpumask_ptr function, at least then the compiler
    would help spot illegal usages.

    Thanks,
    Mike
    --
    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/

  6. Re: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3

    On Fri, Jun 27, 2008 at 8:03 PM, Mike Travis wrote:
    >> So if CONFIG_DEBUG_RODATA=y, I believe we'll see first this warning,
    >> then a panic (after all). Would it be better to make cpu_mask_none
    >> non-const, in spirit of trying to continue as far as possible? I don't
    >> really know if it matters, though. It seems that fedora kernels at
    >> least ship with a default of DEBUG_RODATA=y.
    >>


    ....

    > Ingo asked me to add the "const". But it would be a mistake to use
    > node_to_cpumask_ptr to modify the map... numa_set/clear_node is the
    > proper way to modify the maps. Hmm, maybe I should add "const" to
    > the real node_to_cpumask_ptr function, at least then the compiler
    > would help spot illegal usages.


    That sounds like a brilliant idea, as long as none of the callers
    expect it to be non-const.

    I changed it to return const to see, and built allmodconfig + a few
    randconfigs without error (well, warning). There are not that many
    users of this function anyway.

    BTW, what's up with the topology.h/setup_percpu.c mismatch, when
    topology.c exists as well?


    Vegard

    --
    "The animistic metaphor of the bug that maliciously sneaked in while
    the programmer was not looking is intellectually dishonest as it
    disguises that the error is the programmer's own creation."
    -- E. W. Dijkstra, EWD1036
    --
    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/

  7. Re: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3

    Vegard Nossum wrote:
    > On Fri, Jun 27, 2008 at 8:03 PM, Mike Travis wrote:
    >>> So if CONFIG_DEBUG_RODATA=y, I believe we'll see first this warning,
    >>> then a panic (after all). Would it be better to make cpu_mask_none
    >>> non-const, in spirit of trying to continue as far as possible? I don't
    >>> really know if it matters, though. It seems that fedora kernels at
    >>> least ship with a default of DEBUG_RODATA=y.
    >>>

    >
    > ...
    >
    >> Ingo asked me to add the "const". But it would be a mistake to use
    >> node_to_cpumask_ptr to modify the map... numa_set/clear_node is the
    >> proper way to modify the maps. Hmm, maybe I should add "const" to
    >> the real node_to_cpumask_ptr function, at least then the compiler
    >> would help spot illegal usages.

    >
    > That sounds like a brilliant idea, as long as none of the callers
    > expect it to be non-const.


    Being that I modified all the callers to use this new function, I did
    verify that each caller expected it to be read only... ;-)
    >
    > I changed it to return const to see, and built allmodconfig + a few
    > randconfigs without error (well, warning). There are not that many
    > users of this function anyway.


    Did any violate the read-only nature of the call?
    >
    > BTW, what's up with the topology.h/setup_percpu.c mismatch, when
    > topology.c exists as well?


    Topology.h supplies the generic usage which is to define a local cpumask
    variable, fill it, and then return a pointer to it. This is for arch's
    that do not maintain a separate node_to_cpumask_map[] but allows a
    compatible call interface.
    >
    >
    > Vegard
    >


    Thanks!
    Mike

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

  8. Re: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3

    On Thu, Jul 3, 2008 at 10:44 AM, Ingo Molnar wrote:
    >
    > * Mike Travis wrote:
    >
    >> Subject: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3
    >>
    >> * When CONFIG_DEBUG_PER_CPU_MAPS is set, the node passed to
    >> node_to_cpumask and node_to_cpumask_ptr should be validated.
    >> If invalid, then a dump_stack is performed and a zero cpumask
    >> is returned.
    >>
    >> Based on "Fri Jun 27 10:06:06 PDT 2008" tip/master... ;-)
    >>
    >> Signed-off-by: Mike Travis
    >> ---
    >> V2: Slightly different version to remove a compiler warning.
    >> V3: Redone to reflect moving setup.c -> setup_percpu.c

    >
    > applied to tip/x86/unify-setup - thanks Mike.
    >
    > Vegard, can i add your Acked-by too?


    To be honest, I'd prefer that the function returns a const pointer.
    Mike and I have both reviewed all callers independently and concluded
    that there is no problem in doing this, and that, in fact, this is the
    correct way to deal with it.

    So if Mike submits a V4 with this const return type, or another patch
    on top of this one, I'll ack it :-)


    Vegard

    --
    "The animistic metaphor of the bug that maliciously sneaked in while
    the programmer was not looking is intellectually dishonest as it
    disguises that the error is the programmer's own creation."
    -- E. W. Dijkstra, EWD1036
    --
    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/

  9. Re: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3


    * Mike Travis wrote:

    > Subject: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3
    >
    > * When CONFIG_DEBUG_PER_CPU_MAPS is set, the node passed to
    > node_to_cpumask and node_to_cpumask_ptr should be validated.
    > If invalid, then a dump_stack is performed and a zero cpumask
    > is returned.
    >
    > Based on "Fri Jun 27 10:06:06 PDT 2008" tip/master... ;-)
    >
    > Signed-off-by: Mike Travis
    > ---
    > V2: Slightly different version to remove a compiler warning.
    > V3: Redone to reflect moving setup.c -> setup_percpu.c


    applied to tip/x86/unify-setup - thanks Mike.

    Vegard, can i add your Acked-by too?

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

  10. Re: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3


    * Vegard Nossum wrote:

    > On Thu, Jul 3, 2008 at 10:44 AM, Ingo Molnar wrote:
    > >
    > > * Mike Travis wrote:
    > >
    > >> Subject: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3
    > >>
    > >> * When CONFIG_DEBUG_PER_CPU_MAPS is set, the node passed to
    > >> node_to_cpumask and node_to_cpumask_ptr should be validated.
    > >> If invalid, then a dump_stack is performed and a zero cpumask
    > >> is returned.
    > >>
    > >> Based on "Fri Jun 27 10:06:06 PDT 2008" tip/master... ;-)
    > >>
    > >> Signed-off-by: Mike Travis
    > >> ---
    > >> V2: Slightly different version to remove a compiler warning.
    > >> V3: Redone to reflect moving setup.c -> setup_percpu.c

    > >
    > > applied to tip/x86/unify-setup - thanks Mike.
    > >
    > > Vegard, can i add your Acked-by too?

    >
    > To be honest, I'd prefer that the function returns a const pointer.
    > Mike and I have both reviewed all callers independently and concluded
    > that there is no problem in doing this, and that, in fact, this is the
    > correct way to deal with it.
    >
    > So if Mike submits a V4 with this const return type, or another patch
    > on top of this one, I'll ack it :-)


    ok, i'll wait for v4

    (v3 is applied already so Mike please send a delta to v3.)

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

  11. Re: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3

    Vegard Nossum wrote:
    > On Thu, Jul 3, 2008 at 10:44 AM, Ingo Molnar wrote:
    >> * Mike Travis wrote:
    >>
    >>> Subject: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3
    >>>
    >>> * When CONFIG_DEBUG_PER_CPU_MAPS is set, the node passed to
    >>> node_to_cpumask and node_to_cpumask_ptr should be validated.
    >>> If invalid, then a dump_stack is performed and a zero cpumask
    >>> is returned.
    >>>
    >>> Based on "Fri Jun 27 10:06:06 PDT 2008" tip/master... ;-)
    >>>
    >>> Signed-off-by: Mike Travis
    >>> ---
    >>> V2: Slightly different version to remove a compiler warning.
    >>> V3: Redone to reflect moving setup.c -> setup_percpu.c

    >> applied to tip/x86/unify-setup - thanks Mike.
    >>
    >> Vegard, can i add your Acked-by too?

    >
    > To be honest, I'd prefer that the function returns a const pointer.
    > Mike and I have both reviewed all callers independently and concluded
    > that there is no problem in doing this, and that, in fact, this is the
    > correct way to deal with it.
    >
    > So if Mike submits a V4 with this const return type, or another patch
    > on top of this one, I'll ack it :-)
    >
    >
    > Vegard
    >


    Sure, I can do this...

    Thanks,
    Mike
    --
    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/

  12. [PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr

    Ingo Molnar wrote:
    > * Vegard Nossum wrote:
    >
    >> On Thu, Jul 3, 2008 at 10:44 AM, Ingo Molnar wrote:
    >>> * Mike Travis wrote:
    >>>
    >>>> Subject: [PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3
    >>>>
    >>>> * When CONFIG_DEBUG_PER_CPU_MAPS is set, the node passed to
    >>>> node_to_cpumask and node_to_cpumask_ptr should be validated.
    >>>> If invalid, then a dump_stack is performed and a zero cpumask
    >>>> is returned.
    >>>>
    >>>> Based on "Fri Jun 27 10:06:06 PDT 2008" tip/master... ;-)
    >>>>
    >>>> Signed-off-by: Mike Travis
    >>>> ---
    >>>> V2: Slightly different version to remove a compiler warning.
    >>>> V3: Redone to reflect moving setup.c -> setup_percpu.c
    >>> applied to tip/x86/unify-setup - thanks Mike.
    >>>
    >>> Vegard, can i add your Acked-by too?

    >> To be honest, I'd prefer that the function returns a const pointer.
    >> Mike and I have both reviewed all callers independently and concluded
    >> that there is no problem in doing this, and that, in fact, this is the
    >> correct way to deal with it.
    >>
    >> So if Mike submits a V4 with this const return type, or another patch
    >> on top of this one, I'll ack it :-)

    >
    > ok, i'll wait for v4
    >
    > (v3 is applied already so Mike please send a delta to v3.)
    >
    > Ingo


    Subject: [PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr

    * Strengthen the return type for the _node_to_cpumask_ptr to be
    a const pointer. This adds compiler checking to insure that
    node_to_cpumask_map[] is not changed inadvertently.

    Applies to tip/master with the following patch applied:

    "[PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3"

    Signed-off-by: Mike Travis
    ---
    Note: I did not change node_to_cpumask_ptr() in include/asm-generic/topology.h
    as node_to_cpumask_ptr_next() does change the cpumask value.
    ---
    arch/x86/kernel/setup_percpu.c | 8 ++++----
    include/asm-x86/topology.h | 10 +++++-----
    2 files changed, 9 insertions(+), 9 deletions(-)

    --- linux-2.6.tip.orig/arch/x86/kernel/setup_percpu.c
    +++ linux-2.6.tip/arch/x86/kernel/setup_percpu.c
    @@ -353,23 +353,23 @@ static const cpumask_t cpu_mask_none;
    /*
    * Returns a pointer to the bitmask of CPUs on Node 'node'.
    */
    -cpumask_t *_node_to_cpumask_ptr(int node)
    +const cpumask_t *_node_to_cpumask_ptr(int node)
    {
    if (node_to_cpumask_map == NULL) {
    printk(KERN_WARNING
    "_node_to_cpumask_ptr(%d): no node_to_cpumask_map!\n",
    node);
    dump_stack();
    - return &cpu_online_map;
    + return (const cpumask_t *)&cpu_online_map;
    }
    if (node >= nr_node_ids) {
    printk(KERN_WARNING
    "_node_to_cpumask_ptr(%d): node > nr_node_ids(%d)\n",
    node, nr_node_ids);
    dump_stack();
    - return (cpumask_t *)&cpu_mask_none;
    + return &cpu_mask_none;
    }
    - return (cpumask_t *)&node_to_cpumask_map[node];
    + return &node_to_cpumask_map[node];
    }
    EXPORT_SYMBOL(_node_to_cpumask_ptr);

    --- linux-2.6.tip.orig/include/asm-x86/topology.h
    +++ linux-2.6.tip/include/asm-x86/topology.h
    @@ -82,7 +82,7 @@ DECLARE_EARLY_PER_CPU(int, x86_cpu_to_no
    #ifdef CONFIG_DEBUG_PER_CPU_MAPS
    extern int cpu_to_node(int cpu);
    extern int early_cpu_to_node(int cpu);
    -extern cpumask_t *_node_to_cpumask_ptr(int node);
    +extern const cpumask_t *_node_to_cpumask_ptr(int node);
    extern cpumask_t node_to_cpumask(int node);

    #else /* !CONFIG_DEBUG_PER_CPU_MAPS */
    @@ -103,7 +103,7 @@ static inline int early_cpu_to_node(int
    }

    /* Returns a pointer to the cpumask of CPUs on Node 'node'. */
    -static inline cpumask_t *_node_to_cpumask_ptr(int node)
    +static inline const cpumask_t *_node_to_cpumask_ptr(int node)
    {
    return &node_to_cpumask_map[node];
    }
    @@ -118,7 +118,7 @@ static inline cpumask_t node_to_cpumask(

    /* Replace default node_to_cpumask_ptr with optimized version */
    #define node_to_cpumask_ptr(v, node) \
    - cpumask_t *v = _node_to_cpumask_ptr(node)
    + const cpumask_t *v = _node_to_cpumask_ptr(node)

    #define node_to_cpumask_ptr_next(v, node) \
    v = _node_to_cpumask_ptr(node)
    @@ -186,7 +186,7 @@ extern int __node_distance(int, int);
    #define cpu_to_node(cpu) 0
    #define early_cpu_to_node(cpu) 0

    -static inline cpumask_t *_node_to_cpumask_ptr(int node)
    +static inline const cpumask_t *_node_to_cpumask_ptr(int node)
    {
    return &cpu_online_map;
    }
    @@ -201,7 +201,7 @@ static inline int node_to_first_cpu(int

    /* Replace default node_to_cpumask_ptr with optimized version */
    #define node_to_cpumask_ptr(v, node) \
    - cpumask_t *v = _node_to_cpumask_ptr(node)
    + const cpumask_t *v = _node_to_cpumask_ptr(node)

    #define node_to_cpumask_ptr_next(v, node) \
    v = _node_to_cpumask_ptr(node)
    --
    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/

  13. Re: [PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr

    Hi,

    On Tue, Jul 8, 2008 at 7:06 PM, Mike Travis wrote:
    >> (v3 is applied already so Mike please send a delta to v3.)
    >>
    >> Ingo

    >
    > Subject: [PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr
    >
    > * Strengthen the return type for the _node_to_cpumask_ptr to be
    > a const pointer. This adds compiler checking to insure that
    > node_to_cpumask_map[] is not changed inadvertently.
    >
    > Applies to tip/master with the following patch applied:
    >
    > "[PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3"
    >
    > Signed-off-by: Mike Travis
    > ---
    > Note: I did not change node_to_cpumask_ptr() in include/asm-generic/topology.h
    > as node_to_cpumask_ptr_next() does change the cpumask value.


    Hmmm. Does it really?

    #define node_to_cpumask_ptr_next(v, node) \
    _##v = node_to_cpumask(node)

    This doesn't seem to modify it?

    Also, isn't it unfortunate to have the same function return
    const/non-const depending on your arch/config?


    Vegard

    --
    "The animistic metaphor of the bug that maliciously sneaked in while
    the programmer was not looking is intellectually dishonest as it
    disguises that the error is the programmer's own creation."
    -- E. W. Dijkstra, EWD1036
    --
    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/

  14. Re: [PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr

    Vegard Nossum wrote:
    > Hi,
    >
    > On Tue, Jul 8, 2008 at 7:06 PM, Mike Travis wrote:
    >>> (v3 is applied already so Mike please send a delta to v3.)
    >>>
    >>> Ingo

    >> Subject: [PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr
    >>
    >> * Strengthen the return type for the _node_to_cpumask_ptr to be
    >> a const pointer. This adds compiler checking to insure that
    >> node_to_cpumask_map[] is not changed inadvertently.
    >>
    >> Applies to tip/master with the following patch applied:
    >>
    >> "[PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3"
    >>
    >> Signed-off-by: Mike Travis
    >> ---
    >> Note: I did not change node_to_cpumask_ptr() in include/asm-generic/topology.h
    >> as node_to_cpumask_ptr_next() does change the cpumask value.

    >
    > Hmmm. Does it really?
    >
    > #define node_to_cpumask_ptr_next(v, node) \
    > _##v = node_to_cpumask(node)
    >
    > This doesn't seem to modify it?


    Well I thought about it. The pointer (*v) does not change
    but the underlying cpumask variable is updated with the
    cpumask for the (supposedly) new node number. You can see
    that in this code snippet from kernel/sched.c:

    for (i = 1; i < SD_NODES_PER_DOMAIN; i++) {
    int next_node = find_next_best_node(node, &used_nodes);

    node_to_cpumask_ptr_next(nodemask, next_node);
    cpus_or(*span, *span, *nodemask);
    }

    In the optimized (x86_64) case, the pointer is simply modified
    to point to the new node_to_cpumask_map[node] entry. It remains
    a pointer to a const value.

    But the non-optimized version replaces the const cpumask value
    with the new cpumask value. Isn't this breaking the const
    attribute?

    >
    > Also, isn't it unfortunate to have the same function return
    > const/non-const depending on your arch/config?


    But isn't that exactly what it does? (And in reality, the real
    protection happens when there is a node_to_cpumask_map[] present.)

    But whichever seems more correct is fine with me... ;-)

    Thanks,
    Mike
    --
    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/

  15. Re: [PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr

    On Tue, Jul 8, 2008 at 8:05 PM, Mike Travis wrote:
    >>> Note: I did not change node_to_cpumask_ptr() in include/asm-generic/topology.h
    >>> as node_to_cpumask_ptr_next() does change the cpumask value.

    >>
    >> Hmmm. Does it really?
    >>
    >> #define node_to_cpumask_ptr_next(v, node) \
    >> _##v = node_to_cpumask(node)
    >>
    >> This doesn't seem to modify it?

    >
    > Well I thought about it. The pointer (*v) does not change
    > but the underlying cpumask variable is updated with the
    > cpumask for the (supposedly) new node number. You can see
    > that in this code snippet from kernel/sched.c:
    >
    > for (i = 1; i < SD_NODES_PER_DOMAIN; i++) {
    > int next_node = find_next_best_node(node, &used_nodes);
    >
    > node_to_cpumask_ptr_next(nodemask, next_node);
    > cpus_or(*span, *span, *nodemask);
    > }
    >
    > In the optimized (x86_64) case, the pointer is simply modified
    > to point to the new node_to_cpumask_map[node] entry. It remains
    > a pointer to a const value.
    >
    > But the non-optimized version replaces the const cpumask value
    > with the new cpumask value. Isn't this breaking the const
    > attribute?


    No, I think the pointer really should be const. This doesn't guarantee
    that the value doesn't change behind our backs, it only prevents us
    from modifying it ourselves.


    Vegard

    --
    "The animistic metaphor of the bug that maliciously sneaked in while
    the programmer was not looking is intellectually dishonest as it
    disguises that the error is the programmer's own creation."
    -- E. W. Dijkstra, EWD1036
    --
    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/

  16. Re: [PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr

    Vegard Nossum wrote:
    > On Tue, Jul 8, 2008 at 8:05 PM, Mike Travis wrote:
    >>>> Note: I did not change node_to_cpumask_ptr() in include/asm-generic/topology.h
    >>>> as node_to_cpumask_ptr_next() does change the cpumask value.
    >>> Hmmm. Does it really?
    >>>
    >>> #define node_to_cpumask_ptr_next(v, node) \
    >>> _##v = node_to_cpumask(node)
    >>>
    >>> This doesn't seem to modify it?

    >> Well I thought about it. The pointer (*v) does not change
    >> but the underlying cpumask variable is updated with the
    >> cpumask for the (supposedly) new node number. You can see
    >> that in this code snippet from kernel/sched.c:
    >>
    >> for (i = 1; i < SD_NODES_PER_DOMAIN; i++) {
    >> int next_node = find_next_best_node(node, &used_nodes);
    >>
    >> node_to_cpumask_ptr_next(nodemask, next_node);
    >> cpus_or(*span, *span, *nodemask);
    >> }
    >>
    >> In the optimized (x86_64) case, the pointer is simply modified
    >> to point to the new node_to_cpumask_map[node] entry. It remains
    >> a pointer to a const value.
    >>
    >> But the non-optimized version replaces the const cpumask value
    >> with the new cpumask value. Isn't this breaking the const
    >> attribute?

    >
    > No, I think the pointer really should be const. This doesn't guarantee
    > that the value doesn't change behind our backs, it only prevents us
    > from modifying it ourselves.
    >
    >
    > Vegard
    >


    Is this what you had in mind:


    --- linux-2.6.tip.orig/include/asm-generic/topology.h
    +++ linux-2.6.tip/include/asm-generic/topology.h
    @@ -60,7 +60,7 @@
    #ifndef node_to_cpumask_ptr

    #define node_to_cpumask_ptr(v, node) \
    - cpumask_t _##v = node_to_cpumask(node), *v = &_##v
    + const cpumask_t _##v = node_to_cpumask(node), *v = &_##v

    #define node_to_cpumask_ptr_next(v, node) \
    _##v = node_to_cpumask(node)


    (It's taking a while as now I need to do some cross-compile testing.)

    Thanks,
    Mike
    --
    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/

  17. Re: [PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr

    On Tue, Jul 8, 2008 at 10:51 PM, Mike Travis wrote:
    > Vegard Nossum wrote:
    >> On Tue, Jul 8, 2008 at 8:05 PM, Mike Travis wrote:
    >>>>> Note: I did not change node_to_cpumask_ptr() in include/asm-generic/topology.h
    >>>>> as node_to_cpumask_ptr_next() does change the cpumask value.
    >>>> Hmmm. Does it really?
    >>>>
    >>>> #define node_to_cpumask_ptr_next(v, node) \
    >>>> _##v = node_to_cpumask(node)
    >>>>
    >>>> This doesn't seem to modify it?
    >>> Well I thought about it. The pointer (*v) does not change
    >>> but the underlying cpumask variable is updated with the
    >>> cpumask for the (supposedly) new node number. You can see
    >>> that in this code snippet from kernel/sched.c:
    >>>
    >>> for (i = 1; i < SD_NODES_PER_DOMAIN; i++) {
    >>> int next_node = find_next_best_node(node, &used_nodes);
    >>>
    >>> node_to_cpumask_ptr_next(nodemask, next_node);
    >>> cpus_or(*span, *span, *nodemask);
    >>> }
    >>>
    >>> In the optimized (x86_64) case, the pointer is simply modified
    >>> to point to the new node_to_cpumask_map[node] entry. It remains
    >>> a pointer to a const value.
    >>>
    >>> But the non-optimized version replaces the const cpumask value
    >>> with the new cpumask value. Isn't this breaking the const
    >>> attribute?

    >>
    >> No, I think the pointer really should be const. This doesn't guarantee
    >> that the value doesn't change behind our backs, it only prevents us
    >> from modifying it ourselves.
    >>
    >>
    >> Vegard
    >>

    >
    > Is this what you had in mind:
    >
    >
    > --- linux-2.6.tip.orig/include/asm-generic/topology.h
    > +++ linux-2.6.tip/include/asm-generic/topology.h
    > @@ -60,7 +60,7 @@
    > #ifndef node_to_cpumask_ptr
    >
    > #define node_to_cpumask_ptr(v, node) \
    > - cpumask_t _##v = node_to_cpumask(node), *v = &_##v
    > + const cpumask_t _##v = node_to_cpumask(node), *v = &_##v
    >
    > #define node_to_cpumask_ptr_next(v, node) \
    > _##v = node_to_cpumask(node)
    >
    >
    > (It's taking a while as now I need to do some cross-compile testing.)


    Actually, no.

    We don't want the _##v to be const, do we? What do you think about
    this? (Watch out for whitespace munges)

    diff --git a/include/asm-generic/topology.h b/include/asm-generic/topology.h
    index a6aea79..56957f2 100644
    --- a/include/asm-generic/topology.h
    +++ b/include/asm-generic/topology.h
    @@ -60,7 +60,8 @@
    #ifndef node_to_cpumask_ptr

    #define node_to_cpumask_ptr(v, node)
    - cpumask_t _##v = node_to_cpumask(node), *v = &_##v
    + cpumask_t _##v = node_to_cpumask(node); \
    + const cpumask_t *v = &_##v;

    #define node_to_cpumask_ptr_next(v, node) \
    _##v = node_to_cpumask(node)


    Vegard

    --
    "The animistic metaphor of the bug that maliciously sneaked in while
    the programmer was not looking is intellectually dishonest as it
    disguises that the error is the programmer's own creation."
    -- E. W. Dijkstra, EWD1036
    --
    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/

  18. Re: [PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr

    Vegard Nossum wrote:
    > On Tue, Jul 8, 2008 at 10:51 PM, Mike Travis wrote:
    >> Vegard Nossum wrote:
    >>> On Tue, Jul 8, 2008 at 8:05 PM, Mike Travis wrote:
    >>>>>> Note: I did not change node_to_cpumask_ptr() in include/asm-generic/topology.h
    >>>>>> as node_to_cpumask_ptr_next() does change the cpumask value.
    >>>>> Hmmm. Does it really?
    >>>>>
    >>>>> #define node_to_cpumask_ptr_next(v, node) \
    >>>>> _##v = node_to_cpumask(node)
    >>>>>
    >>>>> This doesn't seem to modify it?
    >>>> Well I thought about it. The pointer (*v) does not change
    >>>> but the underlying cpumask variable is updated with the
    >>>> cpumask for the (supposedly) new node number. You can see
    >>>> that in this code snippet from kernel/sched.c:
    >>>>
    >>>> for (i = 1; i < SD_NODES_PER_DOMAIN; i++) {
    >>>> int next_node = find_next_best_node(node, &used_nodes);
    >>>>
    >>>> node_to_cpumask_ptr_next(nodemask, next_node);
    >>>> cpus_or(*span, *span, *nodemask);
    >>>> }
    >>>>
    >>>> In the optimized (x86_64) case, the pointer is simply modified
    >>>> to point to the new node_to_cpumask_map[node] entry. It remains
    >>>> a pointer to a const value.
    >>>>
    >>>> But the non-optimized version replaces the const cpumask value
    >>>> with the new cpumask value. Isn't this breaking the const
    >>>> attribute?
    >>> No, I think the pointer really should be const. This doesn't guarantee
    >>> that the value doesn't change behind our backs, it only prevents us
    >>> from modifying it ourselves.
    >>>
    >>>
    >>> Vegard
    >>>

    >> Is this what you had in mind:
    >>
    >>
    >> --- linux-2.6.tip.orig/include/asm-generic/topology.h
    >> +++ linux-2.6.tip/include/asm-generic/topology.h
    >> @@ -60,7 +60,7 @@
    >> #ifndef node_to_cpumask_ptr
    >>
    >> #define node_to_cpumask_ptr(v, node) \
    >> - cpumask_t _##v = node_to_cpumask(node), *v = &_##v
    >> + const cpumask_t _##v = node_to_cpumask(node), *v = &_##v
    >>
    >> #define node_to_cpumask_ptr_next(v, node) \
    >> _##v = node_to_cpumask(node)
    >>
    >>
    >> (It's taking a while as now I need to do some cross-compile testing.)

    >
    > Actually, no.
    >
    > We don't want the _##v to be const, do we? What do you think about
    > this? (Watch out for whitespace munges)
    >
    > diff --git a/include/asm-generic/topology.h b/include/asm-generic/topology.h
    > index a6aea79..56957f2 100644
    > --- a/include/asm-generic/topology.h
    > +++ b/include/asm-generic/topology.h
    > @@ -60,7 +60,8 @@
    > #ifndef node_to_cpumask_ptr
    >
    > #define node_to_cpumask_ptr(v, node)
    > - cpumask_t _##v = node_to_cpumask(node), *v = &_##v
    > + cpumask_t _##v = node_to_cpumask(node); \
    > + const cpumask_t *v = &_##v;
    >
    > #define node_to_cpumask_ptr_next(v, node) \
    > _##v = node_to_cpumask(node)
    >
    >
    > Vegard
    >


    Thanks. That was my alternative though I was hoping to confirm that
    the compiler detected the overwrite by node_to_cpumask_ptr_next().
    Unfortunately every non-x86 cross-compile that I have for a machine
    that has NUMA is failing in some other way.

    I'll resubmit with that change.

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

  19. Re: [PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr

    Subject: [PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr

    * Strengthen the return type for the _node_to_cpumask_ptr to be
    a const pointer. This adds compiler checking to insure that
    node_to_cpumask_map[] is not changed inadvertently.

    Applies to tip/master with the following patch applied:

    "[PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3"

    Signed-off-by: Mike Travis
    ---
    arch/x86/kernel/setup_percpu.c | 8 ++++----
    include/asm-generic/topology.h | 3 ++-
    include/asm-x86/topology.h | 10 +++++-----
    3 files changed, 11 insertions(+), 10 deletions(-)

    --- linux-2.6.tip.orig/arch/x86/kernel/setup_percpu.c
    +++ linux-2.6.tip/arch/x86/kernel/setup_percpu.c
    @@ -353,23 +353,23 @@ static const cpumask_t cpu_mask_none;
    /*
    * Returns a pointer to the bitmask of CPUs on Node 'node'.
    */
    -cpumask_t *_node_to_cpumask_ptr(int node)
    +const cpumask_t *_node_to_cpumask_ptr(int node)
    {
    if (node_to_cpumask_map == NULL) {
    printk(KERN_WARNING
    "_node_to_cpumask_ptr(%d): no node_to_cpumask_map!\n",
    node);
    dump_stack();
    - return &cpu_online_map;
    + return (const cpumask_t *)&cpu_online_map;
    }
    if (node >= nr_node_ids) {
    printk(KERN_WARNING
    "_node_to_cpumask_ptr(%d): node > nr_node_ids(%d)\n",
    node, nr_node_ids);
    dump_stack();
    - return (cpumask_t *)&cpu_mask_none;
    + return &cpu_mask_none;
    }
    - return (cpumask_t *)&node_to_cpumask_map[node];
    + return &node_to_cpumask_map[node];
    }
    EXPORT_SYMBOL(_node_to_cpumask_ptr);

    --- linux-2.6.tip.orig/include/asm-generic/topology.h
    +++ linux-2.6.tip/include/asm-generic/topology.h
    @@ -60,7 +60,8 @@
    #ifndef node_to_cpumask_ptr

    #define node_to_cpumask_ptr(v, node) \
    - cpumask_t _##v = node_to_cpumask(node), *v = &_##v
    + cpumask_t _##v = node_to_cpumask(node); \
    + const cpumask_t *v = &_##v

    #define node_to_cpumask_ptr_next(v, node) \
    _##v = node_to_cpumask(node)
    --- linux-2.6.tip.orig/include/asm-x86/topology.h
    +++ linux-2.6.tip/include/asm-x86/topology.h
    @@ -82,7 +82,7 @@ DECLARE_EARLY_PER_CPU(int, x86_cpu_to_no
    #ifdef CONFIG_DEBUG_PER_CPU_MAPS
    extern int cpu_to_node(int cpu);
    extern int early_cpu_to_node(int cpu);
    -extern cpumask_t *_node_to_cpumask_ptr(int node);
    +extern const cpumask_t *_node_to_cpumask_ptr(int node);
    extern cpumask_t node_to_cpumask(int node);

    #else /* !CONFIG_DEBUG_PER_CPU_MAPS */
    @@ -103,7 +103,7 @@ static inline int early_cpu_to_node(int
    }

    /* Returns a pointer to the cpumask of CPUs on Node 'node'. */
    -static inline cpumask_t *_node_to_cpumask_ptr(int node)
    +static inline const cpumask_t *_node_to_cpumask_ptr(int node)
    {
    return &node_to_cpumask_map[node];
    }
    @@ -118,7 +118,7 @@ static inline cpumask_t node_to_cpumask(

    /* Replace default node_to_cpumask_ptr with optimized version */
    #define node_to_cpumask_ptr(v, node) \
    - cpumask_t *v = _node_to_cpumask_ptr(node)
    + const cpumask_t *v = _node_to_cpumask_ptr(node)

    #define node_to_cpumask_ptr_next(v, node) \
    v = _node_to_cpumask_ptr(node)
    @@ -186,7 +186,7 @@ extern int __node_distance(int, int);
    #define cpu_to_node(cpu) 0
    #define early_cpu_to_node(cpu) 0

    -static inline cpumask_t *_node_to_cpumask_ptr(int node)
    +static inline const cpumask_t *_node_to_cpumask_ptr(int node)
    {
    return &cpu_online_map;
    }
    @@ -201,7 +201,7 @@ static inline int node_to_first_cpu(int

    /* Replace default node_to_cpumask_ptr with optimized version */
    #define node_to_cpumask_ptr(v, node) \
    - cpumask_t *v = _node_to_cpumask_ptr(node)
    + const cpumask_t *v = _node_to_cpumask_ptr(node)

    #define node_to_cpumask_ptr_next(v, node) \
    v = _node_to_cpumask_ptr(node)
    --
    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/

  20. Re: [PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr

    On Tue, Jul 8, 2008 at 11:35 PM, Mike Travis wrote:
    > Subject: [PATCH 1/1] x86: Change _node_to_cpumask_ptr to return const ptr
    >
    > * Strengthen the return type for the _node_to_cpumask_ptr to be
    > a const pointer. This adds compiler checking to insure that
    > node_to_cpumask_map[] is not changed inadvertently.
    >
    > Applies to tip/master with the following patch applied:
    >
    > "[PATCH 1/1] x86: Add check for node passed to node_to_cpumask V3"
    >
    > Signed-off-by: Mike Travis
    > ---


    I can successfully cross-compile the asm-generic changes with a NUMA=y
    config on ia64 without error or warning. So I guess, at long last, you
    may put, FWIW:

    Acked-by: Vegard Nossum

    Sorry for causing much trouble...


    Vegard

    --
    "The animistic metaphor of the bug that maliciously sneaked in while
    the programmer was not looking is intellectually dishonest as it
    disguises that the error is the programmer's own creation."
    -- E. W. Dijkstra, EWD1036
    --
    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/

+ Reply to Thread
Page 1 of 2 1 2 LastLast