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;
Message-ID: <54538ac1-c700-bd0b-2a1d-bcbe84067a0c@intel.com>
Date:   Fri, 5 May 2023 16:19:29 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.10.1
Subject: Re: [RFC PATCH 3/7] x86/resctrl: Add driver callback when directories
 are removed
Content-Language: en-US
To:     Tony Luck <tony.luck@intel.com>, Fenghua Yu <fenghua.yu@intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>,
        "H. Peter Anvin" <hpa@zytor.com>,
        Shaopeng Tan <tan.shaopeng@fujitsu.com>,
        Jamie Iles <quic_jiles@quicinc.com>,
        James Morse <james.morse@arm.com>,
        "Babu Moger" <babu.moger@amd.com>
CC:     <x86@kernel.org>, <linux-kernel@vger.kernel.org>
References: <20230420220636.53527-1-tony.luck@intel.com>
 <20230420220636.53527-4-tony.luck@intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20230420220636.53527-4-tony.luck@intel.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?ZmNkcXRoMU5jNTd0QkdNYmxPY2hVM2U4S3IzVEM1dXhMMU1SeUtyL0d2K0Y1?=
 =?utf-8?B?VmR1TitlY3RETGhVaEVCdkMxUzZacm50M0ZCVFNtMGI1MGl4TmxCbzlZOWpJ?=
 =?utf-8?B?TStMS2VEUndXTnl3T0hHc1ZIcWhPVnNQeERmOGEyM1dubVJ0Y3RacjlscTQr?=
 =?utf-8?B?ZDRTZzBXVzVnTndrVml2RlpTdWIwS2JMUWxzL3VSUm5FZXpVak1oK01TemNH?=
 =?utf-8?B?UzZYcDNmQldtNnlpUTIzckxoc2xaeDdmdmN2TGNtODVMYWo1NkNqbGh4NHNv?=
 =?utf-8?B?YkFqdkJmazFkM0ZMaFZKdmNmWDVYTGQ2SVlyYkNmSGNIOXRGVnVod0RjaVlt?=
 =?utf-8?B?N2lXdHp0bmtsTE1OajljQzdYcUVjY2o2eW1WbE5XVG9tcDgySkVOQmVYSVdY?=
 =?utf-8?B?ZU4zL2hpdGFzZVpvUTNCNi9HTTRXaTNJTkN5bTIyZmJBUGc4TUl1VS9kand2?=
 =?utf-8?B?VElweDlFc2RCOFJTait4cTF5MlNheG9rOUtVMmFtNzFlSmMvLzZkMzJUY3Iv?=
 =?utf-8?B?OUo0RHEvZlFpY2RZcTNXMDRyaitodUxzV3QxL0I3RERCZ29zY0xNZ0NNQlZh?=
 =?utf-8?B?OHE1OUt0NS81dG4xRXZEWnpxTTBOZ1hicGZpazVDMFY2Y1NRQVB2eXJPV1lF?=
 =?utf-8?B?SGlJWlBvcTZoOTFrWTJqN1U3OHNXcytxZEdQa2xjeTNIZUJPNGRvcFpzajBX?=
 =?utf-8?B?NzFxNmxCeUhwdWZCazM5MXJ5Mkg2OUlQc2R5cHRzMlhDSzlNUjA0YmdIQmhM?=
 =?utf-8?B?SmVUc2xoNk5NVmpxTlE2Z0N6anMvdkFKQ2RkMDlVS0pwN0FDR3d2NHdQcWRM?=
 =?utf-8?B?V0txZkRXRzh6UVFvZ0g4ZXJrN3hTWlc0SndYeG1IK2w0VjU3bWdKcHJuR253?=
 =?utf-8?B?WmROalNUTkcvckJIZ1owQzZxQVRKNXhjaGhVMFo2NUk4SHkxMk54WHkzdmVw?=
 =?utf-8?B?cVNIb1B4S0QzQSs2cHlKS25xNUNLSnhhRlJieXI3cEJFQ2ErbU1YeUFjM0pk?=
 =?utf-8?B?cFhuLzF2STg3d1VLaGViaGRSNEVTMnEzbWxpV2JZeHB2OENWb3RQQjY2MDE4?=
 =?utf-8?B?dE5DK3N2a2h3Z1pXS1d6WEcwTUUveEtGemNMalIrYVRTcHREalFVejB1WGRY?=
 =?utf-8?B?YnN1L01CZmZvT281NEUwVmgzWGJhU0VLU0gwMFBXczZRV1h1N0loTEVTWm92?=
 =?utf-8?B?QWtLc2JuWmlRVTNpVU9Ubk03VzQ5Z21jL2drdHBIRy9WOHBGa3BoRWhseEpP?=
 =?utf-8?B?MWdVc2haR0tJZytVVUo2Umt5T0RLUTVBZzFLdDhCWDd5K0FacmFwdWIwQUxX?=
 =?utf-8?B?N2Q3UkZzc2QzMFpEYkVDc2RZd3ZEMUZpaEE3YWNLUFY2Y0VjT2pMcXlNRG41?=
 =?utf-8?B?Uk0yL1ZWNGJyZFdHVlQvYTdwQ2J6eXBQYXJKNUsxclZMREREVGtJNk82TzF4?=
 =?utf-8?B?Rm1Wc0h2QUJzMlFwc2xlaGlXMUhtZzRJRG45RzdhSXNFMEpxd0tGMkE3SDlq?=
 =?utf-8?B?RzcvV3NsQTk4cmp4RlhBbXVqSUJBYjQrQTZrdmMzdkdsblNPelZEVUhqUkh6?=
 =?utf-8?B?Q1hkbXc3Q2pzcndFVHVCUWNJcExMUk41eEZ3cFR3VWErRnhDWXprdVM0SllM?=
 =?utf-8?B?bWkvVEp0aENaOFpqZ0RTVC8xTVE5U0NZMjZsbW43UWgxdm1hY0hSWGhjV1Rw?=
 =?utf-8?B?TjZaMk1vNnVqbEJGd0RxTGpXOHhESjN6bzdCSnhiUnBMeDRQbFFHVzBrNm1R?=
 =?utf-8?B?SEx2MGJ5RmNPYWQ1a2JocmlacHdUdWhtRzE1b25uN3Y5UUZmc2lxTzZhdVFu?=
 =?utf-8?B?cmlOTDA5WVQ0dlliYXYzZkpYc2FndmtyUUJabm40cXdQd0Fud0lNYTdnc0l3?=
 =?utf-8?B?TmVFaGU2bllVRGkyOTNWdTd3Y1ZqM1UzL1hlR0xxaTVodGZMZXY3RDg4bHlo?=
 =?utf-8?B?cVhMZDlwN0hXNEN2V3U4bXMySDdnZVRMeitoWkYyNW9TNTBWUjZ4aVdKY1lS?=
 =?utf-8?B?T3ZQdWtlNC9VK1JiMExNd0pMY3VXT3FObStFcWNIVVc5b2Z2dHdUVE1LVUtJ?=
 =?utf-8?B?NDhHN1BZRWdmQm1pellhWDVLbXZ5M0NCeW1YK0lUVHo3TGNBTzNhQU9RR0th?=
 =?utf-8?B?UEdVbS8vZjZVYzdyaVdEM0hvVTZXa05KNzZpcWVYaWFZSVI4aDdnRXE1ei9O?=
 =?utf-8?B?VXc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 4187b0a8-ba4a-4eaf-1823-08db4dbf2ead
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 05 May 2023 23:19:32.0989
 (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: Ok1fYJ9nt1A5FRKDh1REAVRHoXQS5LBCfvU0XoUucYd78RQrGRVbc6CPQgOQ5kbCAWpuWjzRL5wLkujKQsLAKFB8tH5L8zsNKy/3vq0RnyY=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BL1PR11MB5953
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-8.8 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,
        SPF_NONE,T_SCC_BODY_TEXT_LINE 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 Tony,

On 4/20/2023 3:06 PM, Tony Luck wrote:
> When a resctrl directory is removed, entities attached to that
> directory are reassigned with the CLOSID and RMID of the parent
> directory.

Could you please elaborate what you mean with "entities"? In
resctrl there are tasks needing to move but that is done in resctrl.
Would drivers be doing any manipulation of the tasks' closid/rmid?

> 
> Add a callback function so a driver can reset the CLOSID and RMID
> of any resources attached to the removed directory.

I expect this behavior to be different between the removal of
a monitoring group vs a control group (more later) ...


> @@ -3495,11 +3495,18 @@ static int rdtgroup_mkdir(struct kernfs_node *parent_kn, const char *name,
>  static int rdtgroup_rmdir_mon(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
>  {
>  	struct rdtgroup *prdtgrp = rdtgrp->mon.parent;
> +	struct resctrl_driver *d;
>  	int cpu;
>  
>  	/* Give any tasks back to the parent group */
>  	rdt_move_group_tasks(rdtgrp, prdtgrp, tmpmask);
>  
> +	list_for_each_entry(d, &drivers, list) {
> +		if (d->rmdir)
> +			d->rmdir(rdtgrp->closid, rdtgrp->mon.rmid,
> +				 prdtgrp->closid, prdtgrp->mon.rmid);
> +	}
> +

This seems tricky ... if I understand correctly the API provides
limited properties directly to driver to keep things safe but at the
same time the properties need to accommodate all driver usages. All drivers
would need an update if something new is needed.

For example, this seems to ignore whether a resource group is exclusive
or not - could group removal trigger behavior that can circumvent this
restriction?

>  	/* Update per cpu rmid of the moved CPUs first */
>  	for_each_cpu(cpu, &rdtgrp->cpu_mask)
>  		per_cpu(pqr_state.default_rmid, cpu) = prdtgrp->mon.rmid;
> @@ -3535,6 +3542,7 @@ static int rdtgroup_ctrl_remove(struct rdtgroup *rdtgrp)
>  
>  static int rdtgroup_rmdir_ctrl(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
>  {
> +	struct resctrl_driver *d;
>  	int cpu;
>  
>  	/* Give any tasks back to the default group */
> @@ -3544,6 +3552,11 @@ static int rdtgroup_rmdir_ctrl(struct rdtgroup *rdtgrp, cpumask_var_t tmpmask)
>  	cpumask_or(&rdtgroup_default.cpu_mask,
>  		   &rdtgroup_default.cpu_mask, &rdtgrp->cpu_mask);
>  
> +	list_for_each_entry(d, &drivers, list) {
> +		if (d->rmdir)
> +			d->rmdir(rdtgrp->closid, rdtgrp->mon.rmid, 0, 0);
> +	}
> +
>  	/* Update per cpu closid and rmid of the moved CPUs first */
>  	for_each_cpu(cpu, &rdtgrp->cpu_mask) {
>  		per_cpu(pqr_state.default_closid, cpu) = rdtgroup_default.closid;

... is the expectation that the driver would look at the latter parameters to
determine if a monitor or control group is removed? I think that may be troublesome
since API like this relies on driver needing to have a lot of insight into
internal implementation choices of resctrl (which could potentially change?).
If I understand correctly the driver makes assumptions like: (a) default control group
always is assigned closid 0 and rmid 0 and (b) default control group is never removed.
I think that it should not be required for drivers to have knowledge like this
(and then also risk that these assumptions may change).

Reinette