From:   Shinichiro Kawasaki <shinichiro.kawasaki@wdc.com>
To:     Vincent Donnefort <vincent.donnefort@arm.com>
CC:     "peterz@infradead.org" <peterz@infradead.org>,
        "mingo@redhat.com" <mingo@redhat.com>,
        "vincent.guittot@linaro.org" <vincent.guittot@linaro.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "dietmar.eggemann@arm.com" <dietmar.eggemann@arm.com>,
        "valentin.schneider@arm.com" <valentin.schneider@arm.com>,
        "jing-ting.wu@mediatek.com" <jing-ting.wu@mediatek.com>,
        Damien Le Moal <Damien.LeMoal@wdc.com>
Subject: Re: [PATCH] sched/core: Mitigate race
 cpus_share_cache()/update_top_cache_domain()
Thread-Topic: [PATCH] sched/core: Mitigate race
 cpus_share_cache()/update_top_cache_domain()
Thread-Index: AQHX2g37TPTJ8je3+U2WwmDdbyK6Nw==
Date:   Mon, 15 Nov 2021 10:46:02 +0000
Message-ID: <20211115104601.lwcwxoc67ftbf6uh@shindev>
References: <20211104175120.857087-1-vincent.donnefort@arm.com>
In-Reply-To: <20211104175120.857087-1-vincent.donnefort@arm.com>
Accept-Language: en-US
Content-Language: en-US
wdcipoutbound: EOP-TRUE
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?uWc9SXIsaZVzWWTKZBBrSQrlPuZD7J8UlXQ6o+E2V2+d6yj78deyiijYqVfb?=
 =?us-ascii?Q?yNqEzxR9yftqJ0+i7PKCBaxJ1w8f1HGp+JG4B8qKX3/5V54M6BObFVXXk2Zy?=
 =?us-ascii?Q?j3FbS/hwHjfnQZnscJZjXdecRpbzUO1/NIhIjKakomVSg3PSaTScLWK1Jm/c?=
 =?us-ascii?Q?rRES5WlSHuD7Rt3M/H/0RnxAiZZlEmiK6c0oZFvczxh/YjKzv89AwwZXQHfz?=
 =?us-ascii?Q?GXC65A5KHuR0SR5jOICOnLykgllAnlO2QlXsNJlLZwaAgqZx0wYObVu9JEYZ?=
 =?us-ascii?Q?iS9Brhwsxgbx/9VFM0jB0GOK9V/Ue67ly0udNBm92kyM6kxF6PXKrcOKNbbR?=
 =?us-ascii?Q?5MHUhdGxtCERAjW0TYz9v83fXDK8tpwICxeIfcNwUlZZhaYZZZKy0RXu76zU?=
 =?us-ascii?Q?7+9KzVeAFzKI+pCJAvn27+iYrvWr0ovMTZxFJH+bwAqxP9HGbW0MTMz9vkG4?=
 =?us-ascii?Q?8WcmZGBaQQY4ncYatrZgfceubiXKZyt/66WCLYcU+aFUaScrBdjxdqfnt99x?=
 =?us-ascii?Q?uzWuSZaRHYI2L89jvPqjFZLy6WXaU6+YkqPoa2Q+kqx48V1Vi/L/1xDYWaHj?=
 =?us-ascii?Q?MxVSgzuiDHafc8WQ5CWwWMZzfcu52Ww1yPO8omjwXcz6ydkxCfUePPZuqV3w?=
 =?us-ascii?Q?l6/MaS7DFCHQu+69f2ImYpQ60etLM3inCXzpARTGq1U7EPqes+8GS1K7pwo5?=
 =?us-ascii?Q?MbyVaR4nhqdRne37Xx8QXYGViQZzgj1wFVG4mgao91jOm/OZDc92ZPVxTF3d?=
 =?us-ascii?Q?sNsQkvN5QExaP5S5ynJtizhMKozNLzqdctzu2u1uZzfhi+T+hjPc8heiDVzy?=
 =?us-ascii?Q?I5HqzY+sbqBc7ywmaf6ZgXl8qgTEfGVxUnvKm42l+QoxR8qzqV5uBkOWKW2p?=
 =?us-ascii?Q?JTmYWQVBoXDd81y/rHlc383lksmzlwjtkIU++EO5QUBD87qySXO4GF/mvmBS?=
 =?us-ascii?Q?fG/wyJpc8CyIkPZBjOxRE8LckkfYtIxrxQaqUgIMrOSp+Fd2iTu0/En1Xgng?=
 =?us-ascii?Q?EHA3kT9uGzxH+AHc1c4CbjaQN67mTuILDXyKaJEpU8hcW8kkhBS5RU0iXewd?=
 =?us-ascii?Q?RyCGILuABI+1Htim3L/ssSB8a7ffOhZPs+elQyTwfeP3U8Rl7qFLSNO0WgkH?=
 =?us-ascii?Q?cUldMxzN0FxozmruLLRSTj64LbLlps+ThYHm4lCoINORGVGAIF8ZMTfi1Br3?=
 =?us-ascii?Q?MbzKeGcSkqSfmAEHjoMNO5B2tJ3k6xoiLg3y8gzqUdn87W7IdCurVjttKCMt?=
 =?us-ascii?Q?/fJDqRyYWZtjKSRME/4hHN7Qe1ZvtHyiH6eboFlDa91jM3e+zQduqi0LmLkl?=
 =?us-ascii?Q?+TIkJ7ICBqjKOqi6QgvyXCjKJMJiO8YHf2baBS6xHeiIvvYOa1SRyWcktIVo?=
 =?us-ascii?Q?hQMIR5jCMa3fSTkB4Zs116VGsQut2jlR3O2LDmFlo2VI/OCesb/lG5MJIDSb?=
 =?us-ascii?Q?lO/15XZJvGNzhWcn21/ewJ+JCQhhey1itqF0fNrBMNtPcWyOTTLoxsr4YeRa?=
 =?us-ascii?Q?uzWmHuJZrThLoww0c6UGg3z2IComD8utEjOkRT/9Z5SnSg0F2DZ6wSXmsQWT?=
 =?us-ascii?Q?i8WbSeFp1a3Cq0kEQITVHd9QkCiWCfUJZRoSzlU2gUBP4WeFHZoEj6OcDWJM?=
 =?us-ascii?Q?v+uZ196Nn0IHhAUjl6HA6KbOoaPYrfT5tQ9mxS3nVqGSwcfgYZ3yZyQNv3Iv?=
 =?us-ascii?Q?FIIDGFwryvgWA19AyMNfXmbyAYA=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1181AA8717D3654ABD48B071D5CFE5CC@namprd04.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: wdc.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR04MB7184.namprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a9cff36f-010b-46f0-378c-08d9a8251e63
