linux-kernel - Re: [PATCH] sched/core: fix pick_next

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 24 Aug 2021 11:01:50 +0800
From:   Tao Zhou <tao.zhou@...ux.dev>
To:     Josh Don <joshdon@...gle.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        Ingo Molnar <mingo@...hat.com>,
        Juri Lelli <juri.lelli@...hat.com>,
        Vincent Guittot <vincent.guittot@...aro.org>,
        Dietmar Eggemann <dietmar.eggemann@....com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Ben Segall <bsegall@...gle.com>, Mel Gorman <mgorman@...e.de>,
        Daniel Bristot de Oliveira <bristot@...hat.com>,
        Joel Fernandes <joel@...lfernandes.org>,
        Vineeth Pillai <vineethrp@...il.com>,
        linux-kernel <linux-kernel@...r.kernel.org>, tao.zhou@...ux.dev
Subject: Re: [PATCH] sched/core: fix pick_next_task 'max' tracking

Hi Josh,

On Mon, Aug 23, 2021 at 04:24:26PM -0700, Josh Don wrote:
> Hi Peter,
> 
> On Mon, Aug 23, 2021 at 4:17 AM Peter Zijlstra <peterz@...radead.org> wrote:
> [snip]
> > +       for_each_cpu(i, smt_mask) {
> > +               rq_i = cpu_rq(i);
> > +               p = rq_i->core_temp;
> >
> > -                       /*
> > -                        * If this sibling doesn't yet have a suitable task to
> > -                        * run; ask for the most eligible task, given the
> > -                        * highest priority task already selected for this
> > -                        * core.
> > -                        */
> > -                       p = pick_task(rq_i, class, max, fi_before);
> > +               if (!cookie_equals(p, cookie)) {
> > +                       p = NULL;
> > +                       if (cookie)
> > +                               p = sched_core_find(rq_i, cookie);
> 
> In the case that 'max' has a zero cookie, shouldn't we search for a

In the original pick_task(), when cookie is zero, we choose class pick
task or force idle task.

> match on this cpu if the original class pick ('p') had a non-zero
> cookie? We don't enqueue tasks with zero cookie in the core_tree, so I
> forget if there was some other reasoning here.

So, no need to search the core_tree when cookie is zero.

But I'm not sure that force idle pick condition(in pick_task())
is also covered by this clause in the first filter max loop in
the rewrite..
  
  '
  if (!max || prio_less(max, p, fi_before))
		max = p;
  '

I just thought there are three(one add by Josh) places that can return
max;

1, !cookie condition and class_pick > max
2, cookie equal condition and class_pick > max(add by Josh)
3, cookie not equal condition and class_pick > max.

The rewrite change a little with the class pick, it loops all class to
find the max first(finally will get one task and not be possible return
NULL). This is not the same with the original.

It is very possible that I'm getting wrong, then please shout to me.

> >                         if (!p)
> > -                               continue;
> > +                               p = idle_sched_class.pick_task(rq_i);
> > +               }



Thanks,
Tao