From: Tkhai Kirill <tkhai@yandex.ru>
To: Oleg Nesterov <oleg@redhat.com>
Cc: linux-kernel@vger.kernel.org, Ingo Molnar <mingo@elte.hu>,
Peter Zijlstra <peterz@infradead.org>,
Gregory Haskins <ghaskins@novell.com>
Subject: Re: [PATCH]sched_rt.c: Avoid unnecessary dequeue and enqueue of pushable tasks in set_cpus_allowed_rt()
Date: Wed, 21 Dec 2011 01:09:44 +0400 [thread overview]
Message-ID: <324931324415384@web36.yandex.ru> (raw)
In-Reply-To: <203221324412916@web157.yandex.ru>
Again, right patch
21.12.2011, 00:28, "Tkhai Kirill" <tkhai@yandex.ru>:
> 20.12.2011, 21:44, "Oleg Nesterov" <oleg@redhat.com>:
>
>> On 12/02, Kirill Tkhai wrote:
>>> Migration status depends on a difference of weight from 0 and 1. If
>>> weight > 1 (<= 1) and old weight <= 1 (> 1) then task becomes pushable
>>> (not pushable). We are not insterested in exact values of it, is it 3 or
>>> 4, for example.
>>>
>>> Now if we are changing affinity from a set of 3 cpus to a set of 4, the
>>> task will be dequeued and enqueued sequentially without important
>>> difference in comparison with initial state. The only difference is in
>>> internal representation of plist queue of pushable tasks and the fact
>>> that the task may won't be the first in a sequence of the same priority
>>> tasks. But it seems to me it gives nothing.
>> Looks reasonable, although I can't say I really understand this code.
>> Add Gregory.
>>> Signed-off-by: Tkhai Kirill <tkhai@yandex.ru>
>>>
>>> --- kernel/sched_rt.c.orig 2011-12-02 00:29:11.970243145 +0400
>>> +++ kernel/sched_rt.c 2011-12-02 00:37:43.622846606 +0400
>> please use -p1
>
> Sorry, this time I'm sending "git diffed" output.
>
>>> @@ -1572,43 +1572,37 @@ static void set_cpus_allowed_rt(struct t
>>> const struct cpumask *new_mask)
>>> {
>>> int weight = cpumask_weight(new_mask);
>>> + struct rq *rq;
>>>
>>> BUG_ON(!rt_task(p));
>>>
>>> /*
>>> - * Update the migration status of the RQ if we have an RT task
>>> - * which is running AND changing its weight value.
>>> + * Just exit if it's not necessary to change migration status
>>> */
>>> - if (p->on_rq && (weight != p->rt.nr_cpus_allowed)) {
>>> - struct rq *rq = task_rq(p);
>>> + if ((p->rt.nr_cpus_allowed <= 1 && weight <= 1)
>>> + || (p->rt.nr_cpus_allowed > 1 && weight > 1))
>>> + return;
>> Subjective, but may be
>>
>> if ((p->rt.nr_cpus_allowed > 1) != (weight > 1))
>> return;
>>
>> looks more understandable?
>
> Yes, thanks.
>
> ---
>
> diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> index 3640ebb..4467f4d 100644
> --- a/kernel/sched/rt.c
> +++ b/kernel/sched/rt.c
> @@ -1774,43 +1774,36 @@ static void set_cpus_allowed_rt(struct task_struct *p,
> const struct cpumask *new_mask)
> {
> int weight = cpumask_weight(new_mask);
> + struct rq *rq;
>
> BUG_ON(!rt_task(p));
>
> /*
> - * Update the migration status of the RQ if we have an RT task
> - * which is running AND changing its weight value.
> + * Just exit if it's not necessary to change migration status
> */
> - if (p->on_rq && (weight != p->rt.nr_cpus_allowed)) {
> - struct rq *rq = task_rq(p);
> -
> - if (!task_current(rq, p)) {
> - /*
> - * Make sure we dequeue this task from the pushable list
> - * before going further. It will either remain off of
> - * the list because we are no longer pushable, or it
> - * will be requeued.
> - */
> - if (p->rt.nr_cpus_allowed > 1)
> - dequeue_pushable_task(rq, p);
> -
> - /*
> - * Requeue if our weight is changing and still > 1
> - */
> - if (weight > 1)
> - enqueue_pushable_task(rq, p);
> + if ((p->rt.nr_cpus_allowed > 1) != (weight > 1))
> + return;
>
> - }
> + if (!p->on_rq)
> + return;
>
> - if ((p->rt.nr_cpus_allowed <= 1) && (weight > 1)) {
> - rq->rt.rt_nr_migratory++;
> - } else if ((p->rt.nr_cpus_allowed > 1) && (weight <= 1)) {
> - BUG_ON(!rq->rt.rt_nr_migratory);
> - rq->rt.rt_nr_migratory--;
> - }
> + rq = task_rq(p);
>
> - update_rt_migration(&rq->rt);
> + /*
> + * Several cpus were allowed but now it's not so OR vice versa
> + */
> + if (weight <= 1) {
> + if (!task_current(rq, p))
> + dequeue_pushable_task(rq, p);
> + BUG_ON(!rq->rt.rt_nr_migratory);
> + rq->rt.rt_nr_migratory--;
> + } else {
> + if (!task_current(rq, p))
> + enqueue_pushable_task(rq, p);
> + rq->rt.rt_nr_migratory++;
> }
> +
> + update_rt_migration(&rq->rt);
> }
>
> /* Assumes rq->lock is held */
---
diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
index 3640ebb..bf48343 100644
--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -1774,43 +1774,36 @@ static void set_cpus_allowed_rt(struct task_struct *p,
const struct cpumask *new_mask)
{
int weight = cpumask_weight(new_mask);
+ struct rq *rq;
BUG_ON(!rt_task(p));
/*
- * Update the migration status of the RQ if we have an RT task
- * which is running AND changing its weight value.
+ * Just exit if it's not necessary to change migration status
*/
- if (p->on_rq && (weight != p->rt.nr_cpus_allowed)) {
- struct rq *rq = task_rq(p);
-
- if (!task_current(rq, p)) {
- /*
- * Make sure we dequeue this task from the pushable list
- * before going further. It will either remain off of
- * the list because we are no longer pushable, or it
- * will be requeued.
- */
- if (p->rt.nr_cpus_allowed > 1)
- dequeue_pushable_task(rq, p);
-
- /*
- * Requeue if our weight is changing and still > 1
- */
- if (weight > 1)
- enqueue_pushable_task(rq, p);
+ if ((p->rt.nr_cpus_allowed > 1) == (weight > 1))
+ return;
- }
+ if (!p->on_rq)
+ return;
- if ((p->rt.nr_cpus_allowed <= 1) && (weight > 1)) {
- rq->rt.rt_nr_migratory++;
- } else if ((p->rt.nr_cpus_allowed > 1) && (weight <= 1)) {
- BUG_ON(!rq->rt.rt_nr_migratory);
- rq->rt.rt_nr_migratory--;
- }
+ rq = task_rq(p);
- update_rt_migration(&rq->rt);
+ /*
+ * Several cpus were allowed but now it's not so OR vice versa
+ */
+ if (weight <= 1) {
+ if (!task_current(rq, p))
+ dequeue_pushable_task(rq, p);
+ BUG_ON(!rq->rt.rt_nr_migratory);
+ rq->rt.rt_nr_migratory--;
+ } else {
+ if (!task_current(rq, p))
+ enqueue_pushable_task(rq, p);
+ rq->rt.rt_nr_migratory++;
}
+
+ update_rt_migration(&rq->rt);
}
/* Assumes rq->lock is held */
next prev parent reply other threads:[~2011-12-20 21:20 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-12-01 21:26 [PATCH]sched_rt.c: Avoid unnecessary dequeue and enqueue of pushable tasks in set_cpus_allowed_rt() Kirill Tkhai
2011-12-20 17:44 ` Oleg Nesterov
2011-12-20 20:28 ` Tkhai Kirill
2011-12-20 21:09 ` Tkhai Kirill [this message]
2012-02-13 17:23 ` Steven Rostedt
2012-02-19 14:17 ` Kirill Tkhai
2012-03-16 23:58 ` Kirill Tkhai
2012-04-10 13:58 ` Steven Rostedt
2012-04-10 15:52 ` Steven Rostedt
2012-04-11 5:06 ` Kirill Tkhai
2012-04-14 18:22 ` [tip:sched/core] sched_rt: Avoid unnecessary dequeue and enqueue of pushable tasks in set_cpus_allowed_rt () tip-bot for Kirill Tkhai
2012-01-11 19:10 [PATCH] sched_rt.c: Avoid unnecessary dequeue and enqueue of pushable tasks in set_cpus_allowed_rt() Kirill Tkhai
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=324931324415384@web36.yandex.ru \
--to=tkhai@yandex.ru \
--cc=ghaskins@novell.com \
--cc=linux-kernel@vger.kernel.org \
--cc=mingo@elte.hu \
--cc=oleg@redhat.com \
--cc=peterz@infradead.org \
/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).