Received-SPF: pass (google.com: domain of linux-kernel+bounces-147529-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <c754bf86-2c3f-43fc-86d9-0cfd306b50da@amd.com>
Date: Tue, 16 Apr 2024 15:33:55 -0500
User-Agent: Mozilla Thunderbird
Reply-To: babu.moger@amd.com
Subject: Re: [PATCH v1 01/31] x86/resctrl: Fix allocation of cleanest CLOSID
 on platforms with no monitors
To: Dave Martin <Dave.Martin@arm.com>
Cc: James Morse <james.morse@arm.com>, x86@kernel.org,
 linux-kernel@vger.kernel.org, Fenghua Yu <fenghua.yu@intel.com>,
 Reinette Chatre <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>,
 shameerali.kolothum.thodi@huawei.com,
 D Scott Phillips OS <scott@os.amperecomputing.com>,
 carl@os.amperecomputing.com, lcherian@marvell.com,
 bobo.shaobowang@huawei.com, tan.shaopeng@fujitsu.com,
 baolin.wang@linux.alibaba.com, Jamie Iles <quic_jiles@quicinc.com>,
 Xin Hao <xhao@linux.alibaba.com>, peternewman@google.com,
 dfustini@baylibre.com, amitsinght@marvell.com,
 David Hildenbrand <david@redhat.com>, Rex Nie <rex.nie@jaguarmicro.com>
References: <20240321165106.31602-1-james.morse@arm.com>
 <20240321165106.31602-2-james.morse@arm.com>
 <36bdfb3c-2828-4188-88b1-b9d01b2a114f@amd.com>
 <Zh6jtYS0mu9xdwgS@e133380.arm.com>
Content-Language: en-US
From: "Moger, Babu" <babu.moger@amd.com>
In-Reply-To: <Zh6jtYS0mu9xdwgS@e133380.arm.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?T3lYTm9mZDVKMXZEOWhldHQyZ2NlWVBiaUhYQWNTeFBPVlE5SkxGK1l6Sjlw?=
 =?utf-8?B?SWkrTDl5QmFuZk5ZVXIyL3ptTmRnZXlKaG1ZZmJaamR2UFhQOWhHZnNBeXVz?=
 =?utf-8?B?TmY3azhIVXB2Z2VQU3hVR3AyMFVvbmIvL3U0dEFnbzZBRW8vOGt6cmZxbTQy?=
 =?utf-8?B?MFBKaHFwamcxUVpwcjlyWmpYelhYWnI3T25Yb3djUEZOZTRNNjQ3eGxBOEt0?=
 =?utf-8?B?Szgwa1Brc0lSR0VwU2FSRzdkL0MyTUkxL05IVFJHYlFubkJPSmtrb3E1Yytu?=
 =?utf-8?B?ak5WTHZ0Y1g5dUVZWlVJb1ByZ2p5S05zbWZkT3NkSHdZbXZHUzcxeHVzWUVt?=
 =?utf-8?B?U1NuRWhtVFRnb1RWZVd1WmVRUWFLRTdvZXpDUzdsTm8yQ0FaRjRCNEJCdFVL?=
 =?utf-8?B?bEswYlBzOXhqeldHTmxMSDdMUFJZaE0xTWpoMjNiTklmMEFzMVdKOWxOU0Fz?=
 =?utf-8?B?VVlIMHVQRG5lZFJ6cW5JTlhGWmYwVis1cldCYkxiVzJxOGpxd2YzRUkxendj?=
 =?utf-8?B?aFVGNFI4dFIxQm5EUjN1NTE1a2NLV0xRNzJJRE53SklqNGRITVAvakcxTCts?=
 =?utf-8?B?RnpRWjRNaThCb2pnOEFSaDVVd2ZtY2hBTjVwNkdjZU5RRENlR250dDh0MUZk?=
 =?utf-8?B?MHRqZTR1WWlzalZzRDFrNUovS0N5WWhibGtlb25haS9Ta1BLVG5vQnRtQ00y?=
 =?utf-8?B?ZjRXUWtDZWg4WVdkRVZBVDdWeVIzWmJsRG8xNWdRanQyVUtTc0QyN3BQSENq?=
 =?utf-8?B?QzE0Q0prQlo5R0ZMZEYwMnpoRDRIUXV1Q29rN3hVKy9qQUJBbzdQVDEwTE14?=
 =?utf-8?B?d0Z6cXdlTHBBTmZtNkJnTStld3Z3WGQ2S245ZEZFT0I3cWlETUJFSGtucm5Q?=
 =?utf-8?B?eHVSSU1ibUh2L0Rnc0FVa2puQ1FBL2tlMDhqK3BKeGtvTlB0cWhXRXI4QVNE?=
 =?utf-8?B?YjZnb3lTNUZ6KzRIMHhJakJkTEozVnhiY0YwM3Z1S25xbFFXWFRxMFEvUUJQ?=
 =?utf-8?B?b2JwV2RZdm5ZajlKenVQYWhNcFhlTXNiNno3M3lQSUNIenBSYlRsYnNWTDB2?=
 =?utf-8?B?N21kL2NpZnFqREZEdGNabWw0SmU3VnlQUVlTY1RwUVVwYWprOUw0azJFVWZt?=
 =?utf-8?B?MEg1akt2RC8vOWRUeUVVTVRHWGJKa3VtQ1N3bHdqWW1EYmpXYjZoUm83bTlk?=
 =?utf-8?B?UXlsR2RnY1hHaFNrZjd0STVLc1ZlVjdmT2VqUXowOFowdTd2Vkl3RVVDM1hZ?=
 =?utf-8?B?aHhPbm1xMHc5cXJRQVF5b0Y0dlhqV2hjTHNFc1Z3b3B4TGZSNFN4bG1HQUhI?=
 =?utf-8?B?U28vN1RRUVRFYnlQb2p3NFdvRGp4dDZ5S3d6VnBDV0Z6eHVCbEUrUkxXLzRS?=
 =?utf-8?B?NlZnbjJLanhWdXl6WGhHVVV0VksxUHdIMVRVNVB1NjNYMTdhNnhiMVFQcGU2?=
 =?utf-8?B?UStBQnpXQ01yRHB3MDd2dHhoMXhuV2lhekgyOENjaVBydGNVQ250WW43SlU5?=
 =?utf-8?B?eS9qK1NOV0tvZ2JyWVdoUFZNeVV6L3BmMGJocHBtbWNhakIzM1NnK1dtOWdO?=
 =?utf-8?B?TXFCQXp0Q2ZwTFBtSzBiRUcxQzhqVzgzcThOMVN6WE02WVA2L2lNMnJNUTk4?=
 =?utf-8?B?cGVITEQvNzM3MzlrOFJ3L2U1RWlHeXUrNFVPY0pxVXJiL3lRV2tLUzNVcWk1?=
 =?utf-8?B?WmFWdDRxbTMwcHZ4Z1VBUGV3RjFBWDdlbytnYW91Tit4VzNNd2p2cWxPNFh5?=
 =?utf-8?B?NDU5bXZGSlg5Y0pBQk1SaW4xRFNCaUdKV1gzZnUxaVBzbzFvdURpdlhHTDdZ?=
 =?utf-8?B?TlRYcUJjT1pwQm9idWI3NjRBOTNvSUtSdTZOTGZZNHV3Y1V3aWhvRVJSdVpm?=
 =?utf-8?B?WmdpTTRKdG1EM0ErZmtiVGJlRlNCOHRxUzRmbEdhQ1VEdHBMOWtCbzRQYmlh?=
 =?utf-8?B?UWR2NFhaVms3d0VKM1hjbG0yNGc5M01NVzdoM1ZwTmI0VFB1aDA2V3Zici81?=
 =?utf-8?B?WnJoUVE2bitnZFpsWENQaXNhbzNjdTllMS80K0FwMG9KNHV6ajFaeVNlK0FH?=
 =?utf-8?B?eHNkNHUxRERRczJSalNZcG13NU1sbUNVNEtpVkVRNDEvU25FVWRRZVgrNXpi?=
 =?utf-8?Q?endo=3D?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: e5af71bc-5a79-44fb-26d1-08dc5e548bd6
X-MS-Exchange-CrossTenant-AuthSource: MW3PR12MB4553.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 16 Apr 2024 20:33:59.7831
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: FOCM5v5fDcRS3rz0KlfU8XqFTBBPVW0bw6EYzj3J7RdxSMuf0cnt0pYHcosoPiYS
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR12MB6862

Hi Dave,

On 4/16/24 11:13, Dave Martin wrote:
> On Mon, Apr 15, 2024 at 03:27:42PM -0500, Moger, Babu wrote:
>>
>>
>> On 3/21/24 11:50, James Morse wrote:
>>> commit 6eac36bb9eb0 ("x86/resctrl: Allocate the cleanest CLOSID by
>>> searching closid_num_dirty_rmid") added a Kconfig option that causes
>>
>> This is not true. The Kconfig option is never added. It is added later in
>> the series. There is also comment
>> on this https://lore.kernel.org/lkml/ZhecyLQsGZ9Iv8wU@gmail.com/
>>
>>
>> Shouldn't the Kconfig option added first before doing this change?
>>
>>> resctrl to search for the CLOSID with the fewest dirty cache lines when
>>> creating a new control group. This depends on the values read from the
>>> llc_occupancy counters.
> 
> See David's comments and previous discussion on this patch.
> 
> You're right to point out that the description of the original commit
> does seem a bit garbled though: CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID is
> not present in Kconfig here, but already referenced by other code.
> 
> We seem to have a consensus that it's OK to have a dangling IS_ENABLED()
> so long as the option is added formally to Kconfig later, but it looks
> like the commit message here should be reworded.
> 
> Does the following make sense?
> 
> --8<--
> 
> commit 6eac36bb9eb0 ("x86/resctrl: Allocate the cleanest CLOSID by
> searching closid_num_dirty_rmid") added logic that causes resctrl to
> search for the CLOSID with the fewest dirty cache lines when creating a
> new control group, if requested by the arch code. This depends on the
> values read from the llc_occupancy counters. The logic is applicable to
> architectures where the CLOSID effectively forms part of the monitoring
> identifier and so do not allow complete freedom to choose an unused
> monitoring identifier for a given CLOSID.
> 
> -->8--

That looks good. Thanks

> 
> 
> Although it would probably have been better if the Kconfig option had
> been added upstream, this patch does not create that that situation and
> the series (taken as a whole) resolves it.
> 
> So I am not sure that anything would be solved or improved by changing
> the body of this patch, but if people still have concerns then I guess
> we can look at it.

> 
> 
>>>
>>> This support missed that some platforms may not have these counters.
>>> This causes a NULL pointer dereference when creating a new control
>>> group as the array was not allocated by dom_data_init().
>>>
>>> As this feature isn't necessary on platforms that don't have cache
>>> occupancy monitors, add this to the check that occurs when a new
>>> control group is allocated.
>>>
>>> The existing code is not selected by any upstream platform, it makes
>>> no sense to backport this patch to stable.
>>>
>>> Signed-off-by: James Morse <james.morse@arm.com>
>>> ---
>>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> index 011e17efb1a6..1767c1affa60 100644
>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>>> @@ -149,7 +149,8 @@ static int closid_alloc(void)
>>>  
>>>  	lockdep_assert_held(&rdtgroup_mutex);
>>>  
>>> -	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
>>> +	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID) &&
>>> +	    is_llc_occupancy_enabled()) {
>>>  		cleanest_closid = resctrl_find_cleanest_closid();
>>>  		if (cleanest_closid < 0)
>>>  			return cleanest_closid;
>>
>> -- 
>> Thanks
>> Babu Moger
>>
> 
> [...]
> 
> Cheers
> ---Dave

-- 
Thanks
Babu Moger