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

[Zeng Heng <zengheng4@huawei.com>]

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