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:   "Yu, Fenghua" <fenghua.yu@intel.com>
To:     James Morse <james.morse@arm.com>,
        "x86@kernel.org" <x86@kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
CC:     "Chatre, Reinette" <reinette.chatre@intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        H Peter Anvin <hpa@zytor.com>,
        Babu Moger <Babu.Moger@amd.com>,
        "shameerali.kolothum.thodi@huawei.com" 
        <shameerali.kolothum.thodi@huawei.com>,
        D Scott Phillips OS <scott@os.amperecomputing.com>,
        "carl@os.amperecomputing.com" <carl@os.amperecomputing.com>,
        "lcherian@marvell.com" <lcherian@marvell.com>,
        "bobo.shaobowang@huawei.com" <bobo.shaobowang@huawei.com>,
        "tan.shaopeng@fujitsu.com" <tan.shaopeng@fujitsu.com>,
        "xingxin.hx@openanolis.org" <xingxin.hx@openanolis.org>,
        "baolin.wang@linux.alibaba.com" <baolin.wang@linux.alibaba.com>,
        Jamie Iles <quic_jiles@quicinc.com>,
        Xin Hao <xhao@linux.alibaba.com>,
        "peternewman@google.com" <peternewman@google.com>
Subject: RE: [PATCH v2 06/18] x86/resctrl: Allow the allocator to check if a
 CLOSID can allocate clean RMID
Thread-Topic: [PATCH v2 06/18] x86/resctrl: Allow the allocator to check if a
 CLOSID can allocate clean RMID
Thread-Index: AQHZJ3hTfsHcKvV5QEKiMypaOmSjQa6hMmlw
Date:   Tue, 17 Jan 2023 18:29:01 +0000
Message-ID: <IA1PR11MB609729461B9FF9F9BFD1DEC99BC69@IA1PR11MB6097.namprd11.prod.outlook.com>
References: <20230113175459.14825-1-james.morse@arm.com>
 <20230113175459.14825-7-james.morse@arm.com>