X-MS-Exchange-CrossTenant-originalarrivaltime: 15 Nov 2021 10:46:02.5366
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b61c8803-16f3-4c35-9b17-6f65f441df86
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: SiNKS8pCSKbG5daB+zCimguHO3BlankJkLgqeplbtNcBNlgVGcF8m/tRK8OaSpGXnc59M5HbZ8FLnCPh5/39KNPq/KvfUZA4/Jp52BD57qc=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR04MB4295
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Nov 04, 2021 / 17:51, Vincent Donnefort wrote:
> Nothing protects the access to the per_cpu variable sd_llc_id. When testi=
ng
> the same CPU (i.e. this_cpu =3D=3D that_cpu), a race condition exists wit=
h
> update_top_cache_domain(). One scenario being:
>=20
>               CPU1                            CPU2
>   =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
>=20
>   per_cpu(sd_llc_id, CPUX) =3D> 0
>                                     partition_sched_domains_locked()
>       				      detach_destroy_domains()
>   cpus_share_cache(CPUX, CPUX)          update_top_cache_domain(CPUX)
>     per_cpu(sd_llc_id, CPUX) =3D> 0
>                                           per_cpu(sd_llc_id, CPUX) =3D CP=
UX
>     per_cpu(sd_llc_id, CPUX) =3D> CPUX
>     return false
>=20
> ttwu_queue_cond() wouldn't catch smp_processor_id() =3D=3D cpu and the re=
sult
> is a warning triggered from ttwu_queue_wakelist().
>=20
> Avoid a such race in cpus_share_cache() by always returning true when
> this_cpu =3D=3D that_cpu.
>=20
> Fixes: 518cd6234178 ("sched: Only queue remote wakeups when crossing cach=
e boundaries")
> Reported-by: Jing-Ting Wu <jing-ting.wu@mediatek.com>
> Signed-off-by: Vincent Donnefort <vincent.donnefort@arm.com>
> Reviewed-by: Valentin Schneider <valentin.schneider@arm.com>
>=20
> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index f2611b9cf503..f5ca15cdcff4 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -3726,6 +3726,9 @@ void wake_up_if_idle(int cpu)
> =20
>  bool cpus_share_cache(int this_cpu, int that_cpu)
>  {
> +	if (this_cpu =3D=3D that_cpu)
> +		return true;
> +
>  	return per_cpu(sd_llc_id, this_cpu) =3D=3D per_cpu(sd_llc_id, that_cpu)=
;
>  }
> =20
> --=20
> 2.25.1
>=20

Oh, this is the exactly same fix as I posted before [1]. It is a little bit=
 sad
that my post did not get reviewed. Anyway, good to see the issue fixed. Tha=
nks.

[1] https://lore.kernel.org/all/20211029005618.773579-1-shinichiro.kawasaki=
@wdc.com/

--=20
Best Regards,
Shin'ichiro Kawasaki=