Re: [PATCH 11/33] task containersv11 make cpusets a client of containers - Kernel

This is a discussion on Re: [PATCH 11/33] task containersv11 make cpusets a client of containers - Kernel ; Paul M, This snippet from the memory allocation hot path worries me a bit. Once per memory page allocation, we go through here, needing to peak inside the current tasks cpuset to see if it has changed (it's 'mems_generation' value ...

+ Reply to Thread
Results 1 to 4 of 4

Thread: Re: [PATCH 11/33] task containersv11 make cpusets a client of containers

  1. Re: [PATCH 11/33] task containersv11 make cpusets a client of containers

    Paul M,

    This snippet from the memory allocation hot path worries me a bit.

    Once per memory page allocation, we go through here, needing to peak inside
    the current tasks cpuset to see if it has changed (it's 'mems_generation'
    value doesn't match the last seen value we have stashed in the task struct.)

    @@ -653,20 +379,19 @@ void cpuset_update_task_memory_state(voi
    struct task_struct *tsk = current;
    struct cpuset *cs;

    - if (tsk->cpuset == &top_cpuset) {
    + if (task_cs(tsk) == &top_cpuset) {
    /* Don't need rcu for top_cpuset. It's never freed. */
    my_cpusets_mem_gen = top_cpuset.mems_generation;
    } else {
    rcu_read_lock();
    - cs = rcu_dereference(tsk->cpuset);
    - my_cpusets_mem_gen = cs->mems_generation;
    + my_cpusets_mem_gen = task_cs(current)->mems_generation;
    rcu_read_unlock();
    }

    With this new cgroup code, the task_cs macro was added, -twice-,
    which deals with the fact that what used to be a single pointer
    in the task struct directly to the tasks cpuset is now roughly
    two more dereferences and an indexing away:

    static inline struct cpuset *task_cs(struct task_struct *task)
    {
    return container_of(task_subsys_state(task, cpuset_subsys_id),
    struct cpuset, css);
    }

    static inline struct cgroup_subsys_state *task_subsys_state(
    struct task_struct *task, int subsys_id)
    {
    return rcu_dereference(task->cgroups->subsys[subsys_id]);
    }


    At a minimum, could you change that last added line to use 'tsk'
    instead of 'current'? This should save one instruction, as 'tsk'
    will likely already be in a register.

    + my_cpusets_mem_gen = task_cs(tsk)->mems_generation;

    I guess the two, rather than one, invocations of task_cs() won't matter
    much, as they are on the same address, so the second invocation will
    hit cache lines just found on the first invocation.

    I wonder if we can save any cache line hits on this, or if there is
    someway to measure whether or not this has noticeable performance
    impact.

    .... Probably this is all lost in the noise of the other stuff that
    gets coded in the memory allocation hot path. It would be nice to
    think that it actually matters however.

    --
    I won't rest till it's the best ...
    Programmer, Linux Scalability
    Paul Jackson 1.925.600.0401
    -
    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 11/33] task containersv11 make cpusets a client of containers

    On 10/4/07, Paul Jackson wrote:
    > Paul M,
    >
    > This snippet from the memory allocation hot path worries me a bit.
    >
    > Once per memory page allocation, we go through here, needing to peak inside
    > the current tasks cpuset to see if it has changed (it's 'mems_generation'
    > value doesn't match the last seen value we have stashed in the task struct.)
    >
    > @@ -653,20 +379,19 @@ void cpuset_update_task_memory_state(voi
    > struct task_struct *tsk = current;
    > struct cpuset *cs;
    >
    > - if (tsk->cpuset == &top_cpuset) {
    > + if (task_cs(tsk) == &top_cpuset) {
    > /* Don't need rcu for top_cpuset. It's never freed. */
    > my_cpusets_mem_gen = top_cpuset.mems_generation;
    > } else {
    > rcu_read_lock();
    > - cs = rcu_dereference(tsk->cpuset);
    > - my_cpusets_mem_gen = cs->mems_generation;
    > + my_cpusets_mem_gen = task_cs(current)->mems_generation;
    > rcu_read_unlock();
    > }
    >
    > With this new cgroup code, the task_cs macro was added, -twice-,
    > which deals with the fact that what used to be a single pointer
    > in the task struct directly to the tasks cpuset is now roughly
    > two more dereferences and an indexing away:


    It's two constant-indexed dereferences *in total*, compared to a
    single constant-indexed dereference in the pre-cgroup case.

    The cpuset pointer is found at
    task->cgroups->subsys[cpuset_subsys_id], where cpuset_subsys_id is a
    compile-time constant.

    >
    > At a minimum, could you change that last added line to use 'tsk'
    > instead of 'current'? This should save one instruction, as 'tsk'
    > will likely already be in a register.


    Sounds reasonable.

    >
    > I wonder if we can save any cache line hits on this, or if there is
    > someway to measure whether or not this has noticeable performance
    > impact.


    I didn't notice any performance hit on a pure allocate/free memory
    benchmark relative to non-cgroup cpusets. (There was a small
    performance hit relative to not using cpusets at all, but that was to
    be expected).

    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/

  3. Re: [PATCH 11/33] task containersv11 make cpusets a client of containers

    Paul M wrote:
    > It's two constant-indexed dereferences *in total*, compared to a
    > single constant-indexed dereference in the pre-cgroup case.


    Ok - the C expression is longer and I didn't realize how
    little difference it made in the end (the executing code.)

    Good - thanks.

    --
    I won't rest till it's the best ...
    Programmer, Linux Scalability
    Paul Jackson 1.925.600.0401
    -
    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 11/33] task containersv11 make cpusets a client of containers

    Paul M wrote:
    > I didn't notice any performance hit on a pure allocate/free memory
    > benchmark relative to non-cgroup cpusets.


    Good.

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