Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 1 Nov 2022 15:59:25 +0100
From:   Peter Zijlstra <peterz@infradead.org>
To:     Chen Yu <yu.c.chen@intel.com>
Cc:     Tianchen Ding <dtcccc@linux.alibaba.com>,
        Ingo Molnar <mingo@redhat.com>,
        Juri Lelli <juri.lelli@redhat.com>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Dietmar Eggemann <dietmar.eggemann@arm.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Ben Segall <bsegall@google.com>, Mel Gorman <mgorman@suse.de>,
        Daniel Bristot de Oliveira <bristot@redhat.com>,
        Valentin Schneider <vschneid@redhat.com>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] sched: Clear ttwu_pending after enqueue_task
Message-ID: <Y2E0TeFJorjOXikX@hirez.programming.kicks-ass.net>
References: <20221101073630.2797-1-dtcccc@linux.alibaba.com>
 <Y2D2HIZuGP81w25+@hirez.programming.kicks-ass.net>
 <Y2EkXYqZ15/Kjl6H@chenyu5-mobl1>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y2EkXYqZ15/Kjl6H@chenyu5-mobl1>
Precedence: bulk

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.

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.

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