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: <3b19ca17-e56c-0be3-f14d-d5485c5fb020@intel.com>
Date:   Tue, 23 Aug 2022 09:42:37 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.12.0
Subject: Re: [PATCH v5 07/21] x86/resctrl: Abstract and use
 supports_mba_mbps()
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>,
        <lcherian@marvell.com>, <bobo.shaobowang@huawei.com>,
        <tan.shaopeng@fujitsu.com>, Jamie Iles <quic_jiles@quicinc.com>,
        Cristian Marussi <cristian.marussi@arm.com>,
        "Xin Hao" <xhao@linux.alibaba.com>, <xingxin.hx@openanolis.org>,
        <baolin.wang@linux.alibaba.com>
References: <20220622164629.20795-1-james.morse@arm.com>
 <20220622164629.20795-8-james.morse@arm.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20220622164629.20795-8-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?UENadmFmaDRtVy90dXN3aHd3NW9iSmcxVFJhTlBuUitBLzlyVUNLaVZIaDkv?=
 =?utf-8?B?Wi92UjlYYjRXZ3B2amVETDg3cGU4ZnBLZENiRVBRM0QwWGwwZFkrcHc1bHV6?=
 =?utf-8?B?NGZTejFERm9NWGRaaExzU0o5MzFrQ2JzWHRqd2UxQWtBMEVXbTdpdCtmbElT?=
 =?utf-8?B?a3pxQVcyOHNNeCt4OTBoMlNpbURwSTE0Qzdscit6RjlOMmxRakJyMlhOL24w?=
 =?utf-8?B?Z3JFNkhFL01IUnF3ZUJNMkpnSUtHckUxVEZTOS9FSGd0dHpLclNUZS9XUENX?=
 =?utf-8?B?OVI2MFJ2NmZHUHB6UkRpNWNOSURhNEl6bjQzOURoc1l1emk2dVlZelVybUhl?=
 =?utf-8?B?NHFSaGxtOGJrd3VhU1pXb1BFM1hZWHN0M1hZaTEwcFNrV0ZVYUtxWWhtdjRZ?=
 =?utf-8?B?QlIrSzJ6d0NIQ3gwNkNNbEwrdDJXMlNGNkhLTldnY3FLWWI3VkZmWkt3RW9i?=
 =?utf-8?B?Y1VRMnpFYUtWenJtZGdKR2d2dWxKZnM5Z3JIeThwa2d6bVgwOXorZ0hCZkpp?=
 =?utf-8?B?QXJrbEppcnJjMWdGUnVBSkJVYnpvaWs3aXFPNFU2TDJ4WDNuYjFpSE50YjIx?=
 =?utf-8?B?N3RtNjNIYjJsVlVBWHJZQkdsRmZ0QTF1T2NxNzZmd1VaQTBDZE1XeW83dEhM?=
 =?utf-8?B?ZmE4UERST2lDUEZ1OFlQYm1Tajg1Zmo4cmZzZGI2NlB5RTQwSW00ZU43N3NT?=
 =?utf-8?B?azV6SG9HUUdjb0I5YS80V3ZFUUtLNUF0S2UzcHdxWUx2RHNKVlM5eFFiTWlt?=
 =?utf-8?B?UlBzQktBRmxNcEwxSFdrNWkwdGNqUTBQc3NiODM3cGY2VVU3TkhwQzYrQUJn?=
 =?utf-8?B?eDZRNjlFRnRsUlM5ODNuQWFnWXlSWnJkcG5XZDlCT252Lzg0NzhuMWVHR0sw?=
 =?utf-8?B?VzN6aGQwU3UwQi9PUytSMzBGT2hFR1FPenVhNUJ2UVZLMHk3KyszZG1Fd2Ev?=
 =?utf-8?B?SGVmbldncWYzMUtGdDM4Q3o1bGJ2dk44WFhEQXBMUjFoYkcrWEx0UlVzeTlp?=
 =?utf-8?B?VjFWUWx3UjZyTVU4aEJ1TC9xOU5vVnBMbFB6S0tjcjdoVTJuTVVaQ0pqcGxV?=
 =?utf-8?B?REZZREcvdi9UNms4RDVXS2NnNTV6cGNNRGdRUUdsMkdUZ1RzZ01EcEt0SG9Z?=
 =?utf-8?B?bXlBMk5ERGx2TW5vRXdjaUdKVDZOWFRvUnNHdUsyOUZKWmJzT0tUSnp6Sk5Z?=
 =?utf-8?B?OGc1VHBrekgvWmQ0UFgxRXZOUFV2SjhzSkd6emlxd1BEWVVPRkZZV0x3V1gz?=
 =?utf-8?B?Q0ZsMDNtcCs2TXBsc1ZENGlVQnp2dlFzOUxiaXZOT2pXQTRnMyt2MU5VSmFG?=
 =?utf-8?B?anRwbkszOFQvZDE2S3dQYWx0ZDNsb1d1NUZyRWZXTDBoRW1tdUpFZW5PR2xQ?=
 =?utf-8?B?by9QbUdqcjZVRVVwempPY21hSmpRNXVvdzdXUGpUR296NktCK1c0bTZwcGFh?=
 =?utf-8?B?QUNjTVpnZWhQOGpMRDAyb3dCYUx5VUpMditoekxzcENnSUJwWW9xQXV0SXBI?=
 =?utf-8?B?ajBuTFBwWkpBN3pYa1g2Mk82NTFnMExuNk9uUjNEUldCeVFWUlRueUd5Rk5M?=
 =?utf-8?B?eXRNcm54RWt5UWpLSzk1c3FYUkJUY0FhckRwU3B5UWNhTnUvWGFvNFJiR3lh?=
 =?utf-8?B?dk1lYmhDc3N1bTJpN1hPanpVL2E4L1pCYjFSbVhxYWpMdzh4MzF1eFhzMW1q?=
 =?utf-8?B?QUE1b0EySFJ4dlgrRW9vRnJseUZIcFFTMlZNc1ErT3VWM3FLQkY4cE1sUHJz?=
 =?utf-8?B?V3UzZmFHcTRKamhUNTViV3gzSVZJWnlQUHZPYzVpU1N4MmF4eURVRlFaYTVC?=
 =?utf-8?B?V3p0YVd0a3gyRkNhdTF3S3JaUHF6WlJOZWR1SFJIemVBeVhYcnNkMEJaVU1v?=
 =?utf-8?B?NHE5VUFicGRqMTRkUjFaK2pjRnE4M0pDeG9VaHpXSVU0dk1STlkxVEYvVFBi?=
 =?utf-8?B?b2tEeUNxNU5DbndsSUdFSXR0NUhLNm1zL29ZL2VxOG1UWDNERGR5TlNwSjFQ?=
 =?utf-8?B?OEZQcjk2bW5xNmZ4OFVIaGV1VlhXaElOWThzdWFxL2ljYVFLQmVleW1BWlFW?=
 =?utf-8?B?THdqeUJkK1dtdGV1QmhBWUlmTEcrWEdsSXVnVUhVNmRoN0NCVHdOQUFsUGE4?=
 =?utf-8?B?dDIyejNEK1dieW1adzR5WlJDY1c0UlNsM3Q3SnpTV2Z6MTEwNkI2Z3pSMmZt?=
 =?utf-8?B?K1E9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 91b3dd2c-2903-45cf-e8bc-08da85267eae
