Received-SPF: pass (google.com: domain of linux-kernel+bounces-215497-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Message-ID: <a65739a4-1c0e-ab36-611d-e2da0bd1d00a@amd.com>
Date: Fri, 14 Jun 2024 16:29:29 -0500
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.5.0
Reply-To: babu.moger@amd.com
Subject: Re: [PATCH v20 00/18] Add support for Sub-NUMA cluster (SNC) systems
Content-Language: en-US
To: Reinette Chatre <reinette.chatre@intel.com>, babu.moger@amd.com,
 Tony Luck <tony.luck@intel.com>, Fenghua Yu <fenghua.yu@intel.com>,
 Maciej Wieczor-Retman <maciej.wieczor-retman@intel.com>,
 Peter Newman <peternewman@google.com>, James Morse <james.morse@arm.com>,
 Drew Fustini <dfustini@baylibre.com>, Dave Martin <Dave.Martin@arm.com>
Cc: x86@kernel.org, linux-kernel@vger.kernel.org, patches@lists.linux.dev
References: <20240610183528.349198-1-tony.luck@intel.com>
 <658d9869-ef22-48a7-876a-5bbba4f134ff@amd.com>
 <bc52a051-4296-48ac-9a83-29139855553f@intel.com>
 <e17d3a79-9c07-44cd-b0ea-2d0545680342@amd.com>
 <f57e9cf2-35b6-401d-afc2-8d11b22836c2@intel.com>
From: "Moger, Babu" <bmoger@amd.com>
In-Reply-To: <f57e9cf2-35b6-401d-afc2-8d11b22836c2@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0:
	=?utf-8?B?ZlhoOXMzNy8xb2NTZCtkK0tOcnZYN1NYa20rQU1DeDZwSlZmL3N1ZFRHSytL?=
 =?utf-8?B?bGpXcUIxcTYvT0czWUJubDBzTzhRdEZ6OHlzdkpQNXRTTzJzVGtybWNmUkRy?=
 =?utf-8?B?YmFVellBaEhsOWFybkFyaExVT2FIMkhYMXlEbVpib04wdEFYV0ZOY2ZKenY0?=
 =?utf-8?B?MnkxS0pSc3FZMFpVbGtkc0hhMFdsYk5OK1V1SlBic3ZxUzRGbEx0aENNZndN?=
 =?utf-8?B?V0t0Mnd0MUR1U0dMZGVXOXZzUzJBTUhTYkJYVHBqWkU3a2tCQVhiVSt4eXk2?=
 =?utf-8?B?YnMyK25xd1MyS2kyaFoveFVtZkpEZHA4Qk0wVnFHb0duTlc5bVFwVGZBTnow?=
 =?utf-8?B?NURPWE5zLzExTWc5dDlramtNRndaZXNOV1hpMERianhmUUJzZEpKdVcyWER5?=
 =?utf-8?B?aTlGSWE2eXZxL211eEtIenVpTzJIYjBaS2ZNd3piSFlyQ0Z2VFpNUHdkeW1L?=
 =?utf-8?B?SEVMSElzKytMQ2lUK29Vcm02RXhNRHREeVNRQ0FWRlpuMkFvNTRBQWQ2b0xy?=
 =?utf-8?B?bER0MWJrelpGRUdxUWZTRWNtMzBCaGg5K0tGZlZNdmdZSUkwN0s3NnZtM0Vr?=
 =?utf-8?B?Mm1vcFluUytuUTVpdzZvNTBZMUw4REJNSUpUajdiWERSb1VVam9wQyt6anQ0?=
 =?utf-8?B?Vk0vdHhGd2JuTmpKblJaeTBha0lva045U3h6bDkreDZGT2hUYmlVaW9vNUY4?=
 =?utf-8?B?VEtFZy8vdGltcWw3TWFWVXBlQ003alNUbi9UY0kxYVpVSWw3bHcrSElhbHll?=
 =?utf-8?B?dXdVT0RQcXBSYlV1cmdHRVQvZm83dVdkcDNDdDc2SnBSRS81TzlXa1dkcVps?=
 =?utf-8?B?Skl3QTNQMUVQQ2dTNGZ0ZmdteFEyWFZCYzVvYk5CWStrVWZuZkV2S1dUVkNa?=
 =?utf-8?B?amQwMHNUV24yY1JIRHpINllON1hHdzduZWVLVG91VXhsQlBVYWZSeFRQVk1W?=
 =?utf-8?B?dHQ0dVl1T3NWVnV0S1ZqcHZ1TERCMldwT08vYVR5OEhKMVdDcVppdHFCL2d0?=
 =?utf-8?B?WTNET3c2ZzUrNlJiSGtsTTRWNkczazdRamdkaXA1bjNhbnA3UW56U1hWeHB2?=
 =?utf-8?B?NDBycE50d2grTjYyN3U0R1BPVXdKSlJ2R1V2dzJCejN5alJQMnRsT0twc0I1?=
 =?utf-8?B?K1NlY2hYK0Nia2lNRXEydHVnR2RFbm1HNVFGNVQ5RHFoTFRZODZIaVFyc1ZV?=
 =?utf-8?B?OHo0R1Z5UkMzL3AwVTJaWDJZVUhGVUdLV1E2T25oczNBbWNTaVZZeFN3bVFm?=
 =?utf-8?B?M010dm9GNW9jZE9DM0VkNXFDdVlCbVQyejBpRnc4bWs3QU93MUpUdFJnUlRP?=
 =?utf-8?B?OU9weHlTR1kvY2o1Z0VrZ0dJQlNnLzVSMklZSXh5TzcrTTQ1aXRNR0lFL245?=
 =?utf-8?B?WkxRd25Oc0hIRDZjaEdtTFZ4ZGNBWVVMTnNGVkdTaG1kZktaL09ZOTNPRmV6?=
 =?utf-8?B?WUVrcGsxVE9FWkdwSmlkckg2bVZ1MHNMNjJ0UkZZYVdkQVc5STR5QmM3bGc5?=
 =?utf-8?B?alp0RUZUZkN6RUlXVFpKYnZ4Y0RCbC9uZ2VxY28yRmRqcXg1TDNTSUU4QzRw?=
 =?utf-8?B?Y1FCWFN2cXRJbEhRakRmWFUzTHVrTE95OGdCYzZDVkJOenUyY1AxVzVBKy9U?=
 =?utf-8?B?WVJJVkw3Y1ZibjFGRkdGVGtkNnBFcW9RRE5ySXJLMGtwTithSXNESFZCMmls?=
 =?utf-8?B?M0htMXlQd1pMNjBxeldpMWJsL3UvMTNmRkUyVks4Q21TdEVtNG4vL2xmcENo?=
 =?utf-8?B?WXRUMGJuSlZNckwyN0d3MTBRU3BHLzE3SVFreHhWbGludUdUNUpsSEZSNmU0?=
 =?utf-8?B?WFVobXAvQVhLMVp3UnVrRVloY0t2aDZhTE4rZm0zK3BjeEl2TGIyd3QzaU1J?=
 =?utf-8?B?WDhST0hicWVEaUViLyswc3IydHdwWkcxN2xnU25jWm0rRG5vL2FJWk9uakVP?=
 =?utf-8?B?eUNONXBkZ24zY0ppdkxTOWY2VUcwTEdNR0liSGtmci9ydjJzeGVWOXlOMHVn?=
 =?utf-8?B?SlFVM3A5bkJ5NGdTMytrY0dFaUZGZHAya2pKQkduM0Y5Mk1ONzFHRFJ1M0lN?=
 =?utf-8?B?UFhTcUhQbmlXcEt0YnJ0NVVobUlFWjNpN2N0YUtBcXZ3VnpBTTNLOXBlOVpI?=
 =?utf-8?Q?FJiXZTe7KSMC0xU14HCoxeJBv?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: bc28d5a7-fa10-4366-bb33-08dc8cb91513
X-MS-Exchange-CrossTenant-AuthSource: MW3PR12MB4553.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 14 Jun 2024 21:29:33.1529
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: TkvPek3xxdLHt8Nrk6B2cy2zJ5pO7NftGevUJdBgGNMeTTQvYfic1v6k8LmiMUsG
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BY5PR12MB4067

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.

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.

> 
>>
>> But complexity wise, approach (a) adds quite bit of complexity. 
>> Doesn't it?
> 
> "complex" is a subjective term. Could you please elaborate what is complex
> about this? Is your concern about the size of the patch? To me that is
> not a concern when considering the end result of how the resctrl structures
> are organized.
> 
>>
>> For me, solution (b) looks simple and easy. Eventually, we may have to
>> restructure these data structures anyways. I feel it is the right 
>> direction.
>>
> 
> I understand that it is tempting to look for smallest patch possible but we
> really need to ensure that any work integrates well into resctrl. Doing
> so may end up with larger patches but in the end it makes the data 
> structures
> and code easier to understand. I specifically find the duplication of 
> structures
> troublesome since that requires developers to always be on high alert of
> what code is being worked on and what flows the particular code 
> participates in
> since the duplication results in members of structures be invalid based 
> on which
> code flow is used. To me this is an unnecessary burden on developers and 
> against
> your original goal of making resctrl easier to maintain.
> 
> Reinette

-- 
- Babu Moger