Re: [PATCH v20 00/18] Add support for Sub-NUMA cluster (SNC) systems

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

Hi Reinette,

On 6/14/24 18:11, Reinette Chatre wrote:
> Hi Babu,
> 
> On 6/14/24 2:29 PM, Moger, Babu wrote:
>> Hi Reinette,
>>
>> On 6/14/2024 11:46 AM, Reinette Chatre wrote:
>>> Hi Babu,
>>>
>>> On 6/14/24 9:27 AM, Moger, Babu wrote:
>>>> Hi Reinette,
>>>>
>>>> On 6/13/24 15:32, Reinette Chatre wrote:
>>>>> Hi Babu,
>>>>>
>>>>> On 6/13/24 12:17 PM, Moger, Babu wrote:
>>>>>> I may be little bit out of sync here. Also, sorry to come back late
>>>>>> in the
>>>>>> series.
>>>>>>
>>>>>> Looking at the series again, I see this approach adds lots of code.
>>>>>> Look at this structure.
>>>>>>
>>>>>>
>>>>>> @@ -187,10 +196,12 @@ struct rdt_resource {
>>>>>>        bool            alloc_capable;
>>>>>>        bool            mon_capable;
>>>>>>        int            num_rmid;
>>>>>> -    enum resctrl_scope    scope;
>>>>>> +    enum resctrl_scope    ctrl_scope;
>>>>>> +    enum resctrl_scope    mon_scope;
>>>>>>        struct resctrl_cache    cache;
>>>>>>        struct resctrl_membw    membw;
>>>>>> -    struct list_head    domains;
>>>>>> +    struct list_head    ctrl_domains;
>>>>>> +    struct list_head    mon_domains;
>>>>>>        char            *name;
>>>>>>        int            data_width;
>>>>>>        u32            default_ctrl;
>>>>>>
>>>>>> There are two scope fields.
>>>>>> There are two domains fields.
>>>>>>
>>>>>> These are very confusing and very hard to maintain. Also, I am not
>>>>>> sure if
>>>>>> these fields are useful for anything other than SNC feature. This
>>>>>> approach
>>>>>> adds quite a bit of code for no specific advantage.
>>>>>>
>>>>>> Why don't we just split the RDT_RESOURCE_L3 resource
>>>>>> into separate resources, one for control, one for monitoring.
>>>>>> We already have "control" only resources (MBA, SMBA, L2). Lets
>>>>>> create new
>>>>>> "monitor" only resource. I feel it will be much cleaner approach.
>>>>>>
>>>>>> Tony has already tried that approach and showed that it is much
>>>>>> simpler.
>>>>>>
>>>>>> v15-RFC :
>>>>>> https://lore.kernel.org/lkml/20240130222034.37181-1-tony.luck@intel.com/
>>>>>>
>>>>>> What do you think?
>>>>>>
>>>>>
>>>>> Some highlights of my thoughts in response to that series, but the whole
>>>>> thread
>>>>> may be of interest to you:
>>>>> https://lore.kernel.org/lkml/78c88c6d-2e8d-42d1-a6f2-1c5ac38fb258@intel.com/
>>>>> https://lore.kernel.org/lkml/59944211-d34a-4ba3-a1de-095822c0b3f0@intel.com/
>>>>>
>>>>
>>>> Went through the thread, in summary:
>>>>
>>>> The main concerns are related to duplication of code and data structures.
>>>>
>>>> The solutions are
>>>>
>>>> a) Split the domains.
>>>> This is what this series is doing now. This creates members like
>>>> ctrl_scope, mon_scope, ctrl_domains etc.. These fields are added to all
>>>> the resources (MBA, SMBA and L2). Then there is additional domain header.
>>>>
>>>>
>>>> b) Split the resource.
>>>>    Split RDT_RESOURCE_L3 into two, one for "monitor" and one for
>>>> "control".
>>>>    There will be one domain structure for "monitor" and  one for
>>>> "control"
>>>>
>>>> Both these approaches have code and data duplication. So, there is no
>>>> difference that way.
>>>
>>> Could you please elaborate where code and data duplication of (a) is?
>>
>> We have ctrl_scope, mon_scope, ctrl_domains. mon_domains.  Only one
>> resource, RDT_RESOURCE_L3 is going to use these fields. Rest of the
>> resources don't need these fields. But these fields are part of all
>> the resources.
> 
> Correct. There are two new empty fields per resource that does
> not support monitoring. Having the new mon_domains list results in
> the benefit of eliminating monitoring fields from all the domains
> forming part of resources that do not support monitoring. Providing
> more details below but the additional pointer results in a significant
> net reduction of unused fields. Having the new mon_scope field results
> in the benefit to support SNC.
> 
>>
>> I am not too worried about the size of the patch.  But, I don't
>> foresee these fields will be used anytime soon in these
>> resources(MBA. L3. SMBA). Why add it now? In future we may have to
>> cleanup all these anyways.
> 
> This work does indeed go through the effort to _eliminate_ unused fields.
> Note how all domains of all resources (whether they support monitoring or
> not) currently have to carry a significant number of monitoring fields.
> These can be found in both struct rdt_domain (*rmid_busy_llc, *mbm_total,
> *mbm_local, mbm_over, cqm_limbo, mbm_work_cpu, cqm_work_cpu)  as
> well as struct rdt_hw_domain (*arch_mbm_total, *arch_mbm_local).
> 
> For a resource that does not support monitoring it is of course
> unnecessary to carry all of this for _every_ domain instance and
> after this series it no longer will.

Yes. I see that. Thanks for the explanation. Lets go ahead with the
series. This feature is been pending for a while. I will provide my
comments for series.
-- 
Thanks
Babu Moger