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

[Ben Horgan]

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.

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?

Thanks,

Ben

> increased since seq_read_iter() that calls this show() would just keep increasing the
> buffer backing the seq_file (up to a very large limit of MAX_RW_COUNT which is INT_MAX & PAGE_MASK)
> until it does fit.
> 
> Reinette