Re: [PATCH v11 2/8] x86/resctrl: Prepare to split rdt_domain structure

["Moger, Babu" <babu.moger@amd.com>]

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@intel.com>
>>> ---
>>>  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@intel.com/

-- 
Thanks
Babu Moger