Re: [PATCH, v7] cgroups: introduce timer slack controller

["Kirill A. Shutemov" <kirill-oKw7cIdHH8eLwutG50LtGA@public.gmane.org>]

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