On 11/13/23 12:52, Tony Luck wrote:
> On Mon, Nov 13, 2023 at 12:08:19PM -0600, Moger, Babu wrote:
>> Hi Tony,
>>
>> Sorry for the late review. The patches look good for the most part. But we
>> can simplify a little more. Please see my comments below.
>>
>>
>> On 11/9/23 17:09, Tony Luck wrote:
>>> The rdt_domain structure is used for both control and monitor features.
>>> It is about to be split into separate structures for these two usages
>>> because the scope for control and monitoring features for a resource
>>> will be different for future resources.
>>>
>>> To allow for common code that scans a list of domains looking for a
>>> specific domain id, move all the common fields ("list", "id", "cpu_mask")
>>> into their own structure within the rdt_domain structure.
>>>
>>> Signed-off-by: Tony Luck <tony.luck@xxxxxxxxx>
>>> ---
>>> include/linux/resctrl.h | 16 ++++--
>>> arch/x86/kernel/cpu/resctrl/core.c | 26 +++++-----
>>> arch/x86/kernel/cpu/resctrl/ctrlmondata.c | 22 ++++-----
>>> arch/x86/kernel/cpu/resctrl/monitor.c | 10 ++--
>>> arch/x86/kernel/cpu/resctrl/pseudo_lock.c | 14 +++---
>>> arch/x86/kernel/cpu/resctrl/rdtgroup.c | 60 +++++++++++------------
>>> 6 files changed, 78 insertions(+), 70 deletions(-)
>>>
>>> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
>>> index 7d4eb7df611d..c4067150a6b7 100644
>>> --- a/include/linux/resctrl.h
>>> +++ b/include/linux/resctrl.h
>>> @@ -53,10 +53,20 @@ struct resctrl_staged_config {
>>> };
>>>
>>> /**
>>> - * struct rdt_domain - group of CPUs sharing a resctrl resource
>>> + * struct rdt_domain_hdr - common header for different domain types
>>> * @list: all instances of this resource
>>> * @id: unique id for this instance
>>> * @cpu_mask: which CPUs share this resource
>>> + */
>>> +struct rdt_domain_hdr {
>>> + struct list_head list;
>>> + int id;
>>> + struct cpumask cpu_mask;
>>> +};
>>
>> I like the idea of separating the domains, one for control and another for
>> monitor. I have some comments on how it can be done to simplify the code.
>> Adding the hdr adds a little complexity to the code.
>>
>> How about converting the current rdt_domain to explicitly to rdt_mon_domain?
>>
>> Something like this.
>>
>> struct rdt_mon_domain {
>> struct list_head list;
>> int id;
>> struct cpumask cpu_mask;
>> unsigned long *rmid_busy_llc;
>> struct mbm_state *mbm_total;
>> struct mbm_state *mbm_local;
>> struct delayed_work mbm_over;
>> struct delayed_work cqm_limbo;
>> int mbm_work_cpu;
>> int cqm_work_cpu;
>> };
>>
>>
>> Then introduce rdt_ctrl_domain to which separates into two doamins.
>>
>> struct rdt_ctrl_domain {
>> struct list_head list;
>> int id;
>> struct cpumask cpu_mask;
>> struct pseudo_lock_region *plr;
>> struct resctrl_staged_config staged_config[CDP_NUM_TYPES];
>> u32 *mbps_val;
>> };
>>
>> I feel this will be easy to understand and makes the code simpler. Changes
>> will be minimal.
>
> Babu,
>
> I went in this direction because of rdt_find_domain() which is used
> to search either of the "ctrl" or "mon" lists. So it needs to be sure
> that the "list" and "id" fields are at identical offsets in both the
> rdt_ctrl_domain and rdt_mon_domain structures. Best way to guarantee[1]
> that is to create the "hdr" entry (which later acquired the cpu_mask
> field as a common element after a comment from Reinette).
>
> One way to avoid this would be to essentially duplicate
> rdt_find_domain() into rdt_find_ctrl_domain() and rdt_find_mon_domain()
It could been solved by passing a flag(0 or mon and 1 for ctrl) as we know
where this is being called.
> ... but I fear the function is just big enough to get complaints
> that the two copies should somehow be merged.
Ok. That is fine. Lets go with current implementation. Thanks for showing
the discussion.
>
> -Tony
>
> [1] In v5 I tried using a comment in each to say they must be the same,
> but Reinette didn't like comments within declarations:
> https://lore.kernel.org/all/5f1256d3-737e-a447-abbe-f541767b2c8f@xxxxxxxxx/
--
Thanks
Babu Moger
