Message-ID: <af4937a1-62b5-f27f-fd83-5f69f37e8241@intel.com>
Date:   Fri, 10 Mar 2023 12:00:02 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.7.2
Subject: Re: [PATCH v2 07/18] x86/resctrl: Move CLOSID/RMID matching and
 setting to use helpers
Content-Language: en-US
To:     James Morse <james.morse@arm.com>, <x86@kernel.org>,
        <linux-kernel@vger.kernel.org>
CC:     Fenghua Yu <fenghua.yu@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>,
        D Scott Phillips OS <scott@os.amperecomputing.com>,
        <carl@os.amperecomputing.com>, <lcherian@marvell.com>,
        <bobo.shaobowang@huawei.com>, <tan.shaopeng@fujitsu.com>,
        <xingxin.hx@openanolis.org>, <baolin.wang@linux.alibaba.com>,
        Jamie Iles <quic_jiles@quicinc.com>,
        Xin Hao <xhao@linux.alibaba.com>, <peternewman@google.com>
References: <20230113175459.14825-1-james.morse@arm.com>
 <20230113175459.14825-8-james.morse@arm.com>
 <678c5d45-eb91-c000-76d6-7ed9341e52c3@intel.com>
 <4002faaa-d93a-0d9b-0234-05bfae3d7b93@arm.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <4002faaa-d93a-0d9b-0234-05bfae3d7b93@arm.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?c3pFZzN3OC9oT3Bwa0NqTGJ1QVR0eGdoR1JpQitHT1h1VURjeUgyL0RpR2hE?=
 =?utf-8?B?WVdPWGVMR3d4KzY4Y0dFNjVOeDJOeTlIVjRhWkhKWW92ZmhzWXoySUg4a1dL?=
 =?utf-8?B?cW1SM2t5SjVlNklkK25PNjlDZ1ZoTEdtZjZmQXR2MmVuUmxQOWh1b0VFOVMw?=
 =?utf-8?B?STQvSDFVaVVnWFZWdklGRlNIK0RCOFNHNTA1MWpCRDZLcVdqU0VmTGlvRlNs?=
 =?utf-8?B?dEtkQVFJdkEzdWJZd0dzdGFNOUxwN1hQS2JtN0FXUVJsd2xnNTY2TUtpL3U0?=
 =?utf-8?B?TmZYNVZ5SVJPdWc1VlFOUC9PZjJoYkh0SktjbTg5S2FRZTN6TFVuQVNtZkhP?=
 =?utf-8?B?b2l3cDl2VjNwQkY5Z3hFOTJmL3haYmJpdzU4UHZVMzRqamZiWlFRdjR1VHMx?=
 =?utf-8?B?YjF5Q1g2WjdvMVB5ODNldmFycDFGNzg0ajRIQlhhWVZWdGszbSszY2NYUlBP?=
 =?utf-8?B?YUt6SjlCcjRaUXY1dHFhLzYvc3FKS05VaTdFbWR2Qnc5TUkzZEF1eUtlYmxB?=
 =?utf-8?B?UjZZcU9ZakdzWDFTMHZvVWsvSkY3enp6MEZNYjNkeTFjNDM2cTRFYWFCaFZm?=
 =?utf-8?B?anFWTW9USDYxYmZrVmlTUU9LMHZrT3BsVWtDazhaUVBpbmJGbzZZWWpsVS80?=
 =?utf-8?B?VlF6NmlicGJHcE9XVVpDQUdWZm5tNnQzWlkreVh5dkV1VmdQbmNoR1B3ZVlq?=
 =?utf-8?B?ZVQvazhCSjB1TEd0NDRhTWsvVGlFbWtuY25YclQrTmJGVTB6K1lhWnFyTmtw?=
 =?utf-8?B?Qk5UM3dzSnBPY09XZXlNZlhBSEVjN29TZDVKSHhIMFJXYnF0Wko5VWluMXVz?=
 =?utf-8?B?ckVNbXBETjZ4RzZweXpDelVQQ252RFkwR1E1Tmp1Z1F0dGFhSGpXVEJnQW0w?=
 =?utf-8?B?MVRHWHRjM2g5TzJPUDAwQkRtNXoyd0I4c2ZmSVlicnZQekdmZDl1YUs2WG9V?=
 =?utf-8?B?YlJVb3dCTWhjVTJ2elk2bGc4Ym45Qkt3d1NnOHExWUxpb0hMRG9oR2c4OTk0?=
 =?utf-8?B?dS9FV3k5anhVbTFtQUljeWkxcGhMY0tlS1JTREhCQWU1RjdtNkQ5KzNhYnZS?=
 =?utf-8?B?d3dicSsyZlBteEZBcnBEMVNJVzRrdldsWXZlMm9qUVNEWFd3S2dhMWJuOEo3?=
 =?utf-8?B?S0RLVVpzMFgrK2xYVWRpcmlyVTlEczZIclRHb1dxTWlLY2lZN2FrSmFNL29x?=
 =?utf-8?B?RDFEeXFRdGorRmdiWEV3M3c2aTFDaGRobUJJZ3pRSUdrLzFXcWFCVGlWU01o?=
 =?utf-8?B?SzRnTmI5NjdQM3NUak9tR0tjQkRhTlBUR0MyV0xVUndVaHJkTGhybHBXVm9T?=
 =?utf-8?B?bmliSHpyZXVZTzgzOHR5Y0tuOXFDdmk1cVloUHVRTUx2blh1UkVMT3h6MUxu?=
 =?utf-8?B?bkJqcGM4ODFxQTlzM2dMOXRRaXgxUW9RbXVvdHVodU82ZGk4MGJ2aXBib0Ro?=
 =?utf-8?B?QzVZWjlKMDVTa3dVbnhSakR4eU5pd1pSS2dTenp3ZFRXQ3JQVXh5aWZmOGt2?=
 =?utf-8?B?bU1OakMxUmlyQ3o5ZGpSc1ZVQlhIZHgvOXBxSkxRdDRYTHJWUStUWFFXZWly?=
 =?utf-8?B?WkFtOU1IajFHQmVmSnFXRzBhL09UVzVhM1NkTDA0MVBvT3hadk9KS1piWngz?=
 =?utf-8?B?UnZMV2hjdDc5QUZsYTZOZ3E3MFpKNlhZSENTbktoNU9VbzZSSVp4b1BxeE53?=
 =?utf-8?B?eXRlUWt4c0dseFhKcGI2Q05aN0F1T21VcjZ5bmV2MGp2Q2R4eDFsTC9ZeFVI?=
 =?utf-8?B?SGlZelVoOUV4TzdiRVhIZkJuZ2k5TmRQK01HT21IQXVQcGpzd1ViWmpzSm5u?=
 =?utf-8?B?b1VUcWdTTkpXQnUrclhqcFk4ZjBySitySC9tRWtNNjU2Ukk5KzJndUFKV05v?=
 =?utf-8?B?a1hyNnNtbGhXZnJpSXR1eUQxR21zNWdHQzhtckk1akpXMzlxRVFiNWZBSVM3?=
 =?utf-8?B?SC9QNTE5TlVhalBHa3Z6Nk1pcUg0OWxkOExUVE1xSEVVUTkxTm9sYktPK0Iv?=
 =?utf-8?B?TFFMZ0xzdjV2a3kvWWhjMGM3RjFzV29lc0xaM3I1QmMwQWhJTGF6dk5jaWtP?=
 =?utf-8?B?SHJSTEI3UVdySkp4TngwSTRxaTJTTzVlNDRYMHZ4Y3M3MDlvdkNRODk4NW9Z?=
 =?utf-8?B?aHRKcGVXVjB0eVdNbit2TXRIcHdrMm5mOTJZMGJkYzZBa1c1eCtGUVMwYWU1?=
 =?utf-8?B?WXc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 3bf92856-5b6f-444d-f1d6-08db21a20add
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 10 Mar 2023 20:00:05.6449
 (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: 9n2Tc7h2P+XIJ3bamZA9a1uiebGSo3Grl3N8x61HbYJJfiaNBovSDiDoNa05/TyK4pMPpepJ8bYABDXv2uO8irdsJ8od4UhuWYecbBVOwhA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR11MB6320
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi James,

On 3/6/2023 3:32 AM, James Morse wrote:
> On 02/02/2023 23:47, Reinette Chatre wrote:
>> On 1/13/2023 9:54 AM, James Morse wrote:
>>
>> ...
>>
>>> @@ -567,19 +579,14 @@ static int __rdtgroup_move_task(struct task_struct *tsk,
>>>  	 * For monitor groups, can move the tasks only from
>>>  	 * their parent CTRL group.
>>>  	 */
>>> -
>>> -	if (rdtgrp->type == RDTCTRL_GROUP) {
>>> -		WRITE_ONCE(tsk->closid, rdtgrp->closid);
>>> -		WRITE_ONCE(tsk->rmid, rdtgrp->mon.rmid);
>>> -	} else if (rdtgrp->type == RDTMON_GROUP) {
>>> -		if (rdtgrp->mon.parent->closid == tsk->closid) {
>>> -			WRITE_ONCE(tsk->rmid, rdtgrp->mon.rmid);
>>> -		} else {
>>> -			rdt_last_cmd_puts("Can't move task to different control group\n");
>>> -			return -EINVAL;
>>> -		}
>>> +	if (rdtgrp->type == RDTMON_GROUP &&
>>> +	    !resctrl_arch_match_closid(tsk, rdtgrp->mon.parent->closid)) {
>>> +		rdt_last_cmd_puts("Can't move task to different control group\n");
>>> +		return -EINVAL;
>>>  	}
>>>  
>>> +	resctrl_arch_set_closid_rmid(tsk, rdtgrp->closid, rdtgrp->mon.rmid);
>>
>> This does not use the intended closid when rdtgrp->type == RDTMON_GROUP.
> 
> Yes, it should be rdtgrp->mon.parent->closid.
> 
> rdtgroup_mkdir_mon() initialises them to be the same

Even so, I do find the code to be confusing when it jumps from one to
the other within the same function.

>, I guess its Peter's monitor-group
> rename that means this could get the wrong value?
> 
> I've fixed it as:
> ---------%<---------
> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index c7392d10dc5b..30d8961b833c 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -585,7 +585,12 @@ static int __rdtgroup_move_task(struct task_struct *tsk,
>                 return -EINVAL;
>         }
> 
> -       resctrl_arch_set_closid_rmid(tsk, rdtgrp->closid, rdtgrp->mon.rmid);
> +       if (rdtgrp->type == RDTMON_GROUP)
> +               resctrl_arch_set_closid_rmid(tsk, rdtgrp->mon.parent->closid,
> +                                            rdtgrp->mon.rmid);
> +       else
> +               resctrl_arch_set_closid_rmid(tsk, rdtgrp->closid,
> +                                            rdtgrp->mon.rmid);
> 
>         /*
>          * Ensure the task's closid and rmid are written before determining if
> ---------%<---------
> 
> 

ok, thank you.

Reinette