Messages in this thread | | | Date | Wed, 2 Nov 2022 11:01:38 +0800 | From | Chen Yu <> | Subject | Re: [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(). >
| |