Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <076a76c9-ab57-cae2-e6e4-99d26921b578@intel.com>
Date:   Tue, 18 Jul 2023 16:45:47 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.13.0
Subject: Re: [PATCH v3 3/8] x86/resctrl: Add a new node-scoped resource to
 rdt_resources_all[]
Content-Language: en-US
To:     Tony Luck <tony.luck@intel.com>
CC:     Fenghua Yu <fenghua.yu@intel.com>,
        Peter Newman <peternewman@google.com>,
        Jonathan Corbet <corbet@lwn.net>,
        Shuah Khan <skhan@linuxfoundation.org>, <x86@kernel.org>,
        Shaopeng Tan <tan.shaopeng@fujitsu.com>,
        James Morse <james.morse@arm.com>,
        Jamie Iles <quic_jiles@quicinc.com>,
        Babu Moger <babu.moger@amd.com>,
        Randy Dunlap <rdunlap@infradead.org>,
        <linux-kernel@vger.kernel.org>, <linux-doc@vger.kernel.org>,
        <patches@lists.linux.dev>
References: <20230713163207.219710-1-tony.luck@intel.com>
 <20230713163207.219710-4-tony.luck@intel.com>
 <769521ee-6907-1613-442f-e2915a8ea554@intel.com>
 <ZLcY1MQr5/Ibnv7N@agluck-desk3>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <ZLcY1MQr5/Ibnv7N@agluck-desk3>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?N2tkOG5rUS9VU29QbEpGMkNxWERFVmdIdU5UMEloY3E1Y2FSVFdjSXp3SmtT?=
 =?utf-8?B?d3NYdHlBbXA3WUxhYjFmRUNYUE1pZFBnNUM2TDdxZFNEMEJ0cXQwaUFxK0Zp?=
 =?utf-8?B?OTRhbFdobnJieUtCc0p4dWNTTVBMN28wY3JOQnRCZWdmRnE2ejMyc0R1b2JP?=
 =?utf-8?B?MDcrM3BLYjF5VnBoenU1cUpyVU9ITXBrYWpWejkybHFzSUxkQVlIeENWMzZu?=
 =?utf-8?B?Mkc2Myt1TGwvYjdsWUh3b3o2S21mbnp3cSs2L0VUTWNTRGsrcndIWk5ZOENy?=
 =?utf-8?B?UE9YWFlVc0VXOUxhRy9HSWlHOUtVRysvZnRONk1ZRGd5TUl3ZE13aXRUSVZX?=
 =?utf-8?B?amgwZ3J6a2RKa2lqMzNzeXNYeVhZblZXWnJCMFlHS1Y3NXZFdER5WlNhU0lF?=
 =?utf-8?B?clB0MVRtZ1RsZzZRMDZaTCtCSHBUbWNDM2djWURYTTV6K1ppMyszN2dYVEtC?=
 =?utf-8?B?b2lVVU95dXdnd3dkeWE2Y3hxVFd2a2h2QjZwVWFiTU5IcW1nVjNiVWdSbExD?=
 =?utf-8?B?UUlsL0dxZFVEeGdvVE5aTVFUcFhDVEFnQ0xOa3VlSjFqZGE5NDFwRjVIdlZv?=
 =?utf-8?B?VC9ZcSttV1NPT2s2ZnQ2QUpSb2E0UzRDeERiSTBYcWxnQWNPZGE2ZzZOd2NG?=
 =?utf-8?B?b0JuQTVrNTFxRkV6QzZreTlqdjExKys4clZCd0FMZ0VYcFplUkFDWWJTQzcv?=
 =?utf-8?B?QkRvdjBsU3Q0N3VCdzhGUVRWRzlxRjRiZXE1aUhmN2NyS1lvVWVFWDh6dEIy?=
 =?utf-8?B?VGoyR05FS09aZEt3M1BYSkhMV3g1MnVpSE9SYmhrSEtWYlAzNnM0T0hHUldU?=
 =?utf-8?B?VnpJcVgyNE9WU0NGQXY0V1hBbXNDTHQwSWJ3NFB1QVFrRUxlYjk5RTZRSU0y?=
 =?utf-8?B?TVowa0JUNHFNRk50RWtqbjB0Rmh5MHY4VlM3WFZ3N0p3RVd3WmZRUnVMYkhQ?=
 =?utf-8?B?WldmWGVDcUIvNlNUNWFQVXYzZElhdTU2d1M2a1pUZndtT0JKRndIREdNTWpW?=
 =?utf-8?B?eFkrK1hzTUV0VkNucDdqSnozNUtQM0pFQVRSQ2p4L3ZrQ296VUM0b1owbGFB?=
 =?utf-8?B?cGVVM05UV2VZNi85THhRTlc0aTFYRFFUVkg2TTJqQ3BLd1JUN2NQby84MGhY?=
 =?utf-8?B?aExiZjA5OWQvdDhHSXpLb1ZDUERXaDJsVFJpZERzNUpZZTJlOXhSK0N2dTNU?=
 =?utf-8?B?cDdpQVd1d0JXVWttV2Q4VFlPWHZwWUtBMFUvT08xdWVjbXgzamo3cFlxZXJP?=
 =?utf-8?B?R2NJZ0VkOWJiS2ZmNUpuUDBabUswS2hkUENHQXI5bUk3TnNIV2RteFJwdU9a?=
 =?utf-8?B?eU5RK015dGIyTERLL1haSEJ6MTMyWU82Y2NwRHdBLytuNEltZ3JnOWVYaGpW?=
 =?utf-8?B?eURIUFRhZEZWWmVzMisrWFRhTVdDSVVURUtvNWJZaWlHZSs2bXU1d1JTYS9x?=
 =?utf-8?B?dkU4SG5idWlVNUtaQWdIZzZncXRQQk5OYzZRVHkyWU4xZ0pmRlJzQ0E5V3JN?=
 =?utf-8?B?TTlkNHFHc2JwazFLQWxua0xBbS9KT0ZjOGdLL01CbFZlQTBYNTY2MEtvZlJj?=
 =?utf-8?B?NklyMXZFSEFBZnV6Tkd1QnA2MU1WQWhZeHVLVFdiQnEvVnVTdTFQdSt4RDFF?=
 =?utf-8?B?T3ExSmZFd3BpNVVTM3B0bzk2cVBxZEZ0MDdSQTVnYllsSEIyeC9zWFZRaExo?=
 =?utf-8?B?cWI5SjRGb2Vkc01sODdjWTkrWDBGWGRDR3h3SFg4Z0NZajJYTkpqVzUzT05m?=
 =?utf-8?B?ckM4OGd4Ynl0Sk1yR2Nrb01RTEtxTUp1M0FuZzNUUTNpYXpicDlCRkFzcmJX?=
 =?utf-8?B?ZFZqNjVENzU4SjZpck1xd2tKenBvVmV4NDlIOUNKSm9wdFpZOGhYaUNVMno4?=
 =?utf-8?B?a1dvU2ZuaWxGZFR2SE9sVC9xb3E0MkhiR3dRdzIvRkxkQUd1SFJET0pKY2cw?=
 =?utf-8?B?Y29ySy9sTTBNTDNqdytGWFAyUzBVMXN2NUlCZWdBOEw5VTk3YUZWZjVyQUJV?=
 =?utf-8?B?Ymp6UXJ3UHdORERVVDNXOEtMMVZmeWk0QW1oTUhnTjVoRlN5bkRkRlo4UTVG?=
 =?utf-8?B?ZStYWFpjZTROajQ3K0NQaHF4WDFtQy9SWWhrVkVIY0hzRHZ2QnFGK3hWU1hQ?=
 =?utf-8?B?UlBQU0drclp6byt3bjVzYmVUWm9DSjluWEFqRGhMUUFWcnh3OW9HQWEzMUdK?=
 =?utf-8?B?SUE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f0979587-07d3-406e-f3ec-08db87e91dc3
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 18 Jul 2023 23:45:50.2053
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: MuIA8etuLwzohzvDf/R9efhIjsgLqOXyi8YwOAq0ntX3j4ShLG/OBJkdUm2WtIxtVrP1Y50RJtiDndQXPO6N3/5vJNVTRxyGHCM2tVFcX/o=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7460
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.5 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,
        RCVD_IN_DNSWL_MED,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE,
        URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi Tony,

