Re: [PATCH 00/11] x86,fs/resctrl: Improve resctrl quality and consistency

[Reinette Chatre]

Hi Ben,

On 3/19/26 2:53 AM, Ben Horgan wrote:
> Hi Reinette,
> 
> On 3/18/26 20:12, Reinette Chatre wrote:
>> Hi Ben,
>>
>> On 3/18/26 10:10 AM, Ben Horgan wrote:
>>> On 3/18/26 16:35, Reinette Chatre wrote:
>>
>>
>>>> What resctrl could do in such scenario is to at least convey that some messages were
>>>> dropped. Consider, for example:
>>>>
>>>> diff --git a/fs/resctrl/rdtgroup.c b/fs/resctrl/rdtgroup.c
>>>> index 5da305bd36c9..ea77fa6a38f7 100644
>>>> --- a/fs/resctrl/rdtgroup.c
>>>> +++ b/fs/resctrl/rdtgroup.c
>>>> @@ -973,10 +973,13 @@ static int rdt_last_cmd_status_show(struct kernfs_open_file *of,
>>>>  
>>>>  	mutex_lock(&rdtgroup_mutex);
>>>>  	len = seq_buf_used(&last_cmd_status);
>>>> -	if (len)
>>>> +	if (len) {
>>>>  		seq_printf(seq, "%.*s", len, last_cmd_status_buf);
>>>> -	else
>>>> +		if (seq_buf_has_overflowed(&last_cmd_status))
>>>> +			seq_puts(seq, "[truncated]\n");
>>>> +	} else {
>>>>  		seq_puts(seq, "ok\n");
>>>> +	}
>>>>  	mutex_unlock(&rdtgroup_mutex);
>>>>  	return 0;
>>>>  }
>>>
>>> Adding a truncation indication makes sense to me. Would it be good to reserve space in the
>>> last_cmd_status_buf[] to ensure this can always be displayed? It looks like space could be
>>> made by interacting with seq->size directly but I'm not sure if there is a cleaner way
>>> to do it.
>>>
>>
>> Please note the distinction between the struct seq_file instance pointed to by seq and the 
>> struct seq_buf instance last_cmd_status. The last_cmd_status seq_buf instance is backed by
>> last_cmd_status_buf of 512 bytes which is printed to the seq_file instance seq that is
>> backed by another buffer that starts out with size PAGE_SIZE. So, it looks to me
>> like printing last_cmd_status_buf to the seq seq_file instance followed by "[truncated]\n"
>> should fit by default? This should keep working even if last_cmd_status_buf size is
> 
> Thanks for the explanation. I had missed that distinction. I've just given your code a go with
> some hacked in rdt_last_cmd_puts() calls and it behaves as you say.

Thank you very much for trying it out.

> 
> We've discussed two changes, one is adding a truncation message to last_cmd_status and the other
> is carrying on after failure when allocating counters. Are you going to take these from here or would
> you like patches from me?

Adding the truncation message to last_cmd_status seems to complement the other last_cmd_status
improvements in this series. I'd be happy to create the patch and include it as part of this
series.

I would like to confirm behavior when encountering error when allocating counters though:
My previous assessment of rdtgroup_assign_cntr_event() callers was incomplete since it is also
called with NULL domain when the user uses the "*=<assignment state>" syntax to mbm_L3_assignments.
This is a different scenario from directory creation since it passes the error back to
user space. While here it may also be ok to carry on after a counter could not be allocated in
one domain I do think that user space should still learn via error return that not all allocations
succeeded.

Even so, to answer your original question, please create the patches that change the allocation
behavior. This seems to complement the work you are currently doing and would be easy for you to test
(I do not have access to assignable mode hardware).

Thank you.

Reinette