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: <ef509f34-954d-61c0-f730-2cbc93eae4b8@amd.com>
Date:   Mon, 7 Aug 2023 11:19:20 -0500
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.13.0
Reply-To: babu.moger@amd.com
Subject: Re: [PATCH v6 5/8] x86/resctrl: Unwind the errors inside
 rdt_enable_ctx
Content-Language: en-US
To:     Reinette Chatre <reinette.chatre@intel.com>, corbet@lwn.net,
        tglx@linutronix.de, mingo@redhat.com, bp@alien8.de
Cc:     fenghua.yu@intel.com, dave.hansen@linux.intel.com, x86@kernel.org,
        hpa@zytor.com, paulmck@kernel.org, akpm@linux-foundation.org,
        quic_neeraju@quicinc.com, rdunlap@infradead.org,
        damien.lemoal@opensource.wdc.com, songmuchun@bytedance.com,
        peterz@infradead.org, jpoimboe@kernel.org, pbonzini@redhat.com,
        chang.seok.bae@intel.com, pawan.kumar.gupta@linux.intel.com,
        jmattson@google.com, daniel.sneddon@linux.intel.com,
        sandipan.das@amd.com, tony.luck@intel.com, james.morse@arm.com,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org,
        bagasdotme@gmail.com, eranian@google.com,
        christophe.leroy@csgroup.eu, jarkko@kernel.org,
        adrian.hunter@intel.com, quic_jiles@quicinc.com,
        peternewman@google.com
References: <168980872063.1619861.420806535295905172.stgit@bmoger-ubuntu>
 <168980892326.1619861.2405779251348138586.stgit@bmoger-ubuntu>
 <9fd70ef3-ca90-65e3-4746-7d574bdd159b@intel.com>