On 7/18/2023 3:57 PM, Tony Luck wrote:
> On Tue, Jul 18, 2023 at 01:40:32PM -0700, Reinette Chatre wrote:
>>> +	[RDT_RESOURCE_NODE] =
>>> +	{
>>> +		.r_resctrl = {
>>> +			.rid			= RDT_RESOURCE_NODE,
>>> +			.name			= "L3",
>>> +			.scope			= SCOPE_NODE,
>>> +			.domains		= domain_init(RDT_RESOURCE_NODE),
>>> +			.fflags			= 0,
>>> +		},
>>> +	},
>>>  };
>>
>> So the new resource has the same name, from user perspective,
>> as RDT_RESOURCE_L3. From this perspective it thus seems to be a
>> shadow of RDT_RESOURCE_L3 that is used as alternative for some properties
>> of the actual RDT_RESOURCE_L3? This is starting to look as though this
>> solution is wrenching itself into current architecture.
>>
>> >From what I can tell the monitoring in SNC environment needs a different
>> domain list because of the change in scope. What else is needed in the
>> resource that is different from the existing L3 resource? Could the
>> monitoring scope of a resource not instead be made distinct from its
>> allocation scope? By default monitoring and allocation scope will be
>> the same and thus use the same domain list but when SNC is enabled
>> then monitoring uses a different domain list.
> 
> Answering this part first, because my choice here affects a bunch
> of the code that also raised comments from you.

Indeed.

