lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Thu, 15 Nov 2012 15:35:58 -0500
From:	Steven Rostedt <rostedt@...dmis.org>
To:	Yong Zhang <yong.zhang0@...il.com>
Cc:	Kirill Tkhai <tkhai@...dex.ru>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Ingo Molnar <mingo@...hat.com>,
	Peter Zijlstra <peterz@...radead.org>
Subject: Re: [sched/rt] Optimization of function pull_rt_task()

Doing my INBOX maintenance (clean up), I've stumbled on this thread
again. I'm not sure the changes here are hopeless.

On Mon, 2012-06-04 at 13:27 +0800, Yong Zhang wrote:
> On Fri, Jun 01, 2012 at 08:45:16PM +0400, Kirill Tkhai wrote:
> > 
> > 
> > 19.04.2012, 12:54, "Yong Zhang" <yong.zhang0@...il.com>:
> > > On Wed, Apr 18, 2012 at 05:16:55PM -0400, Steven Rostedt wrote:
> > >
> > >> ?On Wed, 2012-04-18 at 14:32 -0400, Steven Rostedt wrote:
> > >>> ?On Mon, 2012-04-16 at 12:06 -0400, Steven Rostedt wrote:
> > >>>> ?On Sun, 2012-04-15 at 23:45 +0400, Kirill Tkhai wrote:
> > >>>>> ?The condition (src_rq->rt.rt_nr_running) is weak because it doesn't
> > >>>>> ?consider the cases when src_rq has only processes bound to it (when
> > >>>>> ?single cpu is allowed). It may be running kernel thread like
> > >>>>> ?migration/x etc.
> > >>>>>
> > >>>>> ?So it's better to use more stronger condition which is able to exclude
> > >>>>> ?above conditions. The function has_pushable_tasks() complitely does
> > >>>>> ?this. A task may be pullable for another cpu rq only if he is pushable
> > >>>>> ?for his own queue.
> > >>>> ?I considered this before, and for some reason I never did the change.
> > >>>> ?I'll have to think about it. It seems like this would be the obvious
> > >>>> ?case, but I think there was something not so obvious that caused issues.
> > >>>> ?But I don't remember what it was.
> > >>>>
> > >>>> ?I'll have to rethink this again.
> > >>> ?I can't find anything wrong with this change. Maybe things change, or I
> > >>> ?was thinking of another change.
> > >>>
> > >>> ?I'll apply it and start running my tests against it.
> > >> ?Not only does this seem to work fine, I took it one step further :-)
> > >
> > > Hmm... throttle doesn't handle the pushable list, so we may find a
> > > throttled task by pick_next_pushable_task().
> > >
> > > Thanks,
> > > Yong
> > 
> > I don't complitelly understand throttle logic.
> > 
> > Is the source patch not-appliable the same reason?
> 
> I guess so.
> 
> Your patch will change the semantic of pick_next_pushable_task().

Looking at the original patch, I don't see how it changes the semantics
(although mine may have). The original patch was:

--- a/kernel/sched/rt.c
+++ b/kernel/sched/rt.c
@@ -1729,7 +1729,7 @@ static int pull_rt_task(struct rq *this_rq)
                /*
                 * Are there still pullable RT tasks?
                 */
-               if (src_rq->rt.rt_nr_running <= 1)
+               if (!has_pushable_tasks(src_rq))
                        goto skip;
 
                p = pick_next_highest_task_rt(src_rq, this_cpu);



And I still don't see a problem with this. If a rq has no pushable
tasks, then we shouldn't bother trying to pull from it (no task can
migrate).

Thus, the original patch, I believe should be applied without question.

Now, about my patch, the one that made pick_next_highest_task_rt into
just:

static struct task_struct *pick_next_highest_task_rt(struct rq *rq, int cpu)
{
       struct plist_head *head = &rq->rt.pushable_tasks;
       struct task_struct *next;
 
       plist_for_each_entry(next, head, pushable_tasks) {
              if (pick_rt_task(rq, next, cpu))
                      return next;
       }
 
       return NULL;
}

You said could pick a task from a throttled rq. I'm not sure that is
different than what we have now. As the current
pick_next_highest_task_rt() just does a loop over the leaf_rt_rqs which
includes throttled rqs. That's because a throttled rq will not dequeue
the rt_rq from the leaf_rt_rq list if the rt_rq has rt_nr_running != 0.


I'm still thinking about adding both patches.

-- Steve

> 
> Thanks,
> Yong
> 
> > 
> > Kirill
> > 
> > >
> > >> ?Peter, do you see anything wrong with this patch?
> > >>
> > >> ?-- Steve
> > >>
> > >> ?diff --git a/kernel/sched/rt.c b/kernel/sched/rt.c
> > >> ?index 61e3086..b44fd1b 100644
> > >> ?--- a/kernel/sched/rt.c
> > >> ?+++ b/kernel/sched/rt.c
> > >> ?@@ -1416,39 +1416,15 @@ static int pick_rt_task(struct rq *rq, struct task_struct *p, int cpu)
> > >> ??/* Return the second highest RT task, NULL otherwise */
> > >> ??static struct task_struct *pick_next_highest_task_rt(struct rq *rq, int cpu)
> > >> ??{
> > >> ?- struct task_struct *next = NULL;
> > >> ?- struct sched_rt_entity *rt_se;
> > >> ?- struct rt_prio_array *array;
> > >> ?- struct rt_rq *rt_rq;
> > >> ?- int idx;
> > >> ?+ struct plist_head *head = &rq->rt.pushable_tasks;
> > >> ?+ struct task_struct *next;
> > >>
> > >> ?- for_each_leaf_rt_rq(rt_rq, rq) {
> > >> ?- array = &rt_rq->active;
> > >> ?- idx = sched_find_first_bit(array->bitmap);
> > >> ?-next_idx:
> > >> ?- if (idx >= MAX_RT_PRIO)
> > >> ?- continue;
> > >> ?- if (next && next->prio <= idx)
> > >> ?- continue;
> > >> ?- list_for_each_entry(rt_se, array->queue + idx, run_list) {
> > >> ?- struct task_struct *p;
> > >> ?-
> > >> ?- if (!rt_entity_is_task(rt_se))
> > >> ?- continue;
> > >> ?-
> > >> ?- p = rt_task_of(rt_se);
> > >> ?- if (pick_rt_task(rq, p, cpu)) {
> > >> ?- next = p;
> > >> ?- break;
> > >> ?- }
> > >> ?- }
> > >> ?- if (!next) {
> > >> ?- idx = find_next_bit(array->bitmap, MAX_RT_PRIO, idx+1);
> > >> ?- goto next_idx;
> > >> ?- }
> > >> ?+ plist_for_each_entry(next, head, pushable_tasks) {
> > >> ?+ if (pick_rt_task(rq, next, cpu))
> > >> ?+ return next;
> > >> ??????????}
> > >>
> > >> ?- return next;
> > >> ?+ return NULL;
> > >> ??}
> > >>
> > >> ??static DEFINE_PER_CPU(cpumask_var_t, local_cpu_mask);
> > >>
> > >> ?--
> > >> ?To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > >> ?the body of a message to majordomo@...r.kernel.org
> > >> ?More majordomo info at ?http://vger.kernel.org/majordomo-info.html
> > >> ?Please read the FAQ at ?http://www.tux.org/lkml/
> > > --
> > > Only stand for myself
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@...r.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists