Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Message-ID: <f91ab4e9-726a-4c9b-b91f-6b419414f96c@intel.com>
Date:   Mon, 6 Nov 2023 16:31:42 -0800
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v10 1/8] x86/resctrl: Prepare for new domain scope
Content-Language: en-US
To:     Tony Luck <tony.luck@intel.com>, Fenghua Yu <fenghua.yu@intel.com>,
        "Peter Newman" <peternewman@google.com>,
        Jonathan Corbet <corbet@lwn.net>,
        "Shuah Khan" <skhan@linuxfoundation.org>, <x86@kernel.org>
CC:     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: <20231020213100.123598-1-tony.luck@intel.com>
 <20231031211708.37390-1-tony.luck@intel.com>
 <20231031211708.37390-2-tony.luck@intel.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20231031211708.37390-2-tony.luck@intel.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?NDBNYWNyeE5DVnJnVzhhaCtwVlo5NmNPQ1BXeHJwaVhjaVNJZHBJZm43blBi?=
 =?utf-8?B?RUwzWHFqaGErSkRPMWNPSnFsOGcwZ2lJZm5nTnNhSEo1WFlVQTBubDlBRzFh?=
 =?utf-8?B?azlhWWdIY0pudDRrUFlqZi92dkdiUGNyRXQ5emM2RmYyN1BHMEpWS3h5WFJ3?=
 =?utf-8?B?RW93ekJZMkFjclJPanFCUDAxTTU1enQ2TkZXWmsvRStNaXZudlRNTWFhVUdI?=
 =?utf-8?B?d3ZRZFR1K3lSUXJnd0E5RjM2NGQvVGFORnh3Z21RcTVGbDg4KzZTWkVwT0Rm?=
 =?utf-8?B?cUMySzBPbFo3U0hCNnVaaHVJcHdiZWpoUlU1S1FYdlZNcU40azhhZ2h6WjRw?=
 =?utf-8?B?TW1nNUFDVm9rS090MkxweHROdkhJRzVDVWFQcExnV0JKMFFsTTdxQ2VOdFFO?=
 =?utf-8?B?dldIbm9VajlyWWM2TU1RY3dEbjRCMDMrK3A5WUhtcEp1cUN5WG5DZjBhWDA1?=
 =?utf-8?B?dDdoTmFhTHRUTVUxaG9DR3oyWUxIMmI3TzlSRjZ3dndJTk1JTkhJYVdUczNC?=
 =?utf-8?B?RDZqbUU3ckJLaFdwdkdXMDRObll0Rk1PQTJZbExQUVlJU3BOQXNXdzdrSm9j?=
 =?utf-8?B?ZW0wcUhtV29lWHlqNnhzQ0lXN3o1Tko0OG9kdlB4cDRYVUIyMzA2OTlrVDRl?=
 =?utf-8?B?cTdadDJEUnVYKzcvUGpKdTZIbzVCSTQ5MFE1a3p4Y0NoT3o1U0JDMTZyZzY1?=
 =?utf-8?B?VS9WcnRFdys2SzB0ZGpncnNvQ0hscGllSWk1am1HNFpKWXNSSjZLSWdxN3ox?=
 =?utf-8?B?ZFFNVUVFdkVNSU15NEt0TjlLZWJXWE1nVDVMSnFNODI4OHRFOW5xOUdGQlRp?=
 =?utf-8?B?dmRCNjU3Mkt3R2hNeHFzNmlWUVpGL2x3WWlWRGRUek9tNWpSWklJR1pjM3cy?=
 =?utf-8?B?R3hYYnY4ZTdmUXQyeWl6YUVQdEdoZDZ0eVhFS0tqNDlzWHFhSFM3d0tPZEs1?=
 =?utf-8?B?WlBkbmpNbVV6a3U2aXhZNTFLeXh4ZFhZOGxoV3JtMzl6Yi9NdDZFQ05XeFBq?=
 =?utf-8?B?V0JiUndjMUNEbzBqNmhKNzlnTW9VYS9CUGhJcFRFOHJ2UURQWmNFaytDaTlm?=
 =?utf-8?B?aWNwM2JsYkE5aGtsN2U3eWpSNFlxNDRmQ1ROYXRzZXZ6SnlUTDQvak94OFMy?=
 =?utf-8?B?aU5vVGRlVHBiZnZZTW52VHp1MTVVbVZyR0FzN2NPamljRUgvbTFBNmRXMXhQ?=
 =?utf-8?B?M21OaGt6bE1EWjlpSmRIU2NneHJlVG5tSjBCVXlvRFFTbGc0ekVVTDlXcFpR?=
 =?utf-8?B?ZnRDM0ErWVBxQ1lEeFFPTStkRW1zYlI2K3BGT1ZPRm5YL29yNkMyOEVMaDRk?=
 =?utf-8?B?Q0pSZkpVYW9ZR1JGV0xwMUpzbFFwWWtBRlFqRmVXbzl1S2Z5Kzl0K2t1Zm9u?=
 =?utf-8?B?Y2JpK21TNlRyUDNGTEIwKzdkdFBWR1FjK0d4WTZVRW1iZUxwRElqaVpHb0VI?=
 =?utf-8?B?OEJDN01ZMEpHdUZlcGNkQTB6MWp3NDdtOEZWNXRIUUVPRTY1bTlVWEdCblNE?=
 =?utf-8?B?bmREODdvUDFnZDVJQ2JlZ0g3VGsrM0N2MTA1WUYvakJvVXJZMWxEYXk1TUhU?=
 =?utf-8?B?eW92b1NrTXB0S0tPbUpEZldWcEtScFFhbVhQUGVad1dNTmtreVRNRHo1UUhY?=
 =?utf-8?B?UkFlc0JZMzlmZkJwVmRsNUI5S013bHE1b0ljMGNablhoZk1iUWJqcnBEVHFr?=
 =?utf-8?B?WUxOenIyUlRLeEZ1bmpBZGs1ZXBFZ3Q0eVYyMDRzMWh3Y3JBU1pvOElvNml0?=
 =?utf-8?B?THpubEdrcjZ2WFhBZE55TkVqL29UaGVzcFlJOE93dHpLQ1JpZi9sVFJwV2tT?=
 =?utf-8?B?YnQ3eGkyYklYYm85OE5LeDJUZUlrSnZqY3NOb0JuQXJOZEpkZkhrQUJGRFZU?=
 =?utf-8?B?RlVuNnlUVnBZN0k3N0c2S3E4OEFoQ04za2dQVDY3MnBPRTk0bDRZaC9QSCtQ?=
 =?utf-8?B?NVRBeTRkaDBXR20xcnord0ZmdGtVa0Q4dVhUbmZ6Y2kvS3R3eTJLclBrODRi?=
 =?utf-8?B?cEoxeEpaSm9GVWZvdlVRRkhXZk1yQmVYTk4xMjZldzk1cnpzY2JSQjNiVjNU?=
 =?utf-8?B?VkNuazRiR3lXMm91clJpVUQvb2IwSTIyRXVYa1I0VGVKb3BWeGVsQk9uazdL?=
 =?utf-8?B?VVJ3QXp6V25ZR09RcHB5Tjl0b0NUMGhrUVR2VC9GRkpzc0MvWjlXbHdPaVNY?=
 =?utf-8?B?Y1E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 6439fad4-5936-42bb-1f3b-08dbdf28eb7b
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 07 Nov 2023 00:31:44.7362
 (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: aMxId5CPmCk7Bd3q4QNpGSz6tw+nsPqNaKcEdOLKFgW97N+V+JE1FVcZmC7MfL82F8bO6NeAaO6ro4hnWmVhSCSp7sjzde8CkCIbMLGazWQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH0PR11MB7614
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,
        RCVD_IN_DNSWL_BLOCKED,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE
        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]); Mon, 06 Nov 2023 16:31:56 -0800 (PST)

