[PATCH] Removes extra checking in kernel/cpuset.c - Kernel

This is a discussion on [PATCH] Removes extra checking in kernel/cpuset.c - Kernel ; Hello guys, this patch removes an extra checking over 'cs' in functions 'guarantee_online_cpus' and 'guarantee_online_mems'. Thanks. Signed-off-by: Md.Rakib H. Mullick (rakib.mullick@gmail.com) --- kernel/cpuset.c.orig 2008-07-31 17:03:34.000000000 +0600 +++ kernel/cpuset.c 2008-07-31 21:33:34.000000000 +0600 @@ -282,10 +282,11 @@ static struct file_system_type cpuset_fs static ...

+ Reply to Thread
Results 1 to 4 of 4

Thread: [PATCH] Removes extra checking in kernel/cpuset.c

  1. [PATCH] Removes extra checking in kernel/cpuset.c

    Hello guys, this patch removes an extra checking over 'cs' in
    functions 'guarantee_online_cpus' and 'guarantee_online_mems'.

    Thanks.

    Signed-off-by: Md.Rakib H. Mullick (rakib.mullick@gmail.com)

    --- kernel/cpuset.c.orig 2008-07-31 17:03:34.000000000 +0600
    +++ kernel/cpuset.c 2008-07-31 21:33:34.000000000 +0600
    @@ -282,10 +282,11 @@ static struct file_system_type cpuset_fs

    static void guarantee_online_cpus(const struct cpuset *cs, cpumask_t *pmask)
    {
    - while (cs && !cpus_intersects(cs->cpus_allowed, cpu_online_map))
    - cs = cs->parent;
    - if (cs)
    + if (cs) {
    + while (!cpus_intersects(cs->cpus_allowed, cpu_online_map))
    + cs = cs->parent;
    cpus_and(*pmask, cs->cpus_allowed, cpu_online_map);
    + }
    else
    *pmask = cpu_online_map;
    BUG_ON(!cpus_intersects(*pmask, cpu_online_map));
    @@ -306,12 +307,13 @@ static void guarantee_online_cpus(const

    static void guarantee_online_mems(const struct cpuset *cs, nodemask_t *pmask)
    {
    - while (cs && !nodes_intersects(cs->mems_allowed,
    + if (cs) {
    + while (!nodes_intersects(cs->mems_allowed,
    node_states[N_HIGH_MEMORY]))
    - cs = cs->parent;
    - if (cs)
    + cs = cs->parent;
    nodes_and(*pmask, cs->mems_allowed,
    node_states[N_HIGH_MEMORY]);
    + }
    else
    *pmask = node_states[N_HIGH_MEMORY];
    BUG_ON(!nodes_intersects(*pmask, node_states[N_HIGH_MEMORY]));
    --
    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] Removes extra checking in kernel/cpuset.c

    Rakib wrote:
    > Hello guys, this patch removes an extra checking over 'cs' in
    > functions 'guarantee_online_cpus' and 'guarantee_online_mems'.
    > ...
    > - while (cs && !cpus_intersects(cs->cpus_allowed, cpu_online_map))
    > - cs = cs->parent;
    > - if (cs)
    > + if (cs) {
    > + while (!cpus_intersects(cs->cpus_allowed, cpu_online_map))
    > + cs = cs->parent;


    I don't think that works - "cs->parent" can be NULL, and will be NULL
    if cs is the root cpuset.

    So we must check for non-NULL cs each step of the way back up the
    cpuset tree.

    I must NAQ this patch.

    Holler if you think I'm missing something.

    --
    I won't rest till it's the best ...
    Programmer, Linux Scalability
    Paul Jackson 1.940.382.4214
    --
    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] Removes extra checking in kernel/cpuset.c

    On Thu, Jul 31, 2008 at 9:15 AM, Paul Jackson wrote:
    > Rakib wrote:
    >> Hello guys, this patch removes an extra checking over 'cs' in
    >> functions 'guarantee_online_cpus' and 'guarantee_online_mems'.
    >> ...
    >> - while (cs && !cpus_intersects(cs->cpus_allowed, cpu_online_map))
    >> - cs = cs->parent;
    >> - if (cs)
    >> + if (cs) {
    >> + while (!cpus_intersects(cs->cpus_allowed, cpu_online_map))
    >> + cs = cs->parent;

    >
    > I don't think that works - "cs->parent" can be NULL, and will be NULL
    > if cs is the root cpuset.


    True, but if top_cpuset.cpus_allowed doesn't intersect with
    cpu_online_map then maybe we have other problems?

    Paul
    --
    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] Removes extra checking in kernel/cpuset.c

    Paul M wrote:
    > True, but if top_cpuset.cpus_allowed doesn't intersect with
    > cpu_online_map then maybe we have other problems?


    Good point.

    .... if I did agree to this patch, there are two nits that would have
    to be fixed, and the patch resubmitted:

    1) The file pathnames should be one level deeper. As stated in
    Documentation/applying-patches.txt:

    This means that paths to files inside the patch file contain the name of the
    kernel source directories it was generated against (or some other directory
    names like "a/" and "b/").

    This patch has:

    --- kernel/cpuset.c.orig 2008-07-31 17:03:34.000000000 +0600
    +++ kernel/cpuset.c 2008-07-31 21:33:34.000000000 +0600

    There needs to be one more level of path, as in:

    --- a/kernel/cpuset.c 2008-07-31 17:03:34.000000000 +0600
    +++ b/kernel/cpuset.c 2008-07-31 21:33:34.000000000 +0600

    I prefer to use specific directory names here, reflecting the
    actual kernel version used in the development, as in:

    --- 2.6.27-rc1-mm1.orig/kernel/cpuset.c 2008-07-31 17:03:34.000000000 +0600
    +++ 2.6.27-rc1-mm1/kernel/cpuset.c 2008-07-31 21:33:34.000000000 +0600

    (though "2.6.27-rc1-mm1" is not the actual version that was
    used in composing this patch -- I don't what version was used.)

    2) The spacing of the "else" should place it on the same line as the
    preceding closing "}" from the preceding "if (...) { ... }", as in:

    if (cs) {
    while (!cpus_intersects(cs->cpus_allowed, cpu_online_map))
    cs = cs->parent;
    cpus_and(*pmask, cs->cpus_allowed, cpu_online_map);
    } else

    As submitted, the "else" was on the next line, by itself.


    However ... I still don't like the patch all that much. Granted, it
    seems to save a few bytes of kernel text space (11 bytes on my x86_64
    build), which is always tempting.

    My problem with the patch is that it entangles the logic a (slight)
    little bit more. This can be seen in the above observation of Paul M,
    which is -needed- in the case of the new code to prove that the kernel
    can't crash here, but is not needed in the case of the old code to help
    prove that.

    I work very hard in code to minimize the special cases and conditions
    that need to be in affect for each code statement. Doing so makes for
    more robust code, that is easier to understand, and less likely to get
    broken by some future change.

    I am presuming here that this code change was motivated by reading the
    source code and noticing that it resulted in some non-essential runtime
    checks for "cs" not being NULL. If this proposed change was motivated
    by actual performance analysis -- some real life need to optimize this
    code path more than I ever imagined it needed to be optimized, then my
    bias would change, toward accepting this patch.

    --
    I won't rest till it's the best ...
    Programmer, Linux Scalability
    Paul Jackson 1.940.382.4214
    --
    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