> 
> The crux of the issue is that when SNC mode is enabled the scope
> for L3 monitoring functions changes to "node" scope, while the
> scope of L3 control functions (CAT, CDP) remains at L3 cache scope.
> 
> My solution was to just create a new resource. But you have an
> interesing alternate solution. Add an extra domain list to the
> resource structure to allow creation of distinct domain lists
> for this case where the scope for control and monitor functions
> differs.
> 
> So change the resource structure like this:
> 
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 8334eeacfec5..01590aa59a67 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -168,10 +168,12 @@ struct rdt_resource {
>  	bool			alloc_capable;
>  	bool			mon_capable;
>  	int			num_rmid;
> -	int			cache_level;
> +	int			ctrl_scope;
> +	int			mon_scope;

I am not sure about getting rid of cache_level so fast.
I see regarding the current problem being solved that
ctrl_scope would have the same values as cache_level but
I find that adding this level of indirection while keeping
the comparison with cacheinfo->level to create a trap
for future mistakes.


>  	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;
> 
> and build/use separate domain lists for when this resource is
> being referenced for allocation/monitoring. E.g. domain_add_cpu()
> would check "r->alloc_capable" and add a cpu to the ctrl_domains
> list based on the ctrl_scope value. It would do the same with
> mon_capable / mon_domains / mon_scope.
> 
> If ctrl_scope == mon_scope, just build one list as you suggest above.

Yes, this is the idea. Thank you for considering it. Something else
to consider that may make this even cleaner/simpler would be to review
struct rdt_domain and struct rdt_hw_domain members for "monitor" vs "control"
usage. These structs could potentially be split further into separate
"control" and "monitor" variants. For example, "struct rdt_domain" split into
"struct rdt_ctrl_domain" and "struct rdt_mon_domain". If there is a clean
split then resctrl can always create two lists with the unnecessary duplication
eliminated when two domain lists are created. This would also
eliminate the need to scatter ctrl_scope == mon_scope checks throughout. 

> 
> Maybe there are more places that walk the list of control domains than
> walk the list of monitor domains. Need to audit this set:
> 
> $ git grep list_for_each.*domains -- arch/x86/kernel/cpu/resctrl
> arch/x86/kernel/cpu/resctrl/core.c:     list_for_each_entry(d, &r->domains, list) {
> arch/x86/kernel/cpu/resctrl/core.c:     list_for_each(l, &r->domains) {
> arch/x86/kernel/cpu/resctrl/ctrlmondata.c:      list_for_each_entry(d, &r->domains, list) {
> arch/x86/kernel/cpu/resctrl/ctrlmondata.c:      list_for_each_entry(d, &r->domains, list) {
> arch/x86/kernel/cpu/resctrl/ctrlmondata.c:      list_for_each_entry(dom, &r->domains, list) {
> arch/x86/kernel/cpu/resctrl/monitor.c:  list_for_each_entry(d, &r->domains, list) {
> arch/x86/kernel/cpu/resctrl/pseudo_lock.c:              list_for_each_entry(d_i, &r->domains, list) {
> arch/x86/kernel/cpu/resctrl/rdtgroup.c:         list_for_each_entry(dom, &r->domains, list)
> arch/x86/kernel/cpu/resctrl/rdtgroup.c: list_for_each_entry(dom, &r->domains, list) {
> arch/x86/kernel/cpu/resctrl/rdtgroup.c:         list_for_each_entry(d, &r->domains, list) {
> arch/x86/kernel/cpu/resctrl/rdtgroup.c:         list_for_each_entry(d, &r->domains, list) {
> arch/x86/kernel/cpu/resctrl/rdtgroup.c: list_for_each_entry(dom, &r->domains, list) {
> arch/x86/kernel/cpu/resctrl/rdtgroup.c: list_for_each_entry(d, &r->domains, list) {
> arch/x86/kernel/cpu/resctrl/rdtgroup.c: list_for_each_entry(d, &r_l->domains, list) {
> arch/x86/kernel/cpu/resctrl/rdtgroup.c: list_for_each_entry(d, &r->domains, list) {
> arch/x86/kernel/cpu/resctrl/rdtgroup.c:         list_for_each_entry(dom, &r->domains, list)
> arch/x86/kernel/cpu/resctrl/rdtgroup.c: list_for_each_entry(d, &r->domains, list) {
> arch/x86/kernel/cpu/resctrl/rdtgroup.c: list_for_each_entry(dom, &r->domains, list) {
> arch/x86/kernel/cpu/resctrl/rdtgroup.c: list_for_each_entry(d, &s->res->domains, list) {
> arch/x86/kernel/cpu/resctrl/rdtgroup.c: list_for_each_entry(d, &r->domains, list) {
> 
> Maybe "domains" can keep its name and make a "list_for_each_monitor_domain()" macro
> to pick the right list to walk?

It is not clear to me how "domains" can keep its name. If I understand
the macro would be useful if scope always needs to be considered. I wonder
if the list walkers may not mostly just walk the appropriate list directly
if resctrl always creates separate "control domain" and "monitor domain"
lists.

> I don't think this will reduce the amount of code change in a
> significant way. But it may be conceptually easier to follow
> what is going on.

Reducing the amount of code changed is not a goal to me. If I understand
correctly I think that adapting resctrl to support different monitor and
control scope could create a foundation into which SNC can slot in smoothly. 

Reinette