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

Groups > linux.kernel > #1371709 > unrolled thread

Re: [RFC v2 3/7] Improve the tracking of active utilisation

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: [RFC v2 3/7] Improve the tracking of active utilisation Peter Zijlstra <peterz@infradead.org> - 2016-04-05 17:10 +0200
    Re: [RFC v2 3/7] Improve the tracking of active utilisation luca abeni <luca.abeni@unitn.it> - 2016-04-05 20:00 +0200
      Re: [RFC v2 3/7] Improve the tracking of active utilisation Peter Zijlstra <peterz@infradead.org> - 2016-04-05 20:10 +0200
        Re: [RFC v2 3/7] Improve the tracking of active utilisation luca abeni <luca.abeni@unitn.it> - 2016-04-05 21:30 +0200
          Re: [RFC v2 3/7] Improve the tracking of active utilisation Peter Zijlstra <peterz@infradead.org> - 2016-04-05 21:40 +0200
          Re: [RFC v2 3/7] Improve the tracking of active utilisation luca abeni <luca.abeni@unitn.it> - 2016-04-05 21:40 +0200
      Re: [RFC v2 3/7] Improve the tracking of active utilisation Peter Zijlstra <peterz@infradead.org> - 2016-04-05 20:10 +0200
        Re: [RFC v2 3/7] Improve the tracking of active utilisation luca abeni <luca.abeni@unitn.it> - 2016-04-05 21:40 +0200
      Re: [RFC v2 3/7] Improve the tracking of active utilisation Peter Zijlstra <peterz@infradead.org> - 2016-04-05 20:20 +0200

#1371709 — Re: [RFC v2 3/7] Improve the tracking of active utilisation

On Fri, Apr 01, 2016 at 05:12:29PM +0200, Luca Abeni wrote:
> +static void task_go_inactive(struct task_struct *p)
> +{
> +	struct sched_dl_entity *dl_se = &p->dl;
> +	struct hrtimer *timer = &dl_se->inactive_timer;
> +	struct dl_rq *dl_rq = dl_rq_of_se(dl_se);
> +	struct rq *rq = rq_of_dl_rq(dl_rq);
> +	ktime_t now, act;
> +	s64 delta;
> +	u64 zerolag_time;
> +
> +	WARN_ON(dl_se->dl_runtime == 0);
> +
> +	/* If the inactive timer is already armed, return immediately */
> +	if (hrtimer_active(&dl_se->inactive_timer))
> +		return;

So while we start the timer on the local cpu, we don't migrate the timer
when we migrate the task, so the callback can happen on a remote cpu,
right?

Therefore, the timer function might still be running, but just have done
task_rq_unlock(), which would have allowed our cpu to acquire the
rq->lock and get here.

Then the above check is true, we'll quit, but effectively the inactive
timer will not run 'again'.

> +
> +
> +	/*
> +	 * We want the timer to fire at the "0 lag time", but considering
> +	 * that it is actually coming from rq->clock and not from
> +	 * hrtimer's time base reading.
> +	 */
> +	zerolag_time = dl_se->deadline -
> +		 div64_long((dl_se->runtime * dl_se->dl_period),
> +			dl_se->dl_runtime);
> +
> +	act = ns_to_ktime(zerolag_time);
> +	now = hrtimer_cb_get_time(timer);
> +	delta = ktime_to_ns(now) - rq_clock(rq);
> +	act = ktime_add_ns(act, delta);
> +
> +	/*
> +	 * If the "0-lag time" already passed, decrease the active
> +	 * utilization now, instead of starting a timer
> +	 */
> +	if (ktime_us_delta(act, now) < 0) {
> +		sub_running_bw(dl_se, dl_rq);
> +		if (!dl_task(p))
> +			__dl_clear_params(p);
> +
> +		return;
> +	}
> +
> +	get_task_struct(p);
> +	hrtimer_start(timer, act, HRTIMER_MODE_ABS);
> +}


