Messages in this thread Patch in this message | | | Date | Thu, 25 Aug 2022 15:44:20 -0500 | Subject | Re: [PATCH v3 06/10] x86/resctrl: Introduce mon_configurable to detect Bandwidth Monitoring Event Configuration | From | "Moger, Babu" <> |
| |
On 8/25/2022 10:56 AM, Reinette Chatre wrote: > Hi Babu, > > On 8/25/2022 8:11 AM, Moger, Babu wrote: >> On 8/24/22 16:15, Reinette Chatre wrote: >>> On 8/22/2022 6:43 AM, Babu Moger wrote: >>>> Newer AMD processors support the new feature Bandwidth Monitoring Event >>>> Configuration (BMEC). The events mbm_total_bytes and mbm_local_bytes >>>> are configurable when this feature is present. >>>> >>>> Set mon_configurable if the feature is available. >>>> > ... > >>>> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >>>> index fc5286067201..855483b297a8 100644 >>>> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c >>>> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c >>>> @@ -995,6 +995,16 @@ static int rdt_num_rmids_show(struct kernfs_open_file *of, >>>> return 0; >>>> } >>>> >>>> +static int rdt_mon_configurable_show(struct kernfs_open_file *of, >>>> + struct seq_file *seq, void *v) >>>> +{ >>>> + struct rdt_resource *r = of->kn->parent->priv; >>>> + >>>> + seq_printf(seq, "%d\n", r->mon_configurable); >>> Why is this file needed? It seems that the next patches also introduce >>> files in support of this new feature that will make the actual configuration >>> data accessible - those files are only created if this feature is supported. >>> Would those files not be sufficient for user space to learn about the feature >>> support? >> This is part of /sys/fs/resctrl/info/L3_MON# directory which basically has >> the information about all the monitoring features. As this is one of the >> mon features, I have added it there. Also, this can be used from the >> utility(like pqos or rdtset) to check if the configuring the monitor is >> supported without looking at individual files. It makes things easier. > I understand the motivation. My concern is that this is a resource wide > file that will display a binary value that, if true, currently means two > events are configurable. We need to consider how things can change in the > future. We should consider that this is only the beginning of monitoring > configuration and need this interface to be ready for future changes. For > example, what if all of the monitoring events are configurable? Let's say, > for example, in future AMD hardware the "llc_occupancy" event also becomes > configurable, how should info/L3_MON/configurable be interpreted? On some > machines it would thus mean that mbm_total_bytes and mbm_local_bytes are > configurable and on some machines it would mean that mbm_total_bytes, > mbm_local_bytes, and llc_occupancy are configurable. This does not make > it easy for utilities. > > So, in this series the info directory on a system that supports BMEC > would look like: > > info/L3_MON/mon_features:llc_occupancy > info/L3_MON/mon_features:mbm_total_bytes > info/L3_MON/mon_features:mbm_local_bytes > info/L3_MON/configurable:1 > > Would information like below not be more specific? > info/L3_MON/mon_features:llc_occupancy > info/L3_MON/mon_features:mbm_total_bytes > info/L3_MON/mon_features:mbm_local_bytes > info/L3_MON/mon_features:mbm_total_config > info/L3_MON/mon_features:mbm_local_config
Hi Reinette,
Yes. That is more specific.
So, basically your idea is to print the information from mon_evt structure if mon_configarable is set in the resource structure.
Some thing like ..
diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c index 83c8780726ff..46c6bf3f08e3 100644 --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c @@ -1194,8 +1194,11 @@ static int rdt_mon_features_show(struct kernfs_open_file *of, struct rdt_resource *r = of->kn->parent->priv; struct mon_evt *mevt;
- list_for_each_entry(mevt, &r->evt_list, list) + list_for_each_entry(mevt, &r->evt_list, list) { seq_printf(seq, "%s\n", mevt->name); + if (r->mon_configurable) + seq_printf(seq, "%s\n", mevt->config); + }
return 0; } Is that the idea?
Thanks Babu
| |