Messages in this thread | | | Date | Fri, 26 Aug 2022 10:34:17 -0700 | Subject | Re: [PATCH v3 08/10] x86/resctrl: Add the sysfs interface to read the event configuration | From | Reinette Chatre <> |
| |
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
| |