From:   "Moger, Babu" <babu.moger@amd.com>
In-Reply-To: <9fd70ef3-ca90-65e3-4746-7d574bdd159b@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?anhUM0ZoaWJ4dit4ZlNTMGp2QVpVUUZ1amRvUGgxYzU5cktJbGc3OFVib21C?=
 =?utf-8?B?K2V3L0tJWE45ZkxTQXZsaWV0VGJrNVFNTmlSZTg0SGJickxVbzlxSGZKTStz?=
 =?utf-8?B?RmNZL0xEQWdPY1dKZnBpZExCRjZJd0l0N2MycFk3Z0UwWlRJVFdFNlFLNHdK?=
 =?utf-8?B?MENuZWJyOG5PZEdNMEdhNk0zZ0FaYUZyakgweGZvVTlENUhpZk50WHMwRU1C?=
 =?utf-8?B?ZkdFNE1Pc3FaVW92a1U1VXZoc2Y2KzFiWFpXek1EM0ZTYXVObnRhb0dJZHB3?=
 =?utf-8?B?TWg0TUZPZGk5NDk1Nmk1UlVMWHpnVG0zWTVRaDR3T3U2QkJiM1BTLzR0MFcz?=
 =?utf-8?B?WVFXdG1CaGVrZVVWd2lNY2ZQcmlOS3ZtQzBnWGNINkVWcTQwVzFnODF6UnpQ?=
 =?utf-8?B?NkpreWNzTGhCbUF5QjZqT3JNZDk4Ly96eW9WKzFMMXZkY0FKcDVoU2JmMm9E?=
 =?utf-8?B?d0F3RlNib2N5Z3k4NndDbnJMSDhSSkFuM044UVMweEErdHNRTlBTNmRiUDRV?=
 =?utf-8?B?aTN3SWFWUlNkK1BjaUV3cUx4ODg1SnhoWjJVN3JsRGFzY3F5OUtnM1dhY251?=
 =?utf-8?B?Rk0wTEVWc2EwZ2lqODRrMVdVbUw4bFIyUWk2SWR0YzU0SVFweEdNeDVsc3p3?=
 =?utf-8?B?VDNNbmZQT1QxajlDeWFCbDhCSHVTVUswRVRGRUlkQXRYUXVnMnNLSEJxajN2?=
 =?utf-8?B?K0k3djFwZ1FwQ010STdydnN5Zk51cTd1OVNNcW5kWVI4bHBKOVN4WWJTSFhM?=
 =?utf-8?B?UFRjKzA1Q0xaZkdPMzdLTDRHL1F1R0YzdlJWbWdIM2g3cTMvSjVPL0I2Q2Fa?=
 =?utf-8?B?OCtUTEVxRSs2ZFZjWTVqWHJWR1JMdzRzb3FWOHptOGdQbjhrekhWT2xtVzRq?=
 =?utf-8?B?YllvRzBZK1VsOU9rM3ljbFdwTTJ2NEtZQTBvS0V4OW9SM3UxZUtCSnNHeDFJ?=
 =?utf-8?B?b0Fab3Q2NkpNejBVdHY0QU9DZU50RWNud3BIMWZBWEc1MitDZXpNN1puWGxa?=
 =?utf-8?B?SXVmdW00Mzc5TlBSUTk0Nk1HOXd6M1MyRUxtN3NDSm9xMnVLZnlnTFpkWHVa?=
 =?utf-8?B?RmZpZzlzRHRBVHhuZnlRd1UvV3F1TGVzUXNYRXpmaGVFUmFlU3drRXVUVFBL?=
 =?utf-8?B?Y1hLR24yYjdtN0RuUUxUWmhyVU9oekJ0MnJRWmM4TzVadWhHMklaeHd0SVo4?=
 =?utf-8?B?TXRZU2JyQnhzbkNaQUYrbGFtKy80b1JMQkRHUWt1WnpDZURMRkM3T3cydGZw?=
 =?utf-8?B?OGRPZDhNc01JNFlkZTU0SzQ4SEFHMU04TWErbDBYNVRLYzNCbUtRQi9SdG5X?=
 =?utf-8?B?Nk42d29IbDNWMnYxOTlKY2Jlc1hBRWhQK2JtWHJiemtiOVd1SGlmdytUdndZ?=
 =?utf-8?B?a2Y5aTBoR3o4UVIvVGwzMnB4WmNFVGF4ZlBZclJXdnZoYlIrZ3A1VWpoK1pB?=
 =?utf-8?B?a2xyNS9XVGxueEliSGhHVjZVamluclVWMmZ5Ty80MzFLN0g0R3VZRW5HMm1i?=
 =?utf-8?B?V1JVOG8xOXdDZDdpQXlMZTI0RDZFdy9RUzBHaDgwdlFQRWtwNk5UT2ZMWHhR?=
 =?utf-8?B?WHBObGdFL3paTDlaYTBGL1BiRWZMdnZUakQ3OGZyQzVVTmxkYkhvWUZ3NER2?=
 =?utf-8?B?bnRnbVpSSC8zcGtsYUsyUUNGd2ZrZE52NEJZazlNMkxuck9lZ2V5NVpKRzNM?=
 =?utf-8?B?Z1JqUEtaSVB5eWhUYVh3cVlmbjNOMlNpSGpwWlJBeklmKzZ4V1QzNlJnSzRu?=
 =?utf-8?B?eEV2ak9LNS9Qdi9qZzZGamxiZk9ueTQxVWszS1VlWTZPZDB5QWZPZ0pDbk5k?=
 =?utf-8?B?SktmK0J5bEJ6SUdFNHd6c0dDNGdUQkJjdExESWFYN002WERPbGI5T1M4SG5R?=
 =?utf-8?B?NDZCRmsrUUpaeHFUT1huSVFhUGhiUVRqckpLZS9vTzJMM3UxMzFDL3dYaHNI?=
 =?utf-8?B?d0MxL1E0cmt6OGVTWjFnNytSQm5GREk1cE1CZ1R2UHVDa1hhV0NtU0Y1aXZS?=
 =?utf-8?B?M1JpWlF5aXZLU2s2SnFabzdEZFR5QnN0a28xaDhjdUoyWDVCWklpVFBmUG1q?=
 =?utf-8?B?cXhwWWFDVEJCcnhMME5Jdm4zelpXNmkxV2VwSFN3SG1WZzE5MFEyRFB4WXd0?=
 =?utf-8?Q?E4Xk=3D?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 54417a61-0455-4154-7527-08db97620ede