Hi Tony,

On 10/31/2023 2:17 PM, Tony Luck wrote:
> Resctrl resources operate on subsets of CPUs in the system with the
> defining attribute of each subset being an instance of a particular
> level of cache. E.g. all CPUs sharing an L3 cache would be part of the
> same domain.
> 
> In preparation for features that are scoped at the NUMA node level
> change the code from explicit references to "cache_level" to a more
> generic scope. At this point the only options for this scope are groups
> of CPUs that share an L2 cache or L3 cache.
> 
> Clean up the error handling when looking up domains. Report invalid id's
> before calling rdt_find_domain() in preparation for better messages when
> scope can be other than cache scope. This means that rdt_find_domain()
> will never return an error. So remove checks for error from the callsites.
> 
> Signed-off-by: Tony Luck <tony.luck@intel.com>
> ---
> Changes since v9
> New test for invalid domain id before calling rdt_find_domain() means that
> error handling in that function and at all call-sites can be simplified.

These changes do not appear to be consistent in this series. Simplifying the
call-sites is indeed done in this patch but this work seems to be undone in
patch 3 where it reverts back to the previous error handling in
domain_add_cpu_mon(), domain_remove_cpu_ctrl(), and domain_remove_cpu_mon().

> In pseudo_lock_region_init() use the new enum resctrl_scope for local variable.
> 
>  include/linux/resctrl.h                   |  9 +++--
>  arch/x86/kernel/cpu/resctrl/core.c        | 40 +++++++++++++++--------
>  arch/x86/kernel/cpu/resctrl/ctrlmondata.c |  2 +-
>  arch/x86/kernel/cpu/resctrl/pseudo_lock.c |  6 +++-
>  arch/x86/kernel/cpu/resctrl/rdtgroup.c    |  5 ++-
>  5 files changed, 44 insertions(+), 18 deletions(-)
> 

