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, 27 Jun 2023 16:05:47 +0800
From:   Chen Yu <yu.c.chen@intel.com>
To:     Aaron Lu <aaron.lu@intel.com>
CC:     Peter Zijlstra <peterz@infradead.org>,
        Vincent Guittot <vincent.guittot@linaro.org>,
        Ingo Molnar <mingo@redhat.com>,
        Juri Lelli <juri.lelli@redhat.com>,
        Deng Pan <pan.deng@intel.com>, <tim.c.chen@intel.com>,
        <linux-kernel@vger.kernel.org>, <tianyou.li@intel.com>,
        <yu.ma@intel.com>, <lipeng.zhu@intel.com>,
        Tim Chen <tim.c.chen@linux.intel.com>
Subject: Re: [PATCH v2] sched/task_group: Re-layout structure to reduce false
 sharing
Message-ID: <ZJqYW3Ul/nixpU9m@chenyu5-mobl2.ccr.corp.intel.com>
References: <20230621081425.420607-1-pan.deng@intel.com>
 <20230626054756.GA435374@ziqianlu-dell>
 <ZJlDsbyNmcKtweg4@chenyu5-mobl2.ccr.corp.intel.com>
 <20230626125335.GA508448@ziqianlu-dell>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20230626125335.GA508448@ziqianlu-dell>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?NM9hk1DZ7WQh2zX+wAY0CeQswJpS8ZtrtmIQwXlrc5AEqsi92gFNrK5kfVac?=
 =?us-ascii?Q?yPPN6BY6toeA+UnCBc4NVNPWZoPHNkczL2NE7nOxyoMi6hxY9s8Dkgm5TjSu?=
 =?us-ascii?Q?jg0JpaWPQN3yRfG4xtxnhTH51VoGHWPHqqW/COJpRZ9UDJOqvWvD8EZAe3Ng?=
 =?us-ascii?Q?nl51Yni3FEHFaWisCZswyvLEpzgnH50kSIRlVCb2mcvp/3tRm9jhbvrSHiJe?=
 =?us-ascii?Q?WlRZ3myabXjKunqO2LI9dFo1I4+hOmPEdX4AGvkrq6fwRewC5VMxNEdabSs3?=
 =?us-ascii?Q?vvcgwxPR0AgVdVOB3k4eYfnopTrhpwr0no9cFmbXmiUQrRmkA4hjf4gfrBB3?=
 =?us-ascii?Q?MhT3M4Tjodz4slMe7OOlkXg3HLf7ySA6cjovWhGKsfm2yUXeEdW93/F+nsq0?=
 =?us-ascii?Q?p7AnervXh57Wwzk7ALMz/eR5P6kz13uIT+MILT6mJtQtDoLmYsAXdbbaBjFt?=
 =?us-ascii?Q?gpc1kaq1qLQ2mSsGuP109boYUHWRP1Tcea8NQd/2yWbobYITA7F6a74OtaMU?=
 =?us-ascii?Q?sQu5ks+QiJr711qcOcdjsX7kdqVYz7O7byWCi2QyEbNRHQowa21jIC6DblmQ?=
 =?us-ascii?Q?T615ZloZPgsV7cGD0Z6FPXfpNa3Nz+SAUdD/iogIneXGB2sBnmg8AEzgqEDA?=
 =?us-ascii?Q?FUpwUb5lWjY5DpeNPswWnV2JB3nxkiOqV/Kst256i6YZqPe7imVBCjhar2Ez?=
 =?us-ascii?Q?VTYkoM6l3CojgkMdaIWkyA+IAnccmRNK+YYYgY8oHGwTT4w+i8XFRlHUiNwC?=
 =?us-ascii?Q?MnKrWl2QNLq36309Zdw145UNpmYLj2Y+jmZ9016/iknZYKHw7wPI5g/OEnN4?=
 =?us-ascii?Q?zHBS4RL7mxqSl0HzHbP7Yhf5MXFd6RJe93nXibBGR9oqUIUEojVExRrLZEJU?=
 =?us-ascii?Q?arDSiaFFiOOQOiTyjm7siemwwHtwM57fzVPuoxuEN011SBq5aKXHdaiRzGrk?=
 =?us-ascii?Q?v6IeVrICGF4dChm16HSIM7GzJC9goNwJgmEodY2y+4XdNe/yVlyI8Yij4UT3?=
 =?us-ascii?Q?6FOB/d6hlKIDjgj/cThmlcPcL5gcR6aO50jxnX7GOjQasUiztoCeERV7JG4v?=
 =?us-ascii?Q?R4m7lm7Ddm5mPRszSJWkeHG2XSbXcd/LZ6kxbs4QnznVFmr+h/azKp6EF00x?=
 =?us-ascii?Q?CaBXPeK9AcVRLvwTQ30xOfomtA64Y2YhVL9Fsx5N+b9wvhp+LyMCSftvGTIS?=
 =?us-ascii?Q?x4KeilpG4gmrG465Ngwye/G+lggO28EyZvZJVyP3Bc1ug9bwLs6eJVVRRv3m?=
 =?us-ascii?Q?C68zF3s2YuPOSl2UAjB5c2Keh6s3FQULO3tJbVa0ZMglfE+GRtFsUqxT2jCH?=
 =?us-ascii?Q?JaYfRwH1OCo38eyJG5wFyXLu3VqAX8lZvEZf30ubE8rBq8bZ49b5tzce2GdC?=
 =?us-ascii?Q?8sRPo4/fAxsLgDvNb1/kOlsVHBVT6HJZRRaqJzd10ZYC5Kt4qVtYmIWxpKhf?=
 =?us-ascii?Q?5SdDeUmE3lC1rnH6L1MrG85vgUZjCOA/79KrlLHXllPOo2kagbPFLRbTYD2W?=
 =?us-ascii?Q?taxDsQwOm4SJgJZPqWyKQPLwe1Jl2ew8yTvAJ9zu66604IFx0ibHg0Ew3V6c?=
 =?us-ascii?Q?0UjU8jHK3T+iLoxlYHahtfqbODuA2/kUUFvHhoJY?=
