Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.37 as permitted sender) client-ip=23.128.96.37;
Message-ID: <a362e485-f063-e53e-87e7-ac87bca9baac@intel.com>
Date:   Tue, 3 Oct 2023 14:11:39 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.15.1
Subject: Re: [PATCH v6 05/24] x86/resctrl: Track the closid with the rmid
Content-Language: en-US
To:     James Morse <james.morse@arm.com>, <x86@kernel.org>,
        <linux-kernel@vger.kernel.org>
CC:     Fenghua Yu <fenghua.yu@intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        H Peter Anvin <hpa@zytor.com>,
        Babu Moger <Babu.Moger@amd.com>,
        <shameerali.kolothum.thodi@huawei.com>,
        D Scott Phillips OS <scott@os.amperecomputing.com>,
        <carl@os.amperecomputing.com>, <lcherian@marvell.com>,
        <bobo.shaobowang@huawei.com>, <tan.shaopeng@fujitsu.com>,
        <xingxin.hx@openanolis.org>, <baolin.wang@linux.alibaba.com>,
        Jamie Iles <quic_jiles@quicinc.com>,
        Xin Hao <xhao@linux.alibaba.com>, <peternewman@google.com>,
        <dfustini@baylibre.com>, <amitsinght@marvell.com>
References: <20230914172138.11977-1-james.morse@arm.com>
 <20230914172138.11977-6-james.morse@arm.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20230914172138.11977-6-james.morse@arm.com>
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?SjJ5M0dtWkZZQ0J0UUdTbTdkc2RsOURoT01OenhTQkw0My8vZXVaNnhERVcv?=
 =?utf-8?B?cVNUZW5zT3BObXdZMlJ2SlVxaUovRUpTWThGc0RtNDUvWk5XMHV2YkZsUWFC?=
 =?utf-8?B?azF1cjlldDFrdHNrWmVPTkl2aFo4RjNZcVBHc2xYSWRlR3hMODdpMjFOWnlY?=
 =?utf-8?B?KzZ3NVFTL3NLRGpvb0FKMHRwU2h5MXJob2lrMC9KMlNhODQzZkJBMWJxNnlK?=
 =?utf-8?B?VnROWndoUGlScjZ3QkpSUjVxUmhONGF2cEFLM2JFVUlrMFRESUVqRFZLQmFL?=
 =?utf-8?B?V2RHNjBPTEpBNTlHUUVIOExjZkNJSXBoZ1poUXhLSnJidWE5MUZTYkhZSUxC?=
 =?utf-8?B?NlV0cGIvZGpLalYxREhlcE04VCswdkMrOC9RSlhBSGxzbExuajRpR0kzNVFS?=
 =?utf-8?B?UWpsNThMVnlRNnRRTldXalM5WjNKMjNYTVZ2TWptQ3V2M1BFT0VYQ0Vvck1K?=
 =?utf-8?B?Z0wrWHpmQ1BHRWtIbDhkL255RXVqcTlWd3NmOHZCMW5BRmt3L3FDQWp5aXVy?=
 =?utf-8?B?NDVXSjgzYXhpUDZsRHdOcUh4OHh2TTViVVk3NUZKekVXTmhVWjMvb2NEd29i?=
 =?utf-8?B?ZzViSU1kUXJpRG9BLzR1empmWXBRNlpOUlpSL1EyUk1vYkg2UnZ4MG1iTGFr?=
 =?utf-8?B?QWFqN3RHdVlnL0l4bHg3Vjhhd0dzWEVjZmxEZ3FCUDh4UmxpZS9RY2EwcHE1?=
 =?utf-8?B?VUxjc3hmY29jKzFpSmtnOVJ0OFVYY081dUlIRCtHUXNpMmVQVWJneVRqZzQ4?=
 =?utf-8?B?cHdCMENhZURzaXF6dTNaWldoc3EvRjMrQ2h4SldONkt3QmRLT0FiaWRvdW42?=
 =?utf-8?B?TTV3cE5rZTRMTUFWRXQrbVBJT0pzZHZ0OGFhd3ZiOE1xQWY2ZE02dTZJdGJG?=
 =?utf-8?B?MXFwZ2tSK1FRRWxaYTNEckJtblQzU1ZLYUlxSGhIUTU0UCtNREI3QzJNeFl3?=
 =?utf-8?B?Zm9BTEkvT2JwamlLdjE1MnlmYUt6SERmRDNaYktXL3hJUjk2Q0RMVTJObHpj?=
 =?utf-8?B?dEhxYXB6TXdkRUtaOEJ4SmI0OWVPZlp2Um5pWnR4SllUWjQ2L0RlUE5SNzVE?=
 =?utf-8?B?eWNaOEZKZVo1ZEFKempLbWRVMlRuTFhONGxzYzBoVG1zczQ1NFJ2cXBlb0l4?=
 =?utf-8?B?S04rRlI1eXN2YTQxZFh5WEZ2Smkzb1BFMGZ6aGdnZ1Z5Mml6Um5qb2ZaWWQv?=
 =?utf-8?B?VitJTDBCcVl1ZDJKK2JJdGJteTJKY2RPYS9LRzB3MjBEeVlZVW5Ec1d1elNX?=
 =?utf-8?B?bERnazU5V0xOWDRmWUkwaDg4b3NQeG9xaWhzcysyeHBPemVVY0Z5dzIrb3ZT?=
 =?utf-8?B?OWt0TWRBaDRsU0dKUlByeTRZYlVwU0tDSkU5QUFvV2Q2bklxeGJUdDZaY3Vq?=
 =?utf-8?B?VmVRZGp1NW9NWm5sc20yeE9zNk45by9XYUkxaEpvZUlYYTkxTGJjZnZBdGdU?=
 =?utf-8?B?RG5nZEhxYk9VekFESHY3OGNKVXRCbHNmS2E2dFIzZENhZGZvUWUydCtkMnFQ?=
 =?utf-8?B?M2t1aHJYeTBhQ0tIWi9hOE5RYmowc1liWEROeG1SVWorbDNJY2tseTI2UFhQ?=
 =?utf-8?B?WlVQd0txTWNhZXhrYmRtMDZwNVBLWHFvSHE4NjNDckIxMlIwcEhsZW51WUhL?=
 =?utf-8?B?aFNNVWVkc0NVRlBnSytBUEkwbnRQbUwzY1Ixc0xPdFV5WFAwMVdyS3VhL3hR?=
 =?utf-8?B?clYzTHpkbk9kWlNxTmIwemwwMmk2Ym1XMzVjTno5YmJOYUdST01nN2JhRkVz?=
 =?utf-8?B?aGh2YnhjbW5EcmNpcVhPcDNvWFlEaXV4T3dSenhDcnJtNUFKanJEMDlYRm9a?=
 =?utf-8?B?ZmtYQjBhNWNYK1R2OGUyeXhLdThpaXd3cmtDZUVGS2VpdldPa0wybzRmUWRa?=
 =?utf-8?B?VG5zSE1JbTVqVDNlRzhReDJKemFvcE1TLzlmM3p3N2Z5TWE0dEc5NERBTFlY?=
 =?utf-8?B?TW1oT3hXWCtCY3lReWE2THZ2cWNrRUY2MkttUGkySmNqd3p0NUJoU0sya1RJ?=
 =?utf-8?B?NXZENXljSm1RcHliT1A3SmtIOXFSaVBxM0pxaEZuVEJtNDJ0Uy9kVFNJaGc4?=
 =?utf-8?B?TjRvQ0N0M3FJc3dVc082ZnpYSVJ3MDhlb3Y2RmdrY0svSkNRWG9HV09MWnp5?=
 =?utf-8?B?ajZwT0RPbG51TEdtUEVXanV5R0pOaVd2dlhSSzJxbnZhbE5nazFCdVRUQkNE?=
 =?utf-8?B?ckE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 92405f64-8b8c-4498-e9b8-08dbc4555834
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 03 Oct 2023 21:11:43.5515
 (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: io8Q8SAHrZAMqntfshSZ/V+6gJ3m3QAsA2bMA6+K7fDWR4DaUuEH9EbXpB1rkIV4leMSCr5vK/1HaXRP5FfwdpjT57/HVWF0EbZ9l/yuLbw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DS0PR11MB7577
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-5.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,RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,
        SPF_NONE 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
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Tue, 03 Oct 2023 14:11:51 -0700 (PDT)

Hi James,

On 9/14/2023 10:21 AM, James Morse wrote:

> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index cfb3f632a4b2..42b9a694fe2f 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -24,7 +24,21 @@
>  
>  #include "internal.h"
>  
> +/**
> + * struct rmid_entry - dirty tracking for all RMID.
> + * @closid:	The CLOSID for this entry.
> + * @rmid:	The RMID for this entry.
> + * @busy:	The number of domains with cached data using this RMID.
> + * @list:	Member of the rmid_free_lru list when busy == 0.
> + *
> + * Some architectures's resctrl_arch_rmid_read() needs the CLOSID value
> + * in order to access the correct monitor. @closid provides the value to
> + * list walkers like __check_limbo(). On x86 this is ignored.

I do not think this is correct. At this point in the series
__check_limbo() uses @rmid as index, at end of series it uses the
(@closid, @rmid) index. Never does the list walker use @closid.

Perhaps something like below that matches your later similar comments:

	Depending on the architecture the correct monitor is accessed
	using both @closid and @rmid, or @rmid only.


...

  
> @@ -685,11 +706,11 @@ void mbm_handle_overflow(struct work_struct *work)
>  	d = container_of(work, struct rdt_domain, mbm_over.work);
>  
>  	list_for_each_entry(prgrp, &rdt_all_groups, rdtgroup_list) {
> -		mbm_update(r, d, prgrp->mon.rmid);
> +		mbm_update(r, d, prgrp->closid, prgrp->mon.rmid);
>  
>  		head = &prgrp->mon.crdtgrp_list;
>  		list_for_each_entry(crgrp, head, mon.crdtgrp_list)
> -			mbm_update(r, d, crgrp->mon.rmid);
> +			mbm_update(r, d, crgrp->closid, crgrp->mon.rmid);
>  
>  		if (is_mba_sc(NULL))
>  			update_mba_bw(prgrp, d);
> @@ -732,10 +753,11 @@ static int dom_data_init(struct rdt_resource *r)
>  	}
>  
>  	/*
> -	 * RMID 0 is special and is always allocated. It's used for all
> -	 * tasks that are not monitored.
> +	 * RESCTRL_RESERVED_CLOSID and RESCTRL_RESERVED_RMID are special and
> +	 * are always allocated. These are used for rdtgroup_default control
> +	 * group, which will be setup later. See rdtgroup_setup_root().
>  	 */

This comment will not be accurate after Babu's changes are merged (the function will
be rdtgroup_setup_default()). To avoid that conflict you could perhaps change
last two sentences to something like below that will be accurate no matter the
order of merging between your and Babu's work:

	These are used for rdtgroup_default control group, which will be
	setup later in rdtgroup_init().

> -	entry = __rmid_entry(0);
> +	entry = __rmid_entry(RESCTRL_RESERVED_CLOSID, RESCTRL_RESERVED_RMID);
>  	list_del(&entry->list);
>  
>  	return 0;


My feedback only relates to the comments. The rest of the patch looks good to
me. I could give a review tag with expectation that comments be addressed in next
version but since some review feedback fell through the cracks in this version I
feel that I need to confirm first before providing review tag.

Reinette