lkml.org 
[lkml]   [2022]   [Nov]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] sched: Clear ttwu_pending after enqueue_task
On 2022-11-01 at 15:59:25 +0100, Peter Zijlstra wrote:
> On Tue, Nov 01, 2022 at 09:51:25PM +0800, Chen Yu wrote:
>
> > > Could you try the below instead? Also note the comment; since you did
> > > the work to figure out why -- best record that for posterity.
> > >
> > > @@ -3737,6 +3730,13 @@ void sched_ttwu_pending(void *arg)
> > > set_task_cpu(p, cpu_of(rq));
> > >
> > > ttwu_do_activate(rq, p, p->sched_remote_wakeup ? WF_MIGRATED : 0, &rf);
> > > + /*
> > > + * Must be after enqueueing at least once task such that
> > > + * idle_cpu() does not observe a false-negative -- if it does,
> > > + * it is possible for select_idle_siblings() to stack a number
> > > + * of tasks on this CPU during that window.
> > > + */
> > > + WRITE_ONCE(rq->ttwu_pending, 0);
> > Just curious why do we put above code inside llist_for_each_entry_safe loop?
>
> > My understanding is that once 1 task is queued, select_idle_cpu() would not
> > treat this rq as idle anymore because nr_running is not 0. But would this bring
> > overhead to write the rq->ttwu_pending multiple times, do I miss something?
>
> So the consideration is that by clearing it late, you might also clear a
> next set; consider something like:
>
>
> cpu0 cpu1 cpu2
>
> ttwu_queue()
> ->ttwu_pending = 1;
> llist_add()
>
> sched_ttwu_pending()
> llist_del_all()
> ... long ...
> ttwu_queue()
> ->ttwu_pending = 1
> llist_add()
>
> ... time ...
> ->ttwu_pending = 0
>
> Which leaves you with a non-empty list but with ttwu_pending == 0.
>
Thanks for the explaination, in theory the race windows could
be shrinked but could not be closed due to ttwu_pending is
not protected by lock in ttwu_queue() -> __ttwu_queue_wakelist()
I suppose.
> But I suppose that's not actually better with my variant, since it keeps
> writing 0s. We can make it more complicated again, but perhaps it
> doesn't matter and your version is good enough.
I see, although I'm not the author of this patch :)

thanks,
Chenyu
>
> But please update with a comment on why it needs to be after
> ttwu_do_activate().
>

\
 
 \ /
  Last update: 2022-11-02 04:04    [W:0.110 / U:0.052 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site