Re: [PATCH v3 08/10] x86/resctrl: Add the sysfs interface to read the event configuration

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

Hi Babu,

On 8/26/2022 9:49 AM, Moger, Babu wrote:
> On 8/24/22 16:16, Reinette Chatre wrote:
>> On 8/22/2022 6:44 AM, Babu Moger wrote:

...

>>> +#define READS_TO_REMOTE_S_MEM		BIT(5)
>>> +
>>> +/* Dirty Victims to All Types of Memory */
>>> +#define  DIRTY_VICTIS_TO_ALL_MEM	BIT(6)
>> Is this intended to be "DIRTY_VICTIMS_TO_ALL_MEM" ?
> Yes. that is what spec says.

You did notice the typo, right?

>>> +	}
>>> +
>>> +	rdmsr(MSR_IA32_EVT_CFG_BASE + msr_index, md->u.mon_config, h);
>>> +}
>>> +
>>> +void mondata_config_read(struct rdt_domain *d, union mon_data_bits *md)
>>> +{
>>> +	smp_call_function_any(&d->cpu_mask, mon_event_config_read, md, 1);
>>> +}
>>> +
>>> +int rdtgroup_mondata_config_show(struct seq_file *m, void *arg)
>>> +{
>>> +	struct kernfs_open_file *of = m->private;
>>> +	struct rdt_hw_resource *hw_res;
>>> +	u32 resid, evtid, domid;
>>> +	struct rdtgroup *rdtgrp;
>>> +	struct rdt_resource *r;
>>> +	union mon_data_bits md;
>>> +	struct rdt_domain *d;
>>> +	int ret = 0;
>>> +
>>> +	rdtgrp = rdtgroup_kn_lock_live(of->kn);
>>> +	if (!rdtgrp) {
>>> +		ret = -ENOENT;
>>> +		goto out;
>>> +	}
>>> +
>>> +	md.priv = of->kn->priv;
>>> +	resid = md.u.rid;
>>> +	domid = md.u.domid;
>>> +	evtid = md.u.evtid;
>>> +
>>> +	hw_res = &rdt_resources_all[resid];
>>> +	r = &hw_res->r_resctrl;
>>> +
>>> +	d = rdt_find_domain(r, domid, NULL);
>>> +	if (IS_ERR_OR_NULL(d)) {
>>> +		ret = -ENOENT;
>>> +		goto out;
>>> +	}
>>> +
>>> +	mondata_config_read(d, &md);
>>> +
>>> +	seq_printf(m, "0x%x\n", md.u.mon_config);
>> Looking at this patch and the next, the sysfs files are introduced to read
>> from and write to the configuration register. From what I can tell the
>> data is never used internally (what did I miss?). Why is the value of the
>> configuration register stored? 
> 
> You didn't miss anything. We don't need to store it.  But we need it as
> part of mon_data_bits structure because, it need to be passed to
> mon_event_config_read and rdtgroup_mondata_config_write.

These functions are introduced here ... so it is only needed because
the demand is created here also. This can be changed, no? 

> 
> In these functions we need evtid and also config value (mon_config).
> 

I see no need to pass evtid so deep - it can be checked right in
rdtgroup_mondata_config_show() and then an appropriate wrapper
can be called to just return the config value. Even if had to also
pass evtid through many layers you could create a temporary structure
to do so and not unnecessarily  increase the size of a long lived
system structure to satisfy this temporary need.

Reinette