X-MS-Exchange-CrossTenant-AuthSource: CY4PR11MB1862.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Aug 2022 16:42:40.9333
 (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: Ksf+0qjiiITNhw3fzDEGTWoIzwr5kzfMM3DZ7JtF+CTtJ8LE0Q1j5R8HhkjFPTLLNyAjukrRKpWO6GRHliwAExHInJIjkpbwTGmDLXtQfYQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BN6PR11MB1377
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 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
        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 James,

On 6/22/2022 9:46 AM, James Morse wrote:

...

> diff --git a/arch/x86/kernel/cpu/resctrl/rdtgroup.c b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> index b32ceff8325a..49d9e2c3c743 100644
> --- a/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> +++ b/arch/x86/kernel/cpu/resctrl/rdtgroup.c
> @@ -1890,17 +1890,26 @@ void rdt_domain_reconfigure_cdp(struct rdt_resource *r)
>  }
>  
>  /*
> - * Enable or disable the MBA software controller
> - * which helps user specify bandwidth in MBps.
>   * MBA software controller is supported only if
>   * MBM is supported and MBA is in linear scale.
>   */
> +static bool supports_mba_mbps(void)
> +{
> +	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl;
> +
> +	return (is_mbm_enabled() &&
> +		r->alloc_capable && is_mba_linear());
> +}
> +
> +/*
> + * Enable or disable the MBA software controller
> + * which helps user specify bandwidth in MBps.
> + */
>  static int set_mba_sc(bool mba_sc)
>  {
>  	struct rdt_resource *r = &rdt_resources_all[RDT_RESOURCE_MBA].r_resctrl;
>  
> -	if (!is_mbm_enabled() || !is_mba_linear() ||
> -	    mba_sc == is_mba_sc(r))
> +	if (!supports_mba_mbps() || mba_sc == is_mba_sc(r))
>  		return -EINVAL;
>  
>  	r->membw.mba_sc = mba_sc;


It seems that this change is propagating an issue that exists
in the current implementation (not introduced by this work).

The supports_mba_mbps() test uses is_mbm_enabled() which is:
	(is_mbm_total_enabled() || is_mbm_local_enabled())

That is not accurate since the software controller is only called
when MBM local is enabled:
	mbm_update()
	{
		...
		if (is_mbm_local_enabled()) {
			...
			if (is_mba_sc(NULL))
				mbm_bw_count(rmid, &rr);
		}
	}
			

Could the "is_mbm_enabled()" in the new supports_mba_mbps() introduced
here be changed to use "is_mbm_local_enabled()" instead? It does not seem
an issue for existing hardware (that supports total and local) but
the kernel parameters do enable users to boot with "rdt=!mbmlocal" to
trigger problematic scenarios.

Reinette