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;
From:   "Deng, Pan" <pan.deng@intel.com>
To:     Peter Zijlstra <peterz@infradead.org>,
        "Lu, Aaron" <aaron.lu@intel.com>
CC:     "Chen, Tim C" <tim.c.chen@intel.com>,
        "vincent.guittot@linaro.org" <vincent.guittot@linaro.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "Li, Tianyou" <tianyou.li@intel.com>, "Ma, Yu" <yu.ma@intel.com>,
        "Zhu, Lipeng" <lipeng.zhu@intel.com>,
        "Chen, Yu C" <yu.c.chen@intel.com>,
        Tim Chen <tim.c.chen@linux.intel.com>
Subject: RE: [PATCH v2] sched/task_group: Re-layout structure to reduce false
 sharing
Thread-Topic: [PATCH v2] sched/task_group: Re-layout structure to reduce false
 sharing
Thread-Index: AQHZpBf8y9gtMyr/606YJQEIlMf8Fa+cm1MAgAHc14CAAGHSAA==
Date:   Tue, 27 Jun 2023 16:12:51 +0000
Message-ID: <DM6PR11MB3260026F33333A70AF2F02169627A@DM6PR11MB3260.namprd11.prod.outlook.com>
References: <20230621081425.420607-1-pan.deng@intel.com>
 <20230626054756.GA435374@ziqianlu-dell>
 <20230627101437.GY4253@hirez.programming.kicks-ass.net>
In-Reply-To: <20230627101437.GY4253@hirez.programming.kicks-ass.net>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?+/oZSgnat1e/NsR5rcmReRETdyXLPAjvFjHKP7Ciowwpfx3rp8XeDHnF7n+K?=
 =?us-ascii?Q?cEjp1S3vlvohpjVfvY0TKzcGdhutpXlDLT37CttN9n1xJIKPrJDVcjDEjGeE?=
 =?us-ascii?Q?LiWXfAuGfzMrX23+hnFeVaRdBEWc0P4WDTitktQyxPVTKbvqGJaUsiF6GrxS?=
 =?us-ascii?Q?TQO3rydfgKwFuJ+DDCjLBjn9N/WM6o6bpthUHdXLoUVOJ8QMtJQEipG4C/yN?=
 =?us-ascii?Q?DhHtJi+DNl+vZDV3rfVAsYo6w0HZK5qK14Wj0FyayLUMnL72ep8CQrRX9IG+?=
 =?us-ascii?Q?69xrTdntfOiL8Pcfk5Kr3H7Tsm+BHFzBwn0F2NA2fza0PtY/C447I8hFHbYe?=
 =?us-ascii?Q?FC6gwJlfAQsitdSR0TcqIaOq7eCdoeBh/DakbDPFsrMuShC3LkIOAg9oz1Sx?=
 =?us-ascii?Q?/1X0gtRQ3p8GMd+Tf+TGZ/pzYA/185dfLciPl66Z5psC4UFGPSVby3eLkgxh?=
 =?us-ascii?Q?aNMpYoXAybnSzZKDs+7o39ck56IDsHXuuIAWlETyMmi4EeE5PZnMATCu00Ul?=
 =?us-ascii?Q?hFZqVgitFOZNoBdPtBursbUeaZ7+YWIjB7hZjB96eM94CuKABn7i+wGDgowO?=
 =?us-ascii?Q?X1XvPci2N2tIXMSQBgpv/R8cW6PaWFsGONj5U+w8s9Y2v6RZSBmhUYmVAi6V?=
 =?us-ascii?Q?E9Isa5iq49mJAXuiJryzRMHpq+TWtdAgy93byaNtAL5hCF4L1KbzQCGR67lv?=
 =?us-ascii?Q?mWK4yrfLGAAV7HlS5XGvxc9sOCsvoZJTi5KyzB4s5Q+WVVyMgrX/P4OXKNUv?=
 =?us-ascii?Q?vywgfUKMc3Yrx01honOik2mt7Xfs24mvQmGiFYDjyyFMMJsGZhx9T97VJyCY?=
 =?us-ascii?Q?wTqG9G6g5IDjQMXy72N2MiTJpnO8EtPiDTN66LDwjM/FQUxZPSOV6mCFy4Km?=
 =?us-ascii?Q?QcyXF0XnqueNVUeEqpvDYELpXiXvwKZZo4vigdBjCJOps/aTIcpw2fjBpMA3?=
 =?us-ascii?Q?SKEQ0K5kLx/7rEpPPKj8e9Hg/ktcQ3tp1SJ/8vyFED5PbfMLqwJwMmsdAeF4?=
 =?us-ascii?Q?Y9yJrKuJFXhRcNMNVhpzl4WBC74UYKTxP5JFiGla/ZQ/wTPo7KMHG4geLEs5?=
 =?us-ascii?Q?EHehFcm1+89mmkgrZgjTpoaieCTbzOYJuN+TwgCQuTYZXZ6XXkG63bRhpRGD?=
 =?us-ascii?Q?MreR5K7FjzBvHIQ1/Wn90gmnzgy30zJZ+BdegrOIDDRQYn2aQZ6lzgmnNOx/?=
 =?us-ascii?Q?vEekNhlCpt9zrvZe3sf9O+4/DS+yXXiWg/f8EaZdlDDTqTyAw6SSr4hv0NWI?=
 =?us-ascii?Q?CYPUvgM98nmAXpLzlPBDBtIOUnqJaa6QWG+5c0gIbYsSq3Wo6cg1ahRjPAQ1?=
 =?us-ascii?Q?/6ayUAITpRs78RhQH43F0sHKKAAm/nsoDSiE8WaMyC71BuNESaYxfC17ZRCj?=
 =?us-ascii?Q?CC2yl4geeHUibERWMdyX290u97WqR5GDP2yxK1RzlBBccMi71HWwQHR5cM5Y?=
 =?us-ascii?Q?VDee5fC7bZmOyt4wBHjcQSdHp+udz3CwThyg3+2U4plj8K0LXcq8n2HUgW5n?=
 =?us-ascii?Q?NpX1I+JJpfQC3IK3JgWb8YIUouM9EIFbc0F7EAFaqRRi/6JV+ZlyQC2uwAru?=
 =?us-ascii?Q?8JsfhVoohOtKWMN485Q=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB3260.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 25349cdf-c762-40a4-51ab-08db77295b86
