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:   Wed, 2 Nov 2022 11:01:38 +0800
From:   Chen Yu <yu.c.chen@intel.com>
To:     Peter Zijlstra <peterz@infradead.org>
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: <Y2Hdkkp4mYK+fjDP@chenyu5-mobl1>
References: <20221101073630.2797-1-dtcccc@linux.alibaba.com>
 <Y2D2HIZuGP81w25+@hirez.programming.kicks-ass.net>
 <Y2EkXYqZ15/Kjl6H@chenyu5-mobl1>
 <Y2E0TeFJorjOXikX@hirez.programming.kicks-ass.net>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <Y2E0TeFJorjOXikX@hirez.programming.kicks-ass.net>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?lCa3GtJ7OtbchPpjVHh6e5l6OHqjX5DZHd9zLNihqNCbjG3zz1jJe548yAXn?=
 =?us-ascii?Q?AxGa5Rr1mOYE3engt5netEoaKyLwGQ8eplJC5FECtuZCeZx/gnajsXXjob/H?=
 =?us-ascii?Q?KchVVNQSJBfl56/fq3nIBamxir4FdjYRPquShtFdTXPmeZBznVHtW3WjsAj4?=
 =?us-ascii?Q?cTlIVW/TZG/tkrstTPjtictkKH2owG7lOrzIzV2i378X7PQ2miotV+nGARHo?=
 =?us-ascii?Q?5XtRC2DoyF3EJCA3fxvSF4Yjy1IwXJFl2ilyzpbodJE3Xz+oozQNFtMcwC1l?=
 =?us-ascii?Q?U27vC8q5Dseg8eZYvGG/xKDjWrkpcRtMl2cp2fsx9IPlUffopbJsbwGDGSoC?=
 =?us-ascii?Q?X3z5+z0J+6xDx9JFSCeNW9WaLyafQsQb0fdub2+hVZcrCi5vQH+4HAaPtD+w?=
 =?us-ascii?Q?YGKfMShRyiilcfGfR8po6BQ9UUqZ2qeihGDmIW57kZ/kwbXAmrjIX3HhlyoP?=
 =?us-ascii?Q?vVNdxJBWykmy4JIgTfWIRCG9J9Q06ex4H1ABWTMT5ntZepN2ssHX/LHdtdXQ?=
 =?us-ascii?Q?CPvnesf6gOa/skFfMQ47X0xxvulAANJecjR1WrZfWygFlfW1xRMfQRvbjsS4?=
 =?us-ascii?Q?jRlwv+lZVdQQWWh6w0Udi5PexE/WCj5AQ5zZT3y1UqyHckCth/zncfp84itz?=
 =?us-ascii?Q?kITURFxLhxGVumssOb7mkundttGTRfHP6W27GjbGnytF6Wv9Uhn+fc4ckRH/?=
 =?us-ascii?Q?OHxyiAqf1y1r4FQCfN8ZRrVRTSjaRWgzEKYLa0ETcs6klSlG7GAHuCzHSaDM?=
 =?us-ascii?Q?OhA/Py2dJhBE1Mv370JrEmSPzUN7anMPQ02VXmxSk2lfWqS8/xyV1vMxt7ih?=
 =?us-ascii?Q?QFZZLM/oEtw+QMZ/FM+2JPLo4JjQx9JF5eEwHuQWjuC6gxh9ve/p5FgqruPB?=
 =?us-ascii?Q?ZoiWG4nBUqUwQooOzU4Mgntq3tzYiCQpvpCrFVt92FqH+yz3S9S1LwBn2gRR?=
 =?us-ascii?Q?KSjOEbqmfgr06lthpE6WImjKwqIzEUO4zb0IuS9cCggXIYwxEoVIfljgVrFF?=
 =?us-ascii?Q?1mShfZJK+HGjVADufPPCIXTcjYGz08JHRB69AtKDs/FVsmtWVLMcEbiqoeR5?=
 =?us-ascii?Q?rdiaXQicHrjlI2qSldIVrMUsd9uHP/LmsX9pPHSRZqbyI2BTuYftcjdEIeWk?=
 =?us-ascii?Q?OfLiEckgnLNzL5DsftIUdKd+ePVhWuv7XY/9trAz+FwIcMIRnhCOvnBWeQhI?=
 =?us-ascii?Q?zCms7T3DzBOM0xI7fA4P7FGBghBclhPPyTibS/sTCq33CsBIbQ9/MXqJ8+CJ?=
 =?us-ascii?Q?QZ2S3UTbUpKu1EG6RurGxOZA8RRA/yqdRLGEpm8MQ9c88jnm/4yrxq4GlHoE?=
 =?us-ascii?Q?kR7pI8PBiJ4vrwIuMpgxq0dsPF0sQQ4Ds9OzjlizPyK+IQDTbQ8wHpNYnoyo?=
 =?us-ascii?Q?bjhHhQJhTK7FhWLdnPnivm0tCJg2erIRvEHBj7Nrj5IchCLLaF3eveoyxF3D?=
 =?us-ascii?Q?stn9/2UJla02UyFNXPeILwUdR+UYL57JdsZfmF4F/f6WFBLTWGTmhSkLkRbk?=
 =?us-ascii?Q?7KsoLXKOcBjRnrJ9/jo4a+/Z4cuqy/O13H+sEHYA3j7mbBHl3Bv8T6vI6wLi?=
 =?us-ascii?Q?P3FsnEf+S0xLgQP01vlh5Y+gnPOqkiIgt5MFdXNe?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 4db33d00-0305-44f4-0a03-08dabc7e9d04
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6206.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 02 Nov 2022 03:02:01.4253
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: Boznk0aVofsbmvooMrIfLvTSB0Q5DaPM2dz2EYlHH0TGfUJYotuEvfWvanmiQG0GdXa2vg8R3xHIIxubodNM1g==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB5914
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_PASS,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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().
>