> @@ -1071,6 +1164,23 @@ select_task_rq_dl(struct task_struct *p, int cpu, int sd_flag, int flags)
>  	}
>  	rcu_read_unlock();
>  
> +	if (rq != cpu_rq(cpu)) {

I don't think this is right, you want:

	if (task_cpu(p) != cpu) {

because @cpu does not need to be task_cpu().

> +		int migrate_active;
> +
> +		raw_spin_lock(&rq->lock);

Which then also means @rq is 'wrong', so you'll have to add:

		rq = task_rq(p);

before this.

> +		migrate_active = hrtimer_active(&p->dl.inactive_timer);
> +		if (migrate_active)
> +			sub_running_bw(&p->dl, &rq->dl);
> +		raw_spin_unlock(&rq->lock);

At this point task_rq() is still the above rq, so if the inactive timer
hits here it will lock this rq and subtract the running bw here _again_,
right?

> +		if (migrate_active) {
> +			rq = cpu_rq(cpu);
> +			raw_spin_lock(&rq->lock);
> +			add_running_bw(&p->dl, &rq->dl);
> +			raw_spin_unlock(&rq->lock);
> +		}
> +	}

[toc] | [next] | [standalone]

#1371838

On Tue, 5 Apr 2016 17:00:36 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Fri, Apr 01, 2016 at 05:12:29PM +0200, Luca Abeni wrote:
> > +static void task_go_inactive(struct task_struct *p)
> > +{
> > +	struct sched_dl_entity *dl_se = &p->dl;
> > +	struct hrtimer *timer = &dl_se->inactive_timer;
> > +	struct dl_rq *dl_rq = dl_rq_of_se(dl_se);
> > +	struct rq *rq = rq_of_dl_rq(dl_rq);
> > +	ktime_t now, act;
> > +	s64 delta;
> > +	u64 zerolag_time;
> > +
> > +	WARN_ON(dl_se->dl_runtime == 0);
> > +
> > +	/* If the inactive timer is already armed, return immediately */
> > +	if (hrtimer_active(&dl_se->inactive_timer))
> > +		return;
> 
> So while we start the timer on the local cpu, we don't migrate the timer
> when we migrate the task, so the callback can happen on a remote cpu,
> right?
> 
> Therefore, the timer function might still be running, but just have done
> task_rq_unlock(), which would have allowed our cpu to acquire the
> rq->lock and get here.
> 
> Then the above check is true, we'll quit, but effectively the inactive
> timer will not run 'again'.
Uhm... So the problem is:
- Task T wakes up, but cannot cancel its inactive timer, because it is running
	+ This should not be a problem: inactive_task_timer() will return without
          doing anything
- Before inactive_task_timer() can actually run, task T migrates to a different CPU
- Befere the timer finishes to run, the task blocks again... So, task_go_inactive()
  sees the timer as active and returns immediately. But the timer has already
  executed (without doing anything). So noone decreases the rq utilisation.

I did not think about this issue, and I never managed to trigger it in my
tests... I'll try to see how it can be addressed. Do you have any suggestions?

[...]
> > @@ -1071,6 +1164,23 @@ select_task_rq_dl(struct task_struct *p, int cpu, int sd_flag, int flags)
> >  	}
> >  	rcu_read_unlock();
> >  
> > +	if (rq != cpu_rq(cpu)) {
> 
> I don't think this is right, you want:
> 
> 	if (task_cpu(p) != cpu) {
> 
> because @cpu does not need to be task_cpu().
Uhm... I must have misunderstood something in the code, then :(
What I want to do here is to check if select_task_rq_dl() selected
a new CPU for this task... Since at the beginning of the function
rq is set as
	rq = cpu_rq(cpu);
I was thinkint about checking if this is still true (if not, it
means that the value of "cpu" changed).

I'll look at it again.


> 
> > +		int migrate_active;
> > +
> > +		raw_spin_lock(&rq->lock);
> 
> Which then also means @rq is 'wrong', so you'll have to add:
> 
> 		rq = task_rq(p);
Ok; I completely misunderstood the current code, then... :(


> 
> before this.
> 
> > +		migrate_active = hrtimer_active(&p->dl.inactive_timer);
> > +		if (migrate_active)
> > +			sub_running_bw(&p->dl, &rq->dl);
> > +		raw_spin_unlock(&rq->lock);
> 
> At this point task_rq() is still the above rq, so if the inactive timer
> hits here it will lock this rq and subtract the running bw here _again_,
> right?
I think it will see the task state as TASK_RUNNING, so it will do nothing.
Or it will cancelled later when the task is enqueued... I'll double check this.



			Thanks,
				Luca

> 
> > +		if (migrate_active) {
> > +			rq = cpu_rq(cpu);
> > +			raw_spin_lock(&rq->lock);
> > +			add_running_bw(&p->dl, &rq->dl);
> > +			raw_spin_unlock(&rq->lock);
> > +		}
> > +	}

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

#1371848

On Tue, Apr 05, 2016 at 07:56:57PM +0200, luca abeni wrote:

> > > +		migrate_active = hrtimer_active(&p->dl.inactive_timer);
> > > +		if (migrate_active)
> > > +			sub_running_bw(&p->dl, &rq->dl);
> > > +		raw_spin_unlock(&rq->lock);
> > 
> > At this point task_rq() is still the above rq, so if the inactive timer
> > hits here it will lock this rq and subtract the running bw here _again_,
> > right?
> I think it will see the task state as TASK_RUNNING, so it will do nothing.
> Or it will cancelled later when the task is enqueued... I'll double check this.

Right, so this is select_task_rq_dl(), we run this in wakeups, before
TASK_RUNNING.

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

#1371901

On Tue, 5 Apr 2016 20:02:52 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Tue, Apr 05, 2016 at 07:56:57PM +0200, luca abeni wrote:
> 
> > > > +		migrate_active = hrtimer_active(&p->dl.inactive_timer);
> > > > +		if (migrate_active)
> > > > +			sub_running_bw(&p->dl, &rq->dl);
> > > > +		raw_spin_unlock(&rq->lock);
> > > 
> > > At this point task_rq() is still the above rq, so if the inactive timer
> > > hits here it will lock this rq and subtract the running bw here _again_,
> > > right?
> > I think it will see the task state as TASK_RUNNING, so it will do nothing.
> > Or it will cancelled later when the task is enqueued... I'll double check this.
> 
> Right, so this is select_task_rq_dl(), we run this in wakeups, before
> TASK_RUNNING.

Sigh... I knew I was missing something here... :(
So, I think the solution here is to use double_lock_balance() (or something
like that) to take both the rq locks so that the inactive timer handler cannot
run between sub_running_bw() and add_running_bw()... I'll try this.



			Thanks,
				Luca

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

#1371905

On Tue, Apr 05, 2016 at 09:24:24PM +0200, luca abeni wrote:
> On Tue, 5 Apr 2016 20:02:52 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Tue, Apr 05, 2016 at 07:56:57PM +0200, luca abeni wrote:
> > 
> > > > > +		migrate_active = hrtimer_active(&p->dl.inactive_timer);
> > > > > +		if (migrate_active)
> > > > > +			sub_running_bw(&p->dl, &rq->dl);
> > > > > +		raw_spin_unlock(&rq->lock);
> > > > 
> > > > At this point task_rq() is still the above rq, so if the inactive timer
> > > > hits here it will lock this rq and subtract the running bw here _again_,
> > > > right?
> > > I think it will see the task state as TASK_RUNNING, so it will do nothing.
> > > Or it will cancelled later when the task is enqueued... I'll double check this.
> > 
> > Right, so this is select_task_rq_dl(), we run this in wakeups, before
> > TASK_RUNNING.
> 
> Sigh... I knew I was missing something here... :(
> So, I think the solution here is to use double_lock_balance() (or something
> like that) to take both the rq locks so that the inactive timer handler cannot
> run between sub_running_bw() and add_running_bw()... I'll try this.

I'm not sure that'll fix it, because after you unlock both again, we can
hit after, and there task_rq() will still be the first rq, not the
second. So we again subtract twice from the old rq.

Only after __set_task_cpu()'s store to task_thread_info(p)->cpu will the
timer hit the new rq.

And you cannot hold a lock over that..

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

#1371910

On Tue, 5 Apr 2016 21:24:24 +0200
luca abeni <luca.abeni@unitn.it> wrote:

> On Tue, 5 Apr 2016 20:02:52 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Tue, Apr 05, 2016 at 07:56:57PM +0200, luca abeni wrote:
> > 
> > > > > +		migrate_active = hrtimer_active(&p->dl.inactive_timer);
> > > > > +		if (migrate_active)
> > > > > +			sub_running_bw(&p->dl, &rq->dl);
> > > > > +		raw_spin_unlock(&rq->lock);
> > > > 
> > > > At this point task_rq() is still the above rq, so if the inactive timer
> > > > hits here it will lock this rq and subtract the running bw here _again_,
> > > > right?
> > > I think it will see the task state as TASK_RUNNING, so it will do nothing.
> > > Or it will cancelled later when the task is enqueued... I'll double check this.
> > 
> > Right, so this is select_task_rq_dl(), we run this in wakeups, before
> > TASK_RUNNING.
> 
> Sigh... I knew I was missing something here... :(
> So, I think the solution here is to use double_lock_balance() (or something
> like that) to take both the rq locks so that the inactive timer handler cannot
> run between sub_running_bw() and add_running_bw()... I'll try this.
Double thinking about this: isn't p->pi_lock saving us here?
I mean:
	- try_to_wake_up() takes p->pi_lock before doing anything else
	- so, select_task_rq() is invoked with p->pi_lock locked
	- but inactive_task_timer() does "rq = task_rq_lock(p, &flags)", and
	  task_rq_lock() tries to take p->pi_lock
	- so, we should be safe, no?

Maybe this is why I never managed to trigger this race... :)



			Thanks,
				Luca

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

#1371851

On Tue, Apr 05, 2016 at 07:56:57PM +0200, luca abeni wrote:
> > > +	if (rq != cpu_rq(cpu)) {
> > 
> > I don't think this is right, you want:
> > 
> > 	if (task_cpu(p) != cpu) {
> > 
> > because @cpu does not need to be task_cpu().
> Uhm... I must have misunderstood something in the code, then :(
> What I want to do here is to check if select_task_rq_dl() selected
> a new CPU for this task... Since at the beginning of the function
> rq is set as
> 	rq = cpu_rq(cpu);
> I was thinkint about checking if this is still true (if not, it
> means that the value of "cpu" changed).
> 
> I'll look at it again.

Basically because:

  ac66f5477239 ("sched/numa: Introduce migrate_swap()")

we cannot (in general) assume .cpu == task_cpu(p).

Now it might still be true for deadline tasks, but I find it easier to
simply not rely on such assumptions.

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

#1371909

On Tue, 5 Apr 2016 20:00:50 +0200
Peter Zijlstra <peterz@infradead.org> wrote:

> On Tue, Apr 05, 2016 at 07:56:57PM +0200, luca abeni wrote:
> > > > +	if (rq != cpu_rq(cpu)) {
> > > 
> > > I don't think this is right, you want:
> > > 
> > > 	if (task_cpu(p) != cpu) {
> > > 
> > > because @cpu does not need to be task_cpu().
> > Uhm... I must have misunderstood something in the code, then :(
> > What I want to do here is to check if select_task_rq_dl() selected
> > a new CPU for this task... Since at the beginning of the function
> > rq is set as
> > 	rq = cpu_rq(cpu);
> > I was thinkint about checking if this is still true (if not, it
> > means that the value of "cpu" changed).
> > 
> > I'll look at it again.
> 
> Basically because:
> 
>   ac66f5477239 ("sched/numa: Introduce migrate_swap()")

Thanks; I am going to look at it


			Thanks,
				Luca

> 
> we cannot (in general) assume .cpu == task_cpu(p).
> 
> Now it might still be true for deadline tasks, but I find it easier to
> simply not rely on such assumptions.

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

#1371864

On Tue, Apr 05, 2016 at 07:56:57PM +0200, luca abeni wrote:
> On Tue, 5 Apr 2016 17:00:36 +0200
> Peter Zijlstra <peterz@infradead.org> wrote:
> 
> > On Fri, Apr 01, 2016 at 05:12:29PM +0200, Luca Abeni wrote:
> > > +static void task_go_inactive(struct task_struct *p)
> > > +{
> > > +	struct sched_dl_entity *dl_se = &p->dl;
> > > +	struct hrtimer *timer = &dl_se->inactive_timer;
> > > +	struct dl_rq *dl_rq = dl_rq_of_se(dl_se);
> > > +	struct rq *rq = rq_of_dl_rq(dl_rq);
> > > +	ktime_t now, act;
> > > +	s64 delta;
> > > +	u64 zerolag_time;
> > > +
> > > +	WARN_ON(dl_se->dl_runtime == 0);
> > > +
> > > +	/* If the inactive timer is already armed, return immediately */
> > > +	if (hrtimer_active(&dl_se->inactive_timer))
> > > +		return;
> > 
> > So while we start the timer on the local cpu, we don't migrate the timer
> > when we migrate the task, so the callback can happen on a remote cpu,
> > right?
> > 
> > Therefore, the timer function might still be running, but just have done
> > task_rq_unlock(), which would have allowed our cpu to acquire the
> > rq->lock and get here.
> > 
> > Then the above check is true, we'll quit, but effectively the inactive
> > timer will not run 'again'.
> Uhm... So the problem is:
> - Task T wakes up, but cannot cancel its inactive timer, because it is running
> 	+ This should not be a problem: inactive_task_timer() will return without
>           doing anything
> - Before inactive_task_timer() can actually run, task T migrates to a different CPU
> - Befere the timer finishes to run, the task blocks again... So, task_go_inactive()
>   sees the timer as active and returns immediately. But the timer has already
>   executed (without doing anything). So noone decreases the rq utilisation.
> 
> I did not think about this issue, and I never managed to trigger it in my
> tests... I'll try to see how it can be addressed. Do you have any suggestions?

So my brain is about to give out, but it might be easiest to simply
track if the current tasks' bandwidth is added with a per task variable
under pi and rq lock.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web