Hi Reinette,
On 5/10/2024 1:34 PM, Reinette Chatre wrote:
Hi Babu,
On 5/10/2024 10:01 AM, Moger, Babu wrote:
On 5/9/2024 10:18 PM, Reinette Chatre wrote:
On 5/9/2024 3:34 PM, Moger, Babu wrote:
On 5/7/24 15:27, Reinette Chatre wrote:
On 5/6/2024 12:09 PM, Moger, Babu wrote:
On 5/3/24 18:26, Reinette Chatre wrote:
On 3/28/2024 6:06 PM, Babu Moger wrote:
...
+ * @mbm_assign_cntrs: Maximum number of assignable counters
*/
struct rdt_resource {
int rid;
@@ -188,6 +198,8 @@ struct rdt_resource {
struct list_head evt_list;
unsigned long fflags;
bool cdp_capable;
+ bool mbm_assign_capable;
+ u32 mbm_assign_cntrs;
};
Please check tabs vs spaces (in this whole series please).
Sure. Will do.
I'm thinking that a new "MBM specific" struct within
struct rdt_resource will be helpful to clearly separate the MBM related
data. This will be similar to struct resctrl_cache for
cache allocation and struct resctrl_membw for memory bandwidth
allocation.
Did you mean to move all the fields for monitoring to move to new struct?
Struct resctrl_mbm {
int num_rmid;
bool mbm_assign_capable;
u32 mbm_assign_cntrs;
}:
Indeed, so not actually MBM specific but monitoring specific as you state (with
appropriate naming?). This is purely to help organize data within struct rdt_resource
and (similar to struct resctrl_cache and struct resctrl_membw) not a new
structure expected to be passed around. I think evt_list can also be a member.
How about this?
Lets keep assign_capable in main structure(like we have mon_capable) and
move rest of them into new structure.
Struct resctrl_mon {
int num_rmid;
struct list_head evt_list;
u32 num_assign_cntrs;
}:
This looks like a good start. It certainly supports ABMC. I do not yet
have a clear understanding about how this will support MPAM and soft-RMID/ABMC
since the current implementation has an implicit scope of one counter per event per
monitor group. It thus seem reasonable to have a new generic name of "cntrs".
How about renaming it to "assignable_counters"?
As I mentioned in [1] the "assign" concept is not clear (just to me perhaps?). It
really seems to be the marketing name percolating into the implementation. Why is
"assignable" needed to be in a "counter" variable name? Is a variable not by definition
"assignable"? Why not just, for example, "num_cntrs"? I believe that things to be
as simple and obvious as possible ... this just helps to reduce confusion.
Ok. That sounds fine to me.
My previous comment regarding counter scope is not addressed by a rename though and
I do not expect you to have the answer but I would like us to keep in mind how these
"counters" could end up getting used. We may just later, when soft-RMID/ABMC and MPAM
is understood, need to add a "counter scope" as well.
Sure.
Reinette
[1] https://lore.kernel.org/lkml/0d94849c-828c-4f10-a6f8-e26cc4554909@xxxxxxxxx/
Thanks
- Babu Moger
