Hi Babu,
On 6/30/25 5:43 PM, Moger, Babu wrote:
> On 6/25/2025 6:21 PM, Reinette Chatre wrote:
>> On 6/13/25 2:05 PM, Babu Moger wrote:
...
>>> + * the assignment
>>> + */
>>> + list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) {
>>> + rdtgroup_assign_cntr(r, prgrp, mevt);
>>> +
>>> + list_for_each_entry(crgrp, &prgrp->mon.crdtgrp_list, mon.crdtgrp_list)
>>> + rdtgroup_assign_cntr(r, crgrp, mevt);
>>> + }
>>> +}
>>> +
>>> +static int resctrl_process_configs(char *tok, u32 *val)
>>> +{
>>> + char *evt_str;
>>> + u32 temp_val;
>>> + bool found;
>>> + int i;
>>> +
>>> +next_config:
>>> + if (!tok || tok[0] == '\0')
>>> + return 0;
>>> +
>>> + /* Start processing the strings for each memory transaction type */
>>> + evt_str = strim(strsep(&tok, ","));
>>> + found = false;
>>> + for (i = 0; i < NUM_MBM_EVT_VALUES; i++) {
>>> + if (!strcmp(mbm_config_values[i].name, evt_str)) {
>>> + temp_val = mbm_config_values[i].val;
>>> + found = true;
>>> + break;
>>> + }
>>> + }
>>> +
>>> + if (!found) {
>>> + rdt_last_cmd_printf("Invalid memory transaction type %s\n", evt_str);
>>> + return -EINVAL;
>>> + }
>>> +
>>> + *val |= temp_val;
>>
>> This still returns a partially initialized value on failure. Please only set
>> provided parameter on success.
>
> Yes. Changed it.
>
> if (!tok || tok[0] == '\0') {
> *val = temp_val;
> return 0;
> }
You may just not have included this in your snippet, but please ensure temp_val is always
initialized. Just this snippet on top of original patch risks using uninitialized variable.
>>> +
>>> + goto next_config;
>>> +}
>>> +
>>> +static ssize_t event_filter_write(struct kernfs_open_file *of, char *buf,
>>> + size_t nbytes, loff_t off)
>>> +{
>>> + struct rdt_resource *r = resctrl_arch_get_resource(RDT_RESOURCE_L3);
>>> + struct mon_evt *mevt = rdt_kn_parent_priv(of->kn);
>>
>> With mon_evt::rid available it should not be necessary to hardcode the resource?
>
> changed it
>
> r = resctrl_arch_get_resource(mevt->rid);
>
>> Do any of these new functions need a struct rdt_resource parameter in addition
>> to struct mon_evt?
>
> We need to make a call resctrl_arch_mbm_cntr_assign_enabled(r)) to proceed. So we need struct rdt_resource.
Understood, but since struct rdt_resource can be determined from mon_evt::rid
it is not obvious to me that providing both is always needed by all these functions.
Reinette
