lkml.org 
[lkml]   [2008]   [Aug]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [PATCH] cpuset: Rework sched domains and CPU hotplug handling (2.6.27-rc1)
    So far as I can tell, the logic and design is ok.

    I found some of the comments, function names and code factoring
    to be confusing or incomplete. And I suspect that the rebuilding
    of sched domains in the case that sched_power_savings_store()
    is called in kernel/sched.c, on systems using cpusets, is not
    needed or desirable (I could easily be wrong here - beware!).

    I'm attaching a patch that has the changes that I would like
    to suggest for your consideration. I have only recompiled this
    patch, for one configuration - x86_64 with CPUSETS. I am hoping,
    Max, that you can complete the testing.

    The patch below applies to 2.6.27-rc1, plus the first patch,
    "origin.patch" in Andrew's 2.6.27-rc1-mm1 quilt patch stack,
    plus your (Max's) latest:
    [PATCH] cpuset: Rework sched domains and CPU hotplug handling (2.6.27-rc1)

    Here's a description of most of what I noticed:

    1) Unrelated spacing tweak, changing:
    LIST_HEAD(q); /* queue of cpusets to be scanned*/
    to:
    LIST_HEAD(q); /* queue of cpusets to be scanned */

    2) The phrasing:
    Must be called with cgroup_lock held.
    seems imprecise to me; "cgroup_lock" is the name of a wrapper
    function, not of a lock. The underlying lock is cgroup_mutex,
    which is still mentioned by name in various kernel/cpuset.c
    comments, even though cgroup_mutex is static in kernel/cgroup.c
    So I fiddled with the wording of this phrasing.

    3) You removed the paragraph:
    * ... May take callback_mutex during
    * call due to the kfifo_alloc() and kmalloc() calls. May nest
    * a call to the get_online_cpus()/put_online_cpus() pair.
    * Must not be called holding callback_mutex, because we must not
    * call get_online_cpus() while holding callback_mutex. Elsewhere
    * the kernel nests callback_mutex inside get_online_cpus() calls.
    * So the reverse nesting would risk an ABBA deadlock.

    But you didn't replace it with an updated locking description.
    I expanded and tweaked various locking comments.

    4) The alignment of the right side of consecutive assignment statements,
    as in:
    ndoms = 0;
    doms = NULL;
    dattr = NULL;
    csa = NULL;
    or
    *domains = doms;
    *attributes = dattr;
    is not usually done in kernel code. I suggest obeying convention,
    and not aligning these here either.

    5) The rebuilding of sched domains in the sched_power_savings_store()
    routine in kernel/sched.c struck me as inappropriate for systems
    that were managing sched domains using cpusets. So I made that
    sched.c rebuild only apply if CONFIG_CPUSETS was not configured,
    which in turn enabled me to remove rebuild_sched_domains() from
    being externally visible outside kernel/cpuset.c

    I don't know if this change is correct, however.

    6) The renaming of rebuild_sched_domains() to generate_sched_domains()
    was only partial, and along with the added similarly named routines
    seemed confusing to me. Also, that rename of rebuild_sched_domains()
    was only partially accomplished, not being done in some comments
    and in one printk kernel warning.

    I reverted that rename.

    I also reduced by one the number of functions needed to handle
    the asynchronous invocation of this rebuild, essentially collapsing
    your routines rebuild_sched_domains() and rebuild_domains_callback()
    into a single routine, named rebuild_sched_domains_thread().

    Thanks to the above change (5), the naming and structure of these
    routines was no longer visible outside kernel/cpuset.c, making
    this collapsing of two functions into one easier.

    ---
    include/linux/cpuset.h | 7 ----
    kernel/cpuset.c | 73 ++++++++++++++++++++++++-------------------------
    kernel/sched.c | 18 ++++--------
    3 files changed, 43 insertions(+), 55 deletions(-)

    --- 2.6.27-rc1-mm1.orig/include/linux/cpuset.h 2008-08-02 03:51:59.000000000 -0700
    +++ 2.6.27-rc1-mm1/include/linux/cpuset.h 2008-08-03 19:24:40.306964316 -0700
    @@ -78,8 +78,6 @@ extern void cpuset_track_online_nodes(vo

    extern int current_cpuset_is_being_rebound(void);

    -extern void rebuild_sched_domains(void);
    -
    #else /* !CONFIG_CPUSETS */

    static inline int cpuset_init_early(void) { return 0; }
    @@ -158,11 +156,6 @@ static inline int current_cpuset_is_bein
    return 0;
    }

    -static inline void rebuild_sched_domains(void)
    -{
    - partition_sched_domains(0, NULL, NULL);
    -}
    -
    #endif /* !CONFIG_CPUSETS */

    #endif /* _LINUX_CPUSET_H */
    --- 2.6.27-rc1-mm1.orig/kernel/cpuset.c 2008-08-02 09:42:56.000000000 -0700
    +++ 2.6.27-rc1-mm1/kernel/cpuset.c 2008-08-03 21:55:31.983690126 -0700
    @@ -482,7 +482,7 @@ static int validate_change(const struct
    }

    /*
    - * Helper routine for generate_sched_domains().
    + * Helper routine for rebuild_sched_domains().
    * Do cpusets a, b have overlapping cpus_allowed masks?
    */
    static int cpusets_overlap(struct cpuset *a, struct cpuset *b)
    @@ -526,11 +526,12 @@ update_domain_attr_tree(struct sched_dom
    }

    /*
    - * generate_sched_domains()
    + * rebuild_sched_domains()
    *
    * This function builds a partial partition of the systems CPUs
    * A 'partial partition' is a set of non-overlapping subsets whose
    * union is a subset of that set.
    + *
    * The output of this function needs to be passed to kernel/sched.c
    * partition_sched_domains() routine, which will rebuild the scheduler's
    * load balancing domains (sched domains) as specified by that partial
    @@ -579,10 +580,10 @@ update_domain_attr_tree(struct sched_dom
    * element of the partition (one sched domain) to be passed to
    * partition_sched_domains().
    */
    -static int generate_sched_domains(cpumask_t **domains,
    +static int rebuild_sched_domains(cpumask_t **domains,
    struct sched_domain_attr **attributes)
    {
    - LIST_HEAD(q); /* queue of cpusets to be scanned*/
    + LIST_HEAD(q); /* queue of cpusets to be scanned */
    struct cpuset *cp; /* scans q */
    struct cpuset **csa; /* array of all cpuset ptrs */
    int csn; /* how many cpuset ptrs in csa so far */
    @@ -593,9 +594,9 @@ static int generate_sched_domains(cpumas
    int nslot; /* next empty doms[] cpumask_t slot */

    ndoms = 0;
    - doms = NULL;
    + doms = NULL;
    dattr = NULL;
    - csa = NULL;
    + csa = NULL;

    /* Special case for the 99% of systems with one, full, sched domain */
    if (is_sched_load_balance(&top_cpuset)) {
    @@ -733,49 +734,41 @@ restart:
    done:
    kfree(csa);

    - *domains = doms;
    + *domains = doms;
    *attributes = dattr;
    return ndoms;
    }

    /*
    - * Rebuilds scheduler domains. See generate_sched_domains() description
    - * for details.
    + * Rebuild scheduler domains, called from rebuild_sched_domains_work
    + * work queue.
    + *
    + * Call with neither cgroup_mutex held nor within get_online_cpus().
    + * Takes both cgroup_mutex and get_online_cpus().
    *
    - * Must be called under get_online_cpus(). This is an external interface
    - * and must not be used inside the cpuset code to avoid circular locking
    - * dependency. cpuset code should use async_rebuild_sched_domains() instead.
    + * Cannot be directly called from cpuset code handling changes
    + * to the cpuset pseudo-filesystem, because it cannot be called
    + * from code that already holds cgroup_mutex.
    */
    -void rebuild_sched_domains(void)
    +static void rebuild_sched_domains_thread(struct work_struct *unused)
    {
    struct sched_domain_attr *attr;
    cpumask_t *doms;
    int ndoms;

    - /*
    - * We have to iterate cgroup hierarch which requires cgroup lock.
    - */
    + get_online_cpus();
    cgroup_lock();
    - ndoms = generate_sched_domains(&doms, &attr);
    + ndoms = rebuild_sched_domains(&doms, &attr);
    cgroup_unlock();

    /* Have scheduler rebuild the domains */
    partition_sched_domains(ndoms, doms, attr);
    -}
    -
    -/*
    - * Simply calls rebuild_sched_domains() with correct locking rules.
    - */
    -static void rebuild_domains_callback(struct work_struct *work)
    -{
    - get_online_cpus();
    - rebuild_sched_domains();
    put_online_cpus();
    }
    -static DECLARE_WORK(rebuild_domains_work, rebuild_domains_callback);
    +static DECLARE_WORK(rebuild_sched_domains_work, rebuild_sched_domains_thread);

    /*
    - * Internal helper for rebuilding sched domains when something changes.
    + * Rebuild scheduler domains, asynchronously in a separate thread.
    *
    * If the flag 'sched_load_balance' of any cpuset with non-empty
    * 'cpus' changes, or if the 'cpus' allowed changes in any cpuset
    @@ -783,14 +776,19 @@ static DECLARE_WORK(rebuild_domains_work
    * 'cpus' is removed, then call this routine to rebuild the
    * scheduler's dynamic sched domains.
    *
    - * rebuild_sched_domains() must be called under get_online_cpus() and
    - * it needs to take cgroup_lock(). But most of the cpuset code is already
    - * holding cgroup_lock(). In order to avoid incorrect lock nesting we
    - * delegate the actual domain rebuilding to the workqueue.
    + * The rebuild_sched_domains() and partition_sched_domains()
    + * routines must nest cgroup_lock() inside get_online_cpus(),
    + * but such cpuset changes as these must nest that locking the
    + * other way, holding cgroup_lock() for much of the code.
    + *
    + * So in order to avoid an ABBA deadlock, the cpuset code handling
    + * these user changes delegates the actual sched domain rebuilding
    + * to a separate workqueue thread, which ends up processing the
    + * above rebuild_sched_domains_thread() function.
    */
    static void async_rebuild_sched_domains(void)
    {
    - queue_work(cpuset_wq, &rebuild_domains_work);
    + queue_work(cpuset_wq, &rebuild_sched_domains_work);
    }

    /**
    @@ -1756,7 +1754,7 @@ static struct cgroup_subsys_state *cpuse
    /*
    * If the cpuset being removed has its flag 'sched_load_balance'
    * enabled, then simulate turning sched_load_balance off, which
    - * will call rebuild_sched_domains().
    + * will call async_rebuild_sched_domains().
    */

    static void cpuset_destroy(struct cgroup_subsys *ss, struct cgroup *cont)
    @@ -1775,7 +1773,7 @@ static void cpuset_destroy(struct cgroup
    struct cgroup_subsys cpuset_subsys = {
    .name = "cpuset",
    .create = cpuset_create,
    - .destroy = cpuset_destroy,
    + .destroy = cpuset_destroy,
    .can_attach = cpuset_can_attach,
    .attach = cpuset_attach,
    .populate = cpuset_populate,
    @@ -1966,6 +1964,9 @@ static void scan_for_empty_cpusets(const
    *
    * This routine ensures that top_cpuset.cpus_allowed tracks
    * cpu_online_map on each CPU hotplug (cpuhp) event.
    + *
    + * Called within get_online_cpus(). Needs to call cgroup_lock()
    + * before calling rebuild_sched_domains().
    */
    static int cpuset_track_online_cpus(struct notifier_block *unused_nb,
    unsigned long phase, void *unused_cpu)
    @@ -1988,7 +1989,7 @@ static int cpuset_track_online_cpus(stru
    cgroup_lock();
    top_cpuset.cpus_allowed = cpu_online_map;
    scan_for_empty_cpusets(&top_cpuset);
    - ndoms = generate_sched_domains(&doms, &attr);
    + ndoms = rebuild_sched_domains(&doms, &attr);
    cgroup_unlock();

    /* Have scheduler rebuild the domains */
    --- 2.6.27-rc1-mm1.orig/kernel/sched.c 2008-08-02 04:12:13.000000000 -0700
    +++ 2.6.27-rc1-mm1/kernel/sched.c 2008-08-03 19:55:40.128044004 -0700
    @@ -7645,18 +7645,8 @@ match2:
    }

    #if defined(CONFIG_SCHED_MC) || defined(CONFIG_SCHED_SMT)
    -int arch_reinit_sched_domains(void)
    -{
    - get_online_cpus();
    - rebuild_sched_domains();
    - put_online_cpus();
    - return 0;
    -}
    -
    static ssize_t sched_power_savings_store(const char *buf, size_t count, int smt)
    {
    - int ret;
    -
    if (buf[0] != '0' && buf[0] != '1')
    return -EINVAL;

    @@ -7665,9 +7655,13 @@ static ssize_t sched_power_savings_store
    else
    sched_mc_power_savings = (buf[0] == '1');

    - ret = arch_reinit_sched_domains();
    +#ifndef CONFIG_CPUSETS
    + get_online_cpus();
    + partition_sched_domains(0, NULL, NULL);
    + put_online_cpus();
    +#endif

    - return ret ? ret : count;
    + return count;
    }

    #ifdef CONFIG_SCHED_MC

    --
    I won't rest till it's the best ...
    Programmer, Linux Scalability
    Paul Jackson <pj@sgi.com> 1.940.382.4214


    \
     
     \ /
      Last update: 2008-08-04 08:03    [W:0.039 / U:271.192 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site