In-Reply-To: <20230113175459.14825-7-james.morse@arm.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?/iBxW+s2gmwDXd3qIEZprfpyT0/n62HZKmf9u7P13jCc175rND4EqR4cscnG?=
 =?us-ascii?Q?WdEAgEpVQa0jVLK6/fRFbSYurtU8DznX9ZKxr2sQ0FTLAxe9VNcdz4Zq9nLB?=
 =?us-ascii?Q?AlUYnOm1JsX5NfYxkbhZjm6yppu33APZP7YMZeHlDJihX4sDdpLsqGEHHOWL?=
 =?us-ascii?Q?5tyTbkqhydlGbT5yxftA1kp3gsyH7Vf8gSqqwTNLD1Csu+f53Chd+JxYd9s7?=
 =?us-ascii?Q?FFRhxvhnb3GQc77jRM9XACCvuoABH0WY6QPxh3fZov3nlU/K2kPY6uaK5EqC?=
 =?us-ascii?Q?8WZJnmghnrenbhSY+I4ZkHu5rM6W8L2G7pjswG7Dt7Dm5u3+tkpZZIub5rVH?=
 =?us-ascii?Q?IotZWi8FPLfODp0q/2oqSHoNaiUM0aRv3alJnOR6bxiTB/L6R72/n5WEOBWv?=
 =?us-ascii?Q?hKPpteYlrn6wKKif2/n/yspixJt/w8DhW0TGFLWj++rfRGFBO0Iefjpx/se1?=
 =?us-ascii?Q?F9WxvZ9Ar3zcXue+n0qt4JJ0bJ2iH9LzvAIPRA6hlgD88qWkvmPSMCWoZq2P?=
 =?us-ascii?Q?ugD36EZKvRhbBG1+LsbQlTd+zrGMF8a1c0fQWvpIxYft0ZAR/L4hd66/jNcQ?=
 =?us-ascii?Q?HuLI+W6d1X1hYo2rd0DUuZsDM1jDt6rr2MVhs8E8Y/3ct0STXmRaUOjE5kos?=
 =?us-ascii?Q?yRMHF1z/gscAtX7eqogLs2GmPm5Xx3u2yGuM8mncFtYUKVBh+Ee0VcdNBBk3?=
 =?us-ascii?Q?d21oC3UVDGHip35GufP7AYbcTQmTFIqlJ7q0cLHRf2zSo81tJeuGTLFJFWkj?=
 =?us-ascii?Q?uLfxp62fiT5rTS5hNBsWjsWZvlYr1ibbmYJwlFvghNjLQ7YnuP7aWzQsjZm0?=
 =?us-ascii?Q?R2aVAalalZn2NaeYfI6Y61XKHKPQQU7L6Y9DeNkzCrI3YL7elflTV3zur6IK?=
 =?us-ascii?Q?R211HF2/GkCbmBdvwAWyMt8sNwJ6lQC9bfHmAuq9+ora/Z2I4yHXr8B9h3R3?=
 =?us-ascii?Q?X7f5UlJQAV/HM1Fb/0fx5pWrikG2ZkncdRu3A47io2zdz0gKcEe4R4A9BSRK?=
 =?us-ascii?Q?FoAlm/DJxStYgFmo5v1QSV2MyOm5JdAyxoler6jGZRpMTF/XigKqvsA3oR7t?=
 =?us-ascii?Q?n3Fe49HlP8lGewYu0O0vloKoAcWArS87k3Ok0DpRPDJGcNWVzVu9P3gZ11So?=
 =?us-ascii?Q?q7ajrhe/rWNO+FHp3WJyHpXlOOAqB0R330h199imC2Ar4E6vAt4VLUVBlGKB?=
 =?us-ascii?Q?dE17bf1BXrxnnZDWE980G0Wmt5C+z2nS3Y059nnYa/8Og1JZguyUNSm0nWNe?=
 =?us-ascii?Q?SXGKOSzxZNTdWWbspzNm7PhonuoZ+UTSp0LF0YAPOJzqjJeGRwsxFPYNTMk0?=
 =?us-ascii?Q?zDCiSe0UqOB5kkz11gKQVNQtDYb2rmm6nYW1rp//Yo2Gxs9l03HYWxMPLOS7?=
 =?us-ascii?Q?XkWZfw7s+pRiTQQZDVO28+M8smJF2JT1oA6ZUoHsnPkZPnX2IGshkPISNN/r?=
 =?us-ascii?Q?MXV5hiDLdfTJstvDJdwZRUmycNnNbOMh8WLgQGx5jZ1lf9BZrBtnK5+9ub0A?=
 =?us-ascii?Q?1XR7ppfDwoRPTgCaHonOWzfra/lA4HXQW/cO5MAQyQFf5DcCJEwgChIlUIGt?=
 =?us-ascii?Q?dt7R7rtEYVpgE9Nk+Y6cM6QTg8d1zwD96KSqJ27x?=
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: IA1PR11MB6097.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 1e64bdfa-0314-4bfd-bfef-08daf8b8b499
X-MS-Exchange-CrossTenant-originalarrivaltime: 17 Jan 2023 18:29:01.3523
 (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: QLW1tB5hAF3D6S3thskpc7/gQOuRBSGNSSudNvBuvnq2XYH0bcDzWdiMg4QKOMO03Z5g3KtSVQmZGDE8OZqOtA==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB7541
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 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

Hi, James,

> MPAM's PMG bits extend its PARTID space, meaning the same PMG value can b=
e
> used for different control groups.
>=20
> This means once a CLOSID is allocated, all its monitoring ids may still b=
e dirty,
> and held in limbo.
>=20
> Add a helper to allow the CLOSID allocator to check if a CLOSID has dirty=
 RMID
> values. This behaviour is enabled by a kconfig option selected by the
> architecture, which avoids a pointless search for x86.
>=20
> Tested-by: Shaopeng Tan <tan.shaopeng@fujitsu.com>
> Signed-off-by: James Morse <james.morse@arm.com>
>=20
> ---
> Changes since v1:
>  * Removed superflous IS_ENABLED().
> ---
>  arch/x86/kernel/cpu/resctrl/internal.h |  1 +
> arch/x86/kernel/cpu/resctrl/monitor.c  | 31 ++++++++++++++++++++++++++
> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 17 ++++++++------
>  3 files changed, 42 insertions(+), 7 deletions(-)
>=20
> diff --git a/arch/x86/kernel/cpu/resctrl/internal.h
> b/arch/x86/kernel/cpu/resctrl/internal.h
> index 013c8fc9fd28..adae6231324f 100644
> --- a/arch/x86/kernel/cpu/resctrl/internal.h
> +++ b/arch/x86/kernel/cpu/resctrl/internal.h
> @@ -509,6 +509,7 @@ int rdtgroup_pseudo_lock_create(struct rdtgroup
> *rdtgrp);  void rdtgroup_pseudo_lock_remove(struct rdtgroup *rdtgrp);  st=
ruct
> rdt_domain *get_domain_from_cpu(int cpu, struct rdt_resource *r);  int
> closids_supported(void);
> +bool resctrl_closid_is_dirty(u32 closid);
>  void closid_free(int closid);
>  int alloc_rmid(u32 closid);
>  void free_rmid(u32 closid, u32 rmid);
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c
> b/arch/x86/kernel/cpu/resctrl/monitor.c
> index 347be3767241..190ac183139e 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -327,6 +327,37 @@ static struct rmid_entry *resctrl_find_free_rmid(u32
> closid)
>  	return ERR_PTR(-ENOSPC);
>  }
>=20
> +/**
> + * resctrl_closid_is_dirty - Determine if clean RMID can be allocate for=
 this

s/allocate/allocated/

> + *                           CLOSID.
> + * @closid: The CLOSID that is being queried.
> + *
> + * MPAM's equivalent of RMID are per-CLOSID, meaning a freshly allocate

s/allocate/allocated/

> +CLOSID
> + * may not be able to allocate clean RMID. To avoid this the allocator
> +will
> + * only return clean CLOSID.
> + */
> +bool resctrl_closid_is_dirty(u32 closid) {
> +	struct rmid_entry *entry;
> +	int i;
> +
> +	lockdep_assert_held(&rdtgroup_mutex);

It's better to move lockdep_asser_held() after if (!IS_ENABLE()).
Then compiler might optimize this function to empty on X86.

> +
> +	if (!IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID))
> +		return false;
> +
> +	for (i =3D 0; i < resctrl_arch_system_num_rmid_idx(); i++) {
> +		entry =3D &rmid_ptrs[i];
> +		if (entry->closid !=3D closid)
> +			continue;
> +
> +		if (entry->busy)
> +			return true;
> +	}
> +
> +	return false;
> +}
> +
>  /*
>   * As of now the RMIDs allocation is the same in each domain.
>   * However we keep track of which packages the RMIDs diff --git
> a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index ac88610a6946..e1f879e13823 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -93,7 +93,7 @@ void rdt_last_cmd_printf(const char *fmt, ...)
>   * - Our choices on how to configure each resource become progressively =
more
>   *   limited as the number of resources grows.
>   */
> -static int closid_free_map;
> +static unsigned long closid_free_map;
>  static int closid_free_map_len;
>=20
>  int closids_supported(void)
> @@ -119,14 +119,17 @@ static void closid_init(void)
>=20
>  static int closid_alloc(void)
>  {
> -	u32 closid =3D ffs(closid_free_map);
> +	u32 closid;
>=20
> -	if (closid =3D=3D 0)
> -		return -ENOSPC;
> -	closid--;
> -	closid_free_map &=3D ~(1 << closid);
> +	for_each_set_bit(closid, &closid_free_map, closid_free_map_len) {
> +		if (resctrl_closid_is_dirty(closid))
> +			continue;
>=20
> -	return closid;
> +		clear_bit(closid, &closid_free_map);
> +		return closid;
> +	}
> +
> +	return -ENOSPC;
>  }
>=20
>  void closid_free(int closid)
> --
> 2.30.2

Thanks.

-Fenghua