X-MS-Exchange-CrossTenant-AuthSource: MW3PR12MB4553.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Aug 2023 16:19:21.7370
 (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: YkrTSv+AsZDz92DynA7qRWRn/3IuLDr8w9kMqeSNckCW1bft79dvonRPVCIGqU+E
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR12MB7520
X-Spam-Status: No, score=-2.9 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,
        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 Reinette,

On 8/4/23 15:41, Reinette Chatre wrote:
> Hi Babu,
> 
> On 7/19/2023 4:22 PM, Babu Moger wrote:
>> rdt_enable_ctx() takes care of enabling the features provided during
> 
> "rdt_enable_ctx() takes care of enabling" can just be "rdt_enable_ctx()
> enables"

Sure.

> 
>> resctrl mount. The error unwinding of rdt_enable_ctx is done from the
>> caller rdt_get_tree. This is not ideal and can cause some error unwinding
>> to be omitted.
>>
> 
> Please consistently use () to indicate function names (in
> changelog and subject).

Sure.

> 
> "This is not ideal and can cause some error unwinding to be omitted."
> is a bit vague. How about (in a new paragraph):
> "Additions to rdt_enable_ctx() are required to also modify error paths
> of rdt_enable_ctx() callers to ensure correct unwinding if errors
> are encountered after calling rdt_enable_ctx(). This is error prone."

Sure.

> 
>> Fix this by moving all the error unwinding inside rdt_enable_ctx.
> 
> "Fix" creates expectation for a "fixes" tag which is not needed here. This
> refactors code to simplify future additions.

Sure.
> 
> Even so, I do not think this solution addresses the stated problem (more
> below).
> 
>>
>> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
>> Signed-off-by: Babu Moger <babu.moger@amd.com>
>> ---
>>  arch/x86/kernel/cpu/resctrl/rdtgroup.c |   31 +++++++++++++++++++++++--------
>>  1 file changed, 23 insertions(+), 8 deletions(-)
>>
>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> index 3010e3a1394d..9a7204f71d2d 100644
>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
>> @@ -2381,15 +2381,31 @@ static int rdt_enable_ctx(struct rdt_fs_context *ctx)
>>  {
>>  	int ret = 0;
>>  
>> -	if (ctx->enable_cdpl2)
>> +	if (ctx->enable_cdpl2) {
>>  		ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, true);
>> +		if (ret)
>> +			goto out;
>> +	}
>>  
>> -	if (!ret && ctx->enable_cdpl3)
>> +	if (ctx->enable_cdpl3) {
>>  		ret = resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, true);
>> +		if (ret)
>> +			goto out_cdpl2;
>> +	}
>>  
>> -	if (!ret && ctx->enable_mba_mbps)
>> +	if (ctx->enable_mba_mbps) {
>>  		ret = set_mba_sc(true);
>> +		if (ret)
>> +			goto out_cdpl3;
>> +	}
>>  
>> +	return 0;
>> +
>> +out_cdpl3:
>> +	resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L3, false);
>> +out_cdpl2:
>> +	resctrl_arch_set_cdp_enabled(RDT_RESOURCE_L2, false);
> 
> Be careful here. There is no dependency between L3 and L2 CDP ...
> if L3 CDP was enabled it does not mean that L2 CDP was enabled also.
> Similarly, if the software controller was enabled it does not mean that
> CDP was also enabled.
> Since resctrl_arch_set_cdp_enabled() does much more than just change
> a flag value I think these should first check if it was enabled
> before disabling the feature.

Yes. Agree.
> 
>> +out:
>>  	return ret;
>>  }
>>  
>> @@ -2497,13 +2513,13 @@ static int rdt_get_tree(struct fs_context *fc)
>>  	}
>>  
>>  	ret = rdt_enable_ctx(ctx);
>> -	if (ret < 0)
>> -		goto out_cdp;
>> +	if (ret)
>> +		goto out;
>>  
>>  	ret = schemata_list_create();
>>  	if (ret) {
>>  		schemata_list_destroy();
>> -		goto out_mba;
>> +		goto out_ctx;
>>  	}
>>  
>>  	closid_init();
>> @@ -2562,10 +2578,9 @@ static int rdt_get_tree(struct fs_context *fc)
>>  	kernfs_remove(kn_info);
>>  out_schemata_free:
>>  	schemata_list_destroy();
>> -out_mba:
>> +out_ctx:
>>  	if (ctx->enable_mba_mbps)
>>  		set_mba_sc(false);
>> -out_cdp:
>>  	cdp_disable_all();
>>  out:
>>  	rdt_last_cmd_clear();
>>
> 
> The problem statement in the changelog was that rdt_get_tree() is
> doing error unwinding of rdt_enable_ctx(). Looking at the above it
> seems that the problem remains ... callers of rdt_enable_ctx()
> still need to know all internals of that function to do error
> unwind correctly. Could it perhaps be made simpler with a new
> rdt_disable_ctx() that undoes rdt_enable_ctx()? New additions
> to rdt_enable_ctx() would have more clarity where changes are
> needed and callers only need to call a single rdt_disable_ctx().
> 

Yes. We can do that.
-- 
Thanks
Babu Moger