From: Wanpeng Li <kernellwp@gmail.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
kvm <kvm@vger.kernel.org>, Wanpeng Li <wanpeng.li@hotmail.com>,
Ingo Molnar <mingo@redhat.com>, Juri Lelli <juri.lelli@arm.com>,
Luca Abeni <luca.abeni@unitn.it>,
Frederic Weisbecker <fweisbec@gmail.com>
Subject: Re: [PATCH v3] sched/deadline: Fix the intention to re-evalute tick dependency for offline cpu
Date: Wed, 31 Aug 2016 18:31:19 +0800 [thread overview]
Message-ID: <CANRm+CyKeQi3xLPumHXr1aLttLLODLSPZUxZ9XV7WLjdSUuRAg@mail.gmail.com> (raw)
In-Reply-To: <20160831092125.GA10626@twins.programming.kicks-ass.net>
2016-08-31 17:21 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:
> On Fri, Aug 19, 2016 at 09:56:59PM +0800, Wanpeng Li wrote:
>> 2016-08-19 21:25 GMT+08:00 Peter Zijlstra <peterz@infradead.org>:
>> > On Fri, Aug 12, 2016 at 05:24:03PM +0800, Wanpeng Li wrote:
>> >
>> >> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
>> >> index d091f4a..ce0fb00 100644
>> >> --- a/kernel/sched/deadline.c
>> >> +++ b/kernel/sched/deadline.c
>> >> @@ -641,6 +641,11 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
>> >> goto unlock;
>> >> }
>> >>
>> >> +#ifdef CONFIG_SMP
>> >> + if (unlikely(!rq->online))
>> >> + goto offline;
>> >> +#endif
>> >> +
>> >> enqueue_task_dl(rq, p, ENQUEUE_REPLENISH);
>> >> if (dl_task(rq->curr))
>> >> check_preempt_curr_dl(rq, p, 0);
>> >> @@ -648,6 +653,7 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
>> >> resched_curr(rq);
>> >>
>> >> #ifdef CONFIG_SMP
>> >> +offline:
>> >> /*
>> >> * Perform balancing operations here; after the replenishments. We
>> >> * cannot drop rq->lock before this, otherwise the assertion in
>> >> @@ -659,6 +665,7 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
>> >> * XXX figure out if select_task_rq_dl() deals with offline cpus.
>> >> */
>> >> if (unlikely(!rq->online)) {
>> >> + replenish_dl_entity(dl_se, dl_se);
>> >> lockdep_unpin_lock(&rq->lock, rf.cookie);
>> >> rq = dl_task_offline_migration(rq, p);
>> >
>> > So I don't like this, even if it magically works. With this we end up
>> > calling dl_task_offline_migration() -> deactivate_task() while the task
>> > isn't on the runqueue at all.
>>
>> So how about v1, it also works :), https://lkml.org/lkml/2016/8/10/898
>
> Does something like so work? Notice what we have preemption disabled (by
> virtue of having pi_lock and rq->lock taken) and thus cannot hotplug.
> Therefore if we notice a rq not being online, it must stay that way,
> equally any online rq must also stay that way.
>
> This means we can fold the two online tests you had and simply do the rq
> switch beforehand.
Thanks for the proposal Peterz! It works and I just sent out v4. :)
Regards,
Wanpeng Li
>
> Completely untested...
>
> ---
> kernel/sched/deadline.c | 46 ++++++++++++++++++----------------------------
> 1 file changed, 18 insertions(+), 28 deletions(-)
>
> diff --git a/kernel/sched/deadline.c b/kernel/sched/deadline.c
> index d091f4a95416..bcade08772a8 100644
> --- a/kernel/sched/deadline.c
> +++ b/kernel/sched/deadline.c
> @@ -243,10 +243,8 @@ static struct rq *find_lock_later_rq(struct task_struct *task, struct rq *rq);
> static struct rq *dl_task_offline_migration(struct rq *rq, struct task_struct *p)
> {
> struct rq *later_rq = NULL;
> - bool fallback = false;
>
> later_rq = find_lock_later_rq(p, rq);
> -
> if (!later_rq) {
> int cpu;
>
> @@ -254,7 +252,6 @@ static struct rq *dl_task_offline_migration(struct rq *rq, struct task_struct *p
> * If we cannot preempt any rq, fall back to pick any
> * online cpu.
> */
> - fallback = true;
> cpu = cpumask_any_and(cpu_active_mask, tsk_cpus_allowed(p));
> if (cpu >= nr_cpu_ids) {
> /*
> @@ -274,16 +271,7 @@ static struct rq *dl_task_offline_migration(struct rq *rq, struct task_struct *p
> double_lock_balance(rq, later_rq);
> }
>
> - /*
> - * By now the task is replenished and enqueued; migrate it.
> - */
> - deactivate_task(rq, p, 0);
> set_task_cpu(p, later_rq->cpu);
> - activate_task(later_rq, p, 0);
> -
> - if (!fallback)
> - resched_curr(later_rq);
> -
> double_unlock_balance(later_rq, rq);
>
> return later_rq;
> @@ -641,29 +629,31 @@ static enum hrtimer_restart dl_task_timer(struct hrtimer *timer)
> goto unlock;
> }
>
> - enqueue_task_dl(rq, p, ENQUEUE_REPLENISH);
> - if (dl_task(rq->curr))
> - check_preempt_curr_dl(rq, p, 0);
> - else
> - resched_curr(rq);
> -
> #ifdef CONFIG_SMP
> - /*
> - * Perform balancing operations here; after the replenishments. We
> - * cannot drop rq->lock before this, otherwise the assertion in
> - * start_dl_timer() about not missing updates is not true.
> - *
> - * If we find that the rq the task was on is no longer available, we
> - * need to select a new rq.
> - *
> - * XXX figure out if select_task_rq_dl() deals with offline cpus.
> - */
> if (unlikely(!rq->online)) {
> + /*
> + * If the runqueue is no longer available, migrate the
> + * task elsewhere. This necessarily changes rq.
> + */
> lockdep_unpin_lock(&rq->lock, rf.cookie);
> rq = dl_task_offline_migration(rq, p);
> rf.cookie = lockdep_pin_lock(&rq->lock);
> +
> + /*
> + * Now that the task has been migrated to the new RQ and we
> + * have that locked, proceed as normal and enqueue the task
> + * there.
> + */
> }
> +#endif
>
> + enqueue_task_dl(rq, p, ENQUEUE_REPLENISH);
> + if (dl_task(rq->curr))
> + check_preempt_curr_dl(rq, p, 0);
> + else
> + resched_curr(rq);
> +
> +#ifdef CONFIG_SMP
> /*
> * Queueing this task back might have overloaded rq, check if we need
> * to kick someone away.
--
Regards,
Wanpeng Li
prev parent reply other threads:[~2016-08-31 10:31 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-08-12 9:24 [PATCH v3] sched/deadline: Fix the intention to re-evalute tick dependency for offline cpu Wanpeng Li
2016-08-16 2:04 ` Wanpeng Li
2016-08-19 13:25 ` Peter Zijlstra
2016-08-19 13:56 ` Wanpeng Li
2016-08-31 9:21 ` Peter Zijlstra
2016-08-31 10:31 ` Wanpeng Li [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CANRm+CyKeQi3xLPumHXr1aLttLLODLSPZUxZ9XV7WLjdSUuRAg@mail.gmail.com \
--to=kernellwp@gmail.com \
--cc=fweisbec@gmail.com \
--cc=juri.lelli@arm.com \
--cc=kvm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=luca.abeni@unitn.it \
--cc=mingo@redhat.com \
--cc=peterz@infradead.org \
--cc=wanpeng.li@hotmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).