X-MS-Exchange-CrossTenant-originalarrivaltime: 27 Jun 2023 16:12:51.5931
 (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: DK0cBx2BnFXU89R/sOHmSF3cyLz2LrJ9dHW/4JGqHLsNJexBWo12mgPy1eqx+7YU03DqQ3zNnfvW9CEB1pGmCQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR11MB6848
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,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


> -----Original Message-----
> From: Peter Zijlstra <peterz@infradead.org>
> Sent: Tuesday, June 27, 2023 6:15 PM
> To: Lu, Aaron <aaron.lu@intel.com>
> Cc: Deng, Pan <pan.deng@intel.com>; Chen, Tim C <tim.c.chen@intel.com>;
> vincent.guittot@linaro.org; linux-kernel@vger.kernel.org; Li, Tianyou
> <tianyou.li@intel.com>; Ma, Yu <yu.ma@intel.com>; Zhu, Lipeng
> <lipeng.zhu@intel.com>; Chen, Yu C <yu.c.chen@intel.com>; Tim Chen
> <tim.c.chen@linux.intel.com>
> Subject: Re: [PATCH v2] sched/task_group: Re-layout structure to reduce f=
alse
> sharing
>=20
> On Mon, Jun 26, 2023 at 01:47:56PM +0800, Aaron Lu wrote:
>=20
> > > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h index
> > > ec7b3e0a2b20..4fbd4b3a4bdd 100644
> > > --- a/kernel/sched/sched.h
> > > +++ b/kernel/sched/sched.h
> > > @@ -389,6 +389,19 @@ struct task_group {  #endif  #endif
> > >
> > > +	struct rcu_head		rcu;
> > > +	struct list_head	list;
> > > +
> > > +	struct list_head	siblings;
> > > +	struct list_head	children;
> > > +
> > > +	/*
> > > +	 * To reduce false sharing, current layout is optimized to make
> > > +	 * sure load_avg is in a different cacheline from parent, rt_se
> > > +	 * and rt_rq.
> > > +	 */
>=20
> That comment is misleading I think; you don't particularly care about tho=
se
> fields more than any other active fields that would cause false sharing.
>=20

How about this one:
	/*
	 * load_avg can also cause cacheline bouncing with parent, rt_se
	 * and rt_rq, current layout is optimized to make sure they are in
	 * different cachelines.
	 */

> > > +	struct task_group	*parent;
> > > +
> >
> > I wonder if we can simply do:
> >
> > diff --git a/kernel/sched/sched.h b/kernel/sched/sched.h index
> > ec7b3e0a2b20..31b73e8d9568 100644
> > --- a/kernel/sched/sched.h
> > +++ b/kernel/sched/sched.h
> > @@ -385,7 +385,9 @@ struct task_group {
> >  	 * it in its own cacheline separated from the fields above which
> >  	 * will also be accessed at each tick.
> >  	 */
> > -	atomic_long_t		load_avg ____cacheline_aligned;
> > +	struct {
> > +		atomic_long_t		load_avg;
> > +	} ____cacheline_aligned_in_smp;
> >  #endif
> >  #endif
> >
> > This way it can make sure there is no false sharing with load_avg no
> > matter how the layout of this structure changes in the future.
>=20
> This. Also, ISTR there was a series to split this atomic across nodes; wh=
atever
> happend to that, and can we still measure an improvement over this with t=
hat
> approach?

I just ran unixbench context-switching in 1 node with 40C/80T, without this=
 change
perf c2c data shows c2c bouncing is still there, perf record data shows set=
_task_cpu
takes ~4.5% overall cycles. With this change, that false-sharing is resolve=
d, and
set_task_cpu cycles drop to 0.5%.

Thanks
Pan