X-MS-Exchange-CrossTenant-Network-Message-Id: fca5b440-030e-4fe6-2595-08db76e55811
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6206.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 27 Jun 2023 08:06:00.1589
 (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: BUiT+VmITUTPLFfw+YoQGdJsNI2rtb693Ac00wQxF2flwBf5m+oqSVnV4ukTTyq7QbmcP4L+vmaxSLJBPMI6sQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5007
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 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_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED
        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 2023-06-26 at 20:53:35 +0800, Aaron Lu wrote:
> On Mon, Jun 26, 2023 at 03:52:17PM +0800, Chen Yu wrote:
> > Besides the cache line alignment, if the task is not a rt one,
> > why do we have to touch that, I wonder if the following change can avoid that:
> > 
> > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h
> > index ec7b3e0a2b20..067f1310bad2 100644
> > --- a/kernel/sched/sched.h
> > +++ b/kernel/sched/sched.h
> > @@ -1958,8 +1958,10 @@ static inline void set_task_rq(struct task_struct *p, unsigned int cpu)
> >  #endif
> >  
> >  #ifdef CONFIG_RT_GROUP_SCHED
> > -	p->rt.rt_rq  = tg->rt_rq[cpu];
> > -	p->rt.parent = tg->rt_se[cpu];
> > +	if (p->sched_class = &rt_sched_class) {
>                            ==  :-)
> 
Yes.
> > +		p->rt.rt_rq  = tg->rt_rq[cpu];
> > +		p->rt.parent = tg->rt_se[cpu];
> > +	}
> >  #endif
> >  }
> 
> If a task starts life as a SCHED_NORMAL one and then after some time
> it's changed to a RT one, then during its next ttwu(), if it didn't
> migrate, then set_task_rq() will not be called and p->rt.rt_rq will
> keep as NULL which will cause problem when this task gets enqueued as
> a rt one.
>
Yeah, this case should be covered to avoid the NULL pointer issue.
It seems that set_task_rq() is widely used to assign task's rt field
irrelevant of what context the task is in to avoid extra check. 
> The follow diff seems to cure this issue:
> 
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index c7db597e8175..8c57148e668c 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -7801,6 +7801,20 @@ static int __sched_setscheduler(struct task_struct *p,
>  	}
>  	__setscheduler_uclamp(p, attr);
>  
> +#ifdef CONFIG_RT_GROUP_SCHED
> +	/*
> +	 * Make sure when this task becomes a rt one,
> +	 * its rt fields have valid value.
> +	 */
> +	if (rt_prio(newprio)) {
> +		struct task_group *tg = task_group(p);
> +		int cpu = cpu_of(rq);
> +
> +		p->rt.rt_rq = tg->rt_rq[cpu];
> +		p->rt.parent = tg->rt_se[cpu];
> +	}
> +#endif
> +
>  	if (queued) {
>  		/*
>  		 * We enqueue to tail when the priority of a task is
> 
> But I'm not sure if it's worth the trouble.
>
Yeah, cache alignment change seems to be simpler.

thanks,
Chenyu 
> Thanks,
> Aaron