lkml.org 
[lkml]   [2011]   [Mar]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH, v7] cgroups: introduce timer slack controller
    On Wed, Mar 02, 2011 at 07:40:01PM +0100, Thomas Gleixner wrote:
    > On Wed, 2 Mar 2011, Kirill A. Shutsemov wrote:
    >
    > Not CC'ing me does not avoid another review :)

    Sorry for that. It's by mistake.

    > > diff --git a/fs/select.c b/fs/select.c
    > > index e56560d..a189e4d 100644
    > > --- a/fs/select.c
    > > +++ b/fs/select.c
    > > @@ -69,7 +69,6 @@ static long __estimate_accuracy(struct timespec *tv)
    > >
    > > long select_estimate_accuracy(struct timespec *tv)
    > > {
    > > - unsigned long ret;
    > > struct timespec now;
    > >
    > > /*
    > > @@ -81,10 +80,8 @@ long select_estimate_accuracy(struct timespec *tv)
    > >
    > > ktime_get_ts(&now);
    > > now = timespec_sub(*tv, now);
    > > - ret = __estimate_accuracy(&now);
    > > - if (ret < current->timer_slack_ns)
    > > - return current->timer_slack_ns;
    > > - return ret;
    > > + return clamp(__estimate_accuracy(&now),
    > > + get_task_timer_slack(current), LONG_MAX);
    > > }
    >
    > Can you please split out the get_task_timer_slack() change into a
    > separate patch?
    >
    > Also the function wants to be named different.
    >
    > task_get_effective_timer_slack() or such, so it becomes clear, that
    > it's not just a wrapper around tsk->timer_slack_ns
    >
    > And the places which access tsk->timer_slack_ns directly should be
    > updated with comments, why they are not using the wrapper.

    Ok, I'll fix it.

    > > +static int tslack_write_min(struct cgroup *cgroup, struct cftype *cft, u64 val)
    > > +{
    > > + struct cgroup *cur;
    > > +
    > > + if (val > ULONG_MAX)
    > > + return -EINVAL;
    > > +
    > > + /* the min timer slack value should be more or equal than parent's */
    >
    > s/should/must/

    Ok.

    > > + if (cgroup->parent) {
    > > + struct tslack_cgroup *parent = cgroup_to_tslack(cgroup->parent);
    >
    > New line between variables and code please

    Ok.

    > > + if (parent->min_slack_ns > val)
    > > + return -EPERM;
    > > + }
    > > +
    > > + cgroup_to_tslack(cgroup)->min_slack_ns = val;
    > > +
    > > + /* update children's min slack value if needed */
    > > + list_for_each_entry(cur, &cgroup->children, sibling) {
    > > + struct tslack_cgroup *child = cgroup_to_tslack(cur);
    >
    > Ditto
    >
    > > + if (val > child->min_slack_ns)
    > > + tslack_write_min(cur, cft, val);
    > > + }
    >
    > So, we can increase the value and propagate it through the groups
    > children, but decreasing it requires to go through all child groups
    > seperately.
    >
    > When I'm trying to optimize something during runtime and chose a too
    > high value I need to go through hoops and loops to make it smaller
    > again.
    >
    > Asymetric behaviour sucks always.

    Other option is to remove restriction on the min_slack_ns value and
    modify get_task_timer_slack() to use the most strict value up by
    hierarchy.
    Do you prefer this approach?

    > > +unsigned long get_task_timer_slack(struct task_struct *tsk)
    > > +{
    > > + struct cgroup_subsys_state *css;
    > > + struct tslack_cgroup *tslack_cgroup;
    > > + unsigned long ret;
    > > +
    > > + rcu_read_lock();
    >
    > Did you just remove the odd comment or actually figure out why you
    > need rcu_read_lock() here ?

    The second ;)

    > > + css = task_subsys_state(tsk, timer_slack_subsys.subsys_id);
    > > + tslack_cgroup = container_of(css, struct tslack_cgroup, css);
    > > + ret = max(tsk->timer_slack_ns, tslack_cgroup->min_slack_ns);
    > > + rcu_read_unlock();
    > > +
    > > + return ret;
    > > +}
    >
    > Otherwise, it's way more palatable than the last one.

    Thanks for reviewing.

    --
    Kirill A. Shutemov


    \
     
     \ /
      Last update: 2011-03-03 09:35    [W:0.024 / U:35.004 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site