...

> @@ -506,17 +516,18 @@ static int arch_domain_mbm_alloc(u32 num_rmid, struct rdt_hw_domain *hw_dom)
>   */
>  static void domain_add_cpu(int cpu, struct rdt_resource *r)
>  {
> -	int id = get_cpu_cacheinfo_id(cpu, r->cache_level);
> +	int id = get_domain_id_from_scope(cpu, r->scope);
>  	struct list_head *add_pos = NULL;
>  	struct rdt_hw_domain *hw_dom;
>  	struct rdt_domain *d;
>  	int err;
>  
> -	d = rdt_find_domain(r, id, &add_pos);
> -	if (IS_ERR(d)) {
> -		pr_warn("Couldn't find cache id for CPU %d\n", cpu);
> +	if (id < 0) {
> +		pr_warn_once("Can't find domain id for CPU:%d scope:%d for resource %s\n",
> +			     cpu, r->scope, r->name);
>  		return;
>  	}

Please add empty line here ...

> +	d = rdt_find_domain(r, id, &add_pos);
>  

... and remove this empty line.

>  	if (d) {
>  		cpumask_set_cpu(cpu, &d->cpu_mask);
> @@ -556,12 +567,15 @@ static void domain_add_cpu(int cpu, struct rdt_resource *r)
>  
>  static void domain_remove_cpu(int cpu, struct rdt_resource *r)
>  {
> -	int id = get_cpu_cacheinfo_id(cpu, r->cache_level);
> +	int id = get_domain_id_from_scope(cpu, r->scope);
>  	struct rdt_hw_domain *hw_dom;
>  	struct rdt_domain *d;
>  
> +	if (id < 0)
> +		return;
> +
>  	d = rdt_find_domain(r, id, NULL);
> -	if (IS_ERR_OR_NULL(d)) {
> +	if (!d) {
>  		pr_warn("Couldn't find cache id for CPU %d\n", cpu);

This error message is no longer accurate.

>  		return;
>  	}


Reinette