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

[Reinette Chatre <reinette.chatre@intel.com>]

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/

Reinette