fs/resctrl: Restore the missing rdt_last_cmd_clear()

[PATCH] fs/resctrl: Restore the missing rdt_last_cmd_clear()

Posted by Zeng Heng 6 months, 2 weeks ago

The fixes tag patch resolves the lockdep warning. However, directly
removing rdt_last_cmd_clear() would leave the last_cmd_status interface
with stale logs, which does not conform to the functional definition before
the fix. Therefore, the rdt_last_cmd_clear() operation is performed after
successfully acquiring the rdtgroup_mutex.

Fixes: c8eafe149530 ("x86/resctrl: Fix potential lockdep warning")
Signed-off-by: Zeng Heng <zengheng4@huawei.com>
---
 fs/resctrl/rdtgroup.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
index cc37f58b47dd..4aae9eb74215 100644
--- a/fs/resctrl/rdtgroup.c
+++ b/fs/resctrl/rdtgroup.c
@@ -536,6 +536,8 @@ static ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of,
 		goto unlock;
 	}
 
+	rdt_last_cmd_clear();
+
 	if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED ||
 	    rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
 		ret = -EINVAL;
@@ -3481,6 +3483,8 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
 		goto out_unlock;
 	}
 
+	rdt_last_cmd_clear();
+
 	if (rtype == RDTMON_GROUP &&
 	    (prdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP ||
 	     prdtgrp->mode == RDT_MODE_PSEUDO_LOCKED)) {
-- 
2.25.1

Re: [PATCH] fs/resctrl: Restore the missing rdt_last_cmd_clear()

Posted by Reinette Chatre 6 months, 2 weeks ago

Hi Zeng Heng,

Thank you very much for catching this and providing a fix.

On 5/29/25 4:33 AM, Zeng Heng wrote:
> The fixes tag patch resolves the lockdep warning. However, directly
> removing rdt_last_cmd_clear() would leave the last_cmd_status interface
> with stale logs, which does not conform to the functional definition before
> the fix. Therefore, the rdt_last_cmd_clear() operation is performed after
> successfully acquiring the rdtgroup_mutex.

I would like to suggest some rework to changelog to meet requirements from
Documentation/process/maintainer-tip.rst. Specifically the rules about
imperative tone and structure of the changelog. Below attempts to address
those requirements but please feel free to rework after you considered the
rules yourself:

	A lockdep fix removed two rdt_last_cmd_clear() calls that were used
	to clear the last_cmd_status buffer but called without holding the 
	required rdtgroup_mutex. The impacted resctrl commands are:
	writing to the cpus or cpus_list files and creating a new monitor
	or control group. With stale data in the last_cmd_status buffer the
	impacted resctrl commands report the stale error on success, or append
	its own failure message to the stale error on failure.

	Restore the rdt_last_cmd_clear() calls after acquiring rdtgroup_mutex.

> 
> Fixes: c8eafe149530 ("x86/resctrl: Fix potential lockdep warning")
> Signed-off-by: Zeng Heng <zengheng4@huawei.com>
> ---
>  fs/resctrl/rdtgroup.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
> index cc37f58b47dd..4aae9eb74215 100644
> --- a/fs/resctrl/rdtgroup.c
> +++ b/fs/resctrl/rdtgroup.c
> @@ -536,6 +536,8 @@ static ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of,
>  		goto unlock;
>  	}
>  
> +	rdt_last_cmd_clear();
> +
>  	if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED ||
>  	    rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
>  		ret = -EINVAL;
> @@ -3481,6 +3483,8 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
>  		goto out_unlock;
>  	}
>  
> +	rdt_last_cmd_clear();
> +

Could you please move this to be right after acquiring the mutex? I think clearing
last_cmd_status at beginning of a resctrl command's work is a good pattern to follow.
Thus a change like:

---8<---
diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
index 7410321d01ff..77d08229d855 100644
--- a/fs/resctrl/rdtgroup.c
+++ b/fs/resctrl/rdtgroup.c
@@ -3474,6 +3474,8 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
 		goto out_unlock;
 	}
 
