From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by smtp.subspace.kernel.org (Postfix) with ESMTP id ECD6C219A9B for ; Fri, 20 Dec 2024 18:10:53 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=217.140.110.172 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734718256; cv=none; b=lcXIytGx6FGmtXdg3TgqyqfKn7De40BKIDUGFVu4M4e0ni2sByH7CQcduqUysN+4bQp0uZV8rnfgff5NeM2BGTavIjJp0rACJ9/nx96Z54UJN9ERQYH2S8aTV9h/1aDNZ1uyKqPNBlx02n2pabDo3ZfvWhm/uKNuGwPIUSqmoUc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1734718256; c=relaxed/simple; bh=rWvMKDc4r5SoyT55msKQ1jjAV45ybEhhFTuTozP4D3Q=; h=Message-ID:Date:MIME-Version:Subject:To:Cc:References:From: In-Reply-To:Content-Type; b=c61V9kk317TZRarWkXvtyIyb7cjeMeh1L5csZjN43MDB9ti7NUk6k/G97ObDJBFh/vXsohso6+FYTPn8MREmnW9JLP5lQjri7t4J2Tnd+ZxEwKkSKrtRliYs5wA0Xm+zlp8arBbv66Guu6KfPGg40rGLAcnXcMeWBgXOXyBkE9c= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com; spf=pass smtp.mailfrom=arm.com; arc=none smtp.client-ip=217.140.110.172 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=arm.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 247581480; Fri, 20 Dec 2024 10:11:21 -0800 (PST) Received: from [10.1.196.57] (eglon.cambridge.arm.com [10.1.196.57]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id E425F3F720; Fri, 20 Dec 2024 10:10:48 -0800 (PST) Message-ID: <1b01d18f-c398-4706-9da6-e920512426a1@arm.com> Date: Fri, 20 Dec 2024 18:10:47 +0000 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v5 07/40] x86/resctrl: Add max_bw to struct resctrl_membw To: Reinette Chatre , x86@kernel.org, linux-kernel@vger.kernel.org Cc: Fenghua Yu , Thomas Gleixner , Ingo Molnar , Borislav Petkov , H Peter Anvin , Babu Moger , shameerali.kolothum.thodi@huawei.com, D Scott Phillips OS , carl@os.amperecomputing.com, lcherian@marvell.com, bobo.shaobowang@huawei.com, tan.shaopeng@fujitsu.com, baolin.wang@linux.alibaba.com, Jamie Iles , Xin Hao , peternewman@google.com, dfustini@baylibre.com, amitsinght@marvell.com, David Hildenbrand , Rex Nie , Dave Martin , Shaopeng Tan References: <20241004180347.19985-1-james.morse@arm.com> <20241004180347.19985-8-james.morse@arm.com> <8d035aca-b417-42ab-9c42-759c2695b45a@intel.com> Content-Language: en-GB From: James Morse In-Reply-To: <8d035aca-b417-42ab-9c42-759c2695b45a@intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Hi Reinette, On 23/10/2024 22:14, Reinette Chatre wrote: > On 10/4/24 11:03 AM, James Morse wrote: >> __rdt_get_mem_config_amd() and __get_mem_config_intel() both use >> the default_ctrl property as a maximum value. This is because the >> MBA schema works differently between these platforms. Doing this > The schema works differently but they can still use the same property > as maximum, is that a problem? I think its a problem for user-space - but its an existing problem. Today resctrl uses the default as the maximum. This makes that property explicit. >> complicates determining whether the default_ctrl property belongs >> to the arch code, or can be derived from the schema format. > > So instead of Intel and AMD both using default_ctrl as a maximum this patch > introduces a new max_bw with both using that as maximum instead. > Unclear how this change fixes the unclear complication. Is the default value something that can be determine from the schema format? Previously, no - because the default value is where the per-platform maximum bandwidth is stashed. By making that explicit, we can drop the spoon feeding the architecture code has to do to tell resctrl that the maximum bitmap is all-ones, and the maximum percentage is 100. Bandwidth is always going to be weird like this, so it makes sense to special case it. I'll add some form of this text to the commit message. >> Add a max_bw property for x86 platforms to specify their maximum >> MBA bandwidth. This isn't needed for other schema formats. > > It is not clear to me how replacing one value with a new value that is > used in exactly the same way addresses the initial complaint of complication. > >> >> This will allow the default_ctrl to be generated from the schema >> properties when it is needed. >> diff --git a/arch/x86/kernel/cpu/resctrl/core.c b/arch/x86/kernel/cpu/resctrl/core.c >> index 4c16e58c4a1b..e79807a8f060 100644 >> --- a/arch/x86/kernel/cpu/resctrl/core.c >> +++ b/arch/x86/kernel/cpu/resctrl/core.c >> @@ -248,6 +249,8 @@ static bool __rdt_get_mem_config_amd(struct rdt_resource *r) >> cpuid_count(0x80000020, subleaf, &eax, &ebx, &ecx, &edx); >> hw_res->num_closid = edx + 1; >> r->default_ctrl = 1 << eax; >> + r->schema_fmt = RESCTRL_SCHEMA_RANGE; > > Stray change? Yes, when these were different values, AMD had to override the value in the table. Since they got merged back together, its the same. Thanks! >> + r->membw.max_bw = 1 << eax; >> >> /* AMD does not use delay */ >> r->membw.delay_linear = false; >> diff --git a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c >> index 8d1bdfe89692..56c41bfd07e4 100644 >> --- a/arch/x86/kernel/cpu/resctrl/ctrlmondata.c >> +++ b/arch/x86/kernel/cpu/resctrl/ctrlmondata.c >> @@ -108,8 +108,9 @@ static int parse_bw(struct rdt_parse_data *data, struct resctrl_schema *s, >> */ >> static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r) >> { >> - unsigned long first_bit, zero_bit, val; >> + u32 supported_bits = BIT_MASK(r->cache.cbm_len + 1) - 1; > > Same issue as V4: > https://lore.kernel.org/all/ca528ebd-fb76-40cd-a495-88c2de443cd8@intel.com/ Huh. I was sure I'd fixed that when you first pointed it out. Sorry about that! >> @@ -118,7 +119,7 @@ static bool cbm_validate(char *buf, u32 *data, struct rdt_resource *r) >> return false; >> } >> >> - if ((r->cache.min_cbm_bits > 0 && val == 0) || val > r->default_ctrl) { >> + if ((r->cache.min_cbm_bits > 0 && val == 0) || val > supported_bits) { >> rdt_last_cmd_puts("Mask out of range\n"); >> return false; >> } > > The above two changes have nothing to do with memory bandwidth. They are unrelated > to the changelog. Yes, these should be in the next patch. Thanks! James