Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1462791 > unrolled thread

Re: [PATCH v3 09/13] sched/fair: Let asymmetric cpu configurations balance at wake-up

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 09/13] sched/fair: Let asymmetric cpu configurations  balance at wake-up Peter Zijlstra <peterz@infradead.org> - 2016-08-15 15:50 +0200
    Re: [PATCH v3 09/13] sched/fair: Let asymmetric cpu configurations  balance at wake-up Morten Rasmussen <morten.rasmussen@arm.com> - 2016-08-15 17:10 +0200
      Re: [PATCH v3 09/13] sched/fair: Let asymmetric cpu configurations  balance at wake-up Peter Zijlstra <peterz@infradead.org> - 2016-08-15 17:20 +0200
        Re: [PATCH v3 09/13] sched/fair: Let asymmetric cpu configurations  balance at wake-up Morten Rasmussen <morten.rasmussen@arm.com> - 2016-08-15 17:40 +0200

#1462791 — Re: [PATCH v3 09/13] sched/fair: Let asymmetric cpu configurations balance at wake-up

On Mon, Jul 25, 2016 at 02:34:26PM +0100, Morten Rasmussen wrote:


Because I forgot _again_, I added:

/*
 * Disable WAKE_AFFINE in the case where task @p doesn't fit in the
 * capacity of either the waking CPU @cpu or the previous CPU @prev_cpu.
 *
 * In that case WAKE_AFFINE doesn't make sense and we'll let
 * BALANCE_WAKE sort things out.
 */

> +static int wake_cap(struct task_struct *p, int cpu, int prev_cpu)
> +{
> +	long min_cap, max_cap;
> +
> +	min_cap = min(capacity_orig_of(prev_cpu), capacity_orig_of(cpu));
> +	max_cap = cpu_rq(cpu)->rd->max_cpu_capacity;

There's a tiny hole here, which I'm fairly sure we don't care about. If
@p last ran on @prev_cpu before @prev_cpu was split from @rd this
doesn't 'work' right.

> +	/* Minimum capacity is close to max, no need to abort wake_affine */
> +	if (max_cap - min_cap < max_cap >> 3)
> +		return 0;
> +
> +	return min_cap * 1024 < task_util(p) * capacity_margin;
> +}
> +
>  /*
>   * select_task_rq_fair: Select target runqueue for the waking task in domains
>   * that have the 'sd_flag' flag set. In practice, this is SD_BALANCE_WAKE,
> @@ -5389,7 +5414,8 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int sd_flag, int wake_f
>  
>  	if (sd_flag & SD_BALANCE_WAKE) {
>  		record_wakee(p);
> -		want_affine = !wake_wide(p) && cpumask_test_cpu(cpu, tsk_cpus_allowed(p));
> +		want_affine = !wake_wide(p) && !wake_cap(p, cpu, prev_cpu)
> +			      && cpumask_test_cpu(cpu, tsk_cpus_allowed(p));
>  	}
>  
>  	rcu_read_lock();
> -- 
> 1.9.1
> 

[toc] | [next] | [standalone]

#1462871

On Mon, Aug 15, 2016 at 03:39:49PM +0200, Peter Zijlstra wrote:
> On Mon, Jul 25, 2016 at 02:34:26PM +0100, Morten Rasmussen wrote:
> 
> 
> Because I forgot _again_, I added:
> 
> /*
>  * Disable WAKE_AFFINE in the case where task @p doesn't fit in the
>  * capacity of either the waking CPU @cpu or the previous CPU @prev_cpu.
>  *
>  * In that case WAKE_AFFINE doesn't make sense and we'll let
>  * BALANCE_WAKE sort things out.
>  */

Thanks.

