Re: [PATCH v17 25/33] fs/resctrl: Provide interface to update the event configurations

[Reinette Chatre <reinette.chatre@intel.com>]

Hi Babu,

On 9/3/25 11:32 AM, Moger, Babu wrote:
> Hi Reinette,
> 
> On 9/3/25 12:55, Reinette Chatre wrote:
>> Hi Babu,
>>
>> On 9/3/25 10:38 AM, Moger, Babu wrote:
>>> On 9/2/25 21:41, Reinette Chatre wrote:
>>>> On 8/14/25 7:25 PM, Babu Moger wrote:
>>
>>>>> +
>>>>> +ssize_t event_filter_write(struct kernfs_open_file *of, char *buf, size_t nbytes,
>>>>> +			   loff_t off)
>>>>> +{
>>>>> +	struct mon_evt *mevt = rdt_kn_parent_priv(of->kn);
>>>>> +	struct rdt_resource *r;
>>>>> +	u32 evt_cfg = 0;
>>>>> +	int ret = 0;
>>>>> +
>>>>> +	/* Valid input requires a trailing newline */
>>>>> +	if (nbytes == 0 || buf[nbytes - 1] != '\n')
>>>>> +		return -EINVAL;
>>>>> +
>>>>> +	buf[nbytes - 1] = '\0';
>>>>> +
>>>>> +	cpus_read_lock();
>>>>> +	mutex_lock(&rdtgroup_mutex);
>>>>> +
>>>>> +	rdt_last_cmd_clear();
>>>>> +
>>>>> +	r = resctrl_arch_get_resource(mevt->rid);
>>>>> +	if (!resctrl_arch_mbm_cntr_assign_enabled(r)) {
>>>>> +		rdt_last_cmd_puts("mbm_event counter assignment mode is not enabled\n");
>>>>> +		ret = -EINVAL;
>>>>> +		goto out_unlock;
>>>>> +	}
>>>>> +
>>>>> +	ret = resctrl_parse_mem_transactions(buf, &evt_cfg);
>>>>> +	if (!ret && mevt->evt_cfg != evt_cfg) {
>>>>
>>>> ... is evt_cfg of 0 (a) a valid value (that will not cause hardware to fault) and
>>>> (b) a reasonable value to allow? 
>>>>
>>>
>>> The value evt_cfg = 0 is valid and permitted for both ABMC and BMEC. I
>>> have confirmed here through verification and testing. In that case, the
>>> event counter will not be monitoring anything.
>>
>> Thank you for checking.
>>
>> I do not know what a use case for this would be but I also do not see why kernel
>> should prevent user space from doing this. Looks like event_filter_show() will then
>> just print a '\n' that is expected. Are counters expected to always return 0 in this
>> case or will they return an error? I am not clear on what qualifies as "invalid counter
>> configuration" for ABMC that results in RMID_VAL_ERROR.
> 
> The event counters return 0 in this case. I would not think this as an
> invalid event configuration.
> 
> One option is to add a text in last_cmd_status from event_filter_show().
> 
> rdt_last_cmd_printf("Event %s is not configured to monitor any memory
> transactions\n", mevt->name);


event_filter_show() does not return an error in this case so a user may not
check last_cmd_status since the doc specifies that "If the command was successful,
it will read as "ok"." Since the write() succeeded we should not fail the associated
show() to prompt the user to check last_cmd_status though.

Even so, an additional message does not seem necessary to me because as I understand
event_filter_show() returns an empty (just a '\n') file that I think
already reflects that no memory transactions are configured. It remains an awkward
use case to me but from what you stated this is valid for the hardware, it is what the
user requested, and the user can use interface to confirm the state. It does not seem
like changes are needed for this scenario.

Reinette