+	rdt_last_cmd_clear();
+
 	/*
 	 * Check that the parent directory for a monitor group is a "mon_groups"
 	 * directory.
@@ -3483,8 +3485,6 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
 		goto out_unlock;
 	}
 
-	rdt_last_cmd_clear();
-
 	if (rtype == RDTMON_GROUP &&
 	    (prdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP ||
 	     prdtgrp->mode == RDT_MODE_PSEUDO_LOCKED)) {
---8<---

>  	if (rtype == RDTMON_GROUP &&
>  	    (prdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP ||
>  	     prdtgrp->mode == RDT_MODE_PSEUDO_LOCKED)) {

Thank you very much.

Reinette

Re: [PATCH] fs/resctrl: Restore the missing rdt_last_cmd_clear()

Posted by Zeng Heng 6 months, 2 weeks ago


On 2025/5/30 6:01, Reinette Chatre wrote:
> Hi Zeng Heng,
> 
> Thank you very much for catching this and providing a fix.
> 
> On 5/29/25 4:33 AM, Zeng Heng wrote:
>> The fixes tag patch resolves the lockdep warning. However, directly
>> removing rdt_last_cmd_clear() would leave the last_cmd_status interface
>> with stale logs, which does not conform to the functional definition before
>> the fix. Therefore, the rdt_last_cmd_clear() operation is performed after
>> successfully acquiring the rdtgroup_mutex.
> 
> I would like to suggest some rework to changelog to meet requirements from
> Documentation/process/maintainer-tip.rst. Specifically the rules about
> imperative tone and structure of the changelog. Below attempts to address
> those requirements but please feel free to rework after you considered the
> rules yourself:
> 
> 	A lockdep fix removed two rdt_last_cmd_clear() calls that were used
> 	to clear the last_cmd_status buffer but called without holding the
> 	required rdtgroup_mutex. The impacted resctrl commands are:
> 	writing to the cpus or cpus_list files and creating a new monitor
> 	or control group. With stale data in the last_cmd_status buffer the
> 	impacted resctrl commands report the stale error on success, or append
> 	its own failure message to the stale error on failure.
> 
> 	Restore the rdt_last_cmd_clear() calls after acquiring rdtgroup_mutex.

Thank you for the correction, I will review the requirements mentioned
in the documents above.

> 
>>
>> Fixes: c8eafe149530 ("x86/resctrl: Fix potential lockdep warning")
>> Signed-off-by: Zeng Heng <zengheng4@huawei.com>
>> ---
>>   fs/resctrl/rdtgroup.c | 4 ++++
>>   1 file changed, 4 insertions(+)
>>
>> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
>> index cc37f58b47dd..4aae9eb74215 100644
>> --- a/fs/resctrl/rdtgroup.c
>> +++ b/fs/resctrl/rdtgroup.c
>> @@ -536,6 +536,8 @@ static ssize_t rdtgroup_cpus_write(struct kernfs_open_file *of,
>>   		goto unlock;
>>   	}
>>   
>> +	rdt_last_cmd_clear();
>> +
>>   	if (rdtgrp->mode == RDT_MODE_PSEUDO_LOCKED ||
>>   	    rdtgrp->mode == RDT_MODE_PSEUDO_LOCKSETUP) {
>>   		ret = -EINVAL;
>> @@ -3481,6 +3483,8 @@ static int mkdir_rdt_prepare(struct kernfs_node *parent_kn,
>>   		goto out_unlock;
>>   	}
>>   
>> +	rdt_last_cmd_clear();
>> +
> 
> Could you please move this to be right after acquiring the mutex? I think clearing
> last_cmd_status at beginning of a resctrl command's work is a good pattern to follow.
> Thus a change like:

The patch will be corrected in version v2. Thank you again.

Best regards，
Zeng Heng

Re: [PATCH] fs/resctrl: Restore the missing rdt_last_cmd_clear()

Posted by Reinette Chatre 6 months, 2 weeks ago

Hi Zeng Heng,

On 5/30/25 2:34 AM, Zeng Heng wrote:
> 
> The patch will be corrected in version v2. Thank you again.
> 

I did not notice the outdated email addresses it until I received 
the mail bounces. Here are some updated email addresses for the
next version:
fenghua.yu@intel.com	-> fenghuay@nvidia.com
bp@suse.de		-> bp@alien8.de

Please drop Xiaochen Shen from next submission.
Please include x86 maintainers in next submission: x86@kernel.org

Thank you

Reinette