> 
> > +static int wake_cap(struct task_struct *p, int cpu, int prev_cpu)
> > +{
> > +	long min_cap, max_cap;
> > +
> > +	min_cap = min(capacity_orig_of(prev_cpu), capacity_orig_of(cpu));
> > +	max_cap = cpu_rq(cpu)->rd->max_cpu_capacity;
> 
> There's a tiny hole here, which I'm fairly sure we don't care about. If
> @p last ran on @prev_cpu before @prev_cpu was split from @rd this
> doesn't 'work' right.

I hadn't considered that. What is 'working right' in this scenario?
Ignoring @prev_cpu as it isn't a valid option anymore?

In that case, since @prev_cpu is only used as part the min() it should
only cause min_cap to be potentially smaller than it should be, not
larger. It could lead us to let BALANCE_WAKE take over in scenarios
where select_idle_sibling() would have been sufficient, but it should
harm.

However, as you say, I'm not sure if we care that much.

Talking about @rd, I discussed with Juri and Dietmar the other week
whether the root_domain is RCU protected, and if we therefore have to
move the call to wake_cap() after the rcu_read_lock() below. I haven't
yet done thorough investigation to find the answer. Should it be
protected?

> 
> > +	/* Minimum capacity is close to max, no need to abort wake_affine */
> > +	if (max_cap - min_cap < max_cap >> 3)
> > +		return 0;
> > +
> > +	return min_cap * 1024 < task_util(p) * capacity_margin;
> > +}
> > +
> >  /*
> >   * select_task_rq_fair: Select target runqueue for the waking task in domains
> >   * that have the 'sd_flag' flag set. In practice, this is SD_BALANCE_WAKE,
> > @@ -5389,7 +5414,8 @@ select_task_rq_fair(struct task_struct *p, int prev_cpu, int sd_flag, int wake_f
> >  
> >  	if (sd_flag & SD_BALANCE_WAKE) {
> >  		record_wakee(p);
> > -		want_affine = !wake_wide(p) && cpumask_test_cpu(cpu, tsk_cpus_allowed(p));
> > +		want_affine = !wake_wide(p) && !wake_cap(p, cpu, prev_cpu)
> > +			      && cpumask_test_cpu(cpu, tsk_cpus_allowed(p));
> >  	}
> >  
> >  	rcu_read_lock();
> > -- 
> > 1.9.1
> > 

[toc] | [prev] | [next] | [standalone]

#1462882

On Mon, Aug 15, 2016 at 04:01:34PM +0100, Morten Rasmussen wrote:
> On Mon, Aug 15, 2016 at 03:39:49PM +0200, Peter Zijlstra wrote:
> > > +static int wake_cap(struct task_struct *p, int cpu, int prev_cpu)
> > > +{
> > > +	long min_cap, max_cap;
> > > +
> > > +	min_cap = min(capacity_orig_of(prev_cpu), capacity_orig_of(cpu));
> > > +	max_cap = cpu_rq(cpu)->rd->max_cpu_capacity;
> > 
> > There's a tiny hole here, which I'm fairly sure we don't care about. If
> > @p last ran on @prev_cpu before @prev_cpu was split from @rd this
> > doesn't 'work' right.
> 
> I hadn't considered that. What is 'working right' in this scenario?
> Ignoring @prev_cpu as it isn't a valid option anymore?

Probably, yeah.

> In that case, since @prev_cpu is only used as part the min() it should
> only cause min_cap to be potentially smaller than it should be, not
> larger. It could lead us to let BALANCE_WAKE take over in scenarios
> where select_idle_sibling() would have been sufficient, but it should
> harm.

+not, right?

> However, as you say, I'm not sure if we care that much.

Yeah, don't think so, its extremely unlikely to happen, almost nobody
mucks about with root_domains anyway. And those that do, do so once to
setup things and then leave them be.

> Talking about @rd, I discussed with Juri and Dietmar the other week
> whether the root_domain is RCU protected, and if we therefore have to
> move the call to wake_cap() after the rcu_read_lock() below. I haven't
> yet done thorough investigation to find the answer. Should it be
> protected?

Yeah, I think either RCU or RCU-sched, I forever forget.

[toc] | [prev] | [next] | [standalone]

#1462926

On Mon, Aug 15, 2016 at 05:10:06PM +0200, Peter Zijlstra wrote:
> On Mon, Aug 15, 2016 at 04:01:34PM +0100, Morten Rasmussen wrote:
> > On Mon, Aug 15, 2016 at 03:39:49PM +0200, Peter Zijlstra wrote:
> > > > +static int wake_cap(struct task_struct *p, int cpu, int prev_cpu)
> > > > +{
> > > > +	long min_cap, max_cap;
> > > > +
> > > > +	min_cap = min(capacity_orig_of(prev_cpu), capacity_orig_of(cpu));
> > > > +	max_cap = cpu_rq(cpu)->rd->max_cpu_capacity;
> > > 
> > > There's a tiny hole here, which I'm fairly sure we don't care about. If
> > > @p last ran on @prev_cpu before @prev_cpu was split from @rd this
> > > doesn't 'work' right.
> > 
> > I hadn't considered that. What is 'working right' in this scenario?
> > Ignoring @prev_cpu as it isn't a valid option anymore?
> 
> Probably, yeah.
> 
> > In that case, since @prev_cpu is only used as part the min() it should
> > only cause min_cap to be potentially smaller than it should be, not
> > larger. It could lead us to let BALANCE_WAKE take over in scenarios
> > where select_idle_sibling() would have been sufficient, but it should
> > harm.
> 
> +not, right?

Yes :)

> 
> > However, as you say, I'm not sure if we care that much.
> 
> Yeah, don't think so, its extremely unlikely to happen, almost nobody
> mucks about with root_domains anyway. And those that do, do so once to
> setup things and then leave them be.
> 
> > Talking about @rd, I discussed with Juri and Dietmar the other week
> > whether the root_domain is RCU protected, and if we therefore have to
> > move the call to wake_cap() after the rcu_read_lock() below. I haven't
> > yet done thorough investigation to find the answer. Should it be
> > protected?
> 
> Yeah, I think either RCU or RCU-sched, I forever forget.

Okay. Should I send an updated version?

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web