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: <7f1c23cd-486d-9bbd-2bcc-c2db0fa1e5c2@intel.com>
Date:   Tue, 17 May 2022 09:18:53 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.9.0
Subject: Re: [PATCH v4 07/21] x86/resctrl: Create mba_sc configuration in the
 rdt_domain
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: <20220412124419.30689-1-james.morse@arm.com>
 <20220412124419.30689-8-james.morse@arm.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20220412124419.30689-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?THZMcGZ1b3cxMEl1c3V1M2NvVVVtcFpFMm81NkFmaWtxM21yK1k5YWFiSExx?=
 =?utf-8?B?Nlp3ZVpjc3VZMVJTbDdXaXBXZExtbmJUWWUySXNweWU5YkhrS20wMklMR2kv?=
 =?utf-8?B?OUp5cnJEdWZtMWNWTGd1NW9HSlVnZzAzOS92Nkxtdng2eHJpVVNWM3J5MHUy?=
 =?utf-8?B?bGE5MmNqK053UDNvYnFsbVVOUDI5Z3N5YXgxSVFIOTlkTHJ1eWl5MWxvSUZC?=
 =?utf-8?B?c3NoU2Z6MFJGSlZmbjJRakk0L08yVEpMU1JYS2xXWUtTRVNKc0hiY29nRVVK?=
 =?utf-8?B?MjdFTUZaYlQxdGd1d3hHcnRtRVdacXFRUjM4UkU0QThLZWN3Y3JISmJFMGhz?=
 =?utf-8?B?N0RnNGVzdUtYaFBRL0d2VHo3Z2FxcFh6UlpqNFFXSGZ3WnRoWHZBNXhmakFO?=
 =?utf-8?B?amE3WmV4NlNsdVJaQUNiT0RuN2t3VWFhRTN1bVNCUUgrd1VoNEFENWloRjIy?=
 =?utf-8?B?M1BPSDV5U1NIcVo4cVVOUjFHZ2w1Y2lpQU5xbWZlZXB6eVYwd0VUUXBoaUcz?=
 =?utf-8?B?UGpYdHVxemgwSURZQlF4RDhCSDhnRGFVWjZNS3o5bzlUYUxld2d3dmV0WnhF?=
 =?utf-8?B?MEQvbmhuU1p0Y0oybFg2bGlTb3ByNlAyR09iM1BrS1hBM1NJZnhVLzIwSkhm?=
 =?utf-8?B?WlBkUXQwQ3FiRk5qaW1XQll0eFZVY21oSXBpdWtzek9leUQ0Q1d1RHNPYlpS?=
 =?utf-8?B?ZnphS25WUWFFeUhYV3FpLzkxM0ZzZGhQZWJLSTRkVEx5VWVtai9mVDFCOHV0?=
 =?utf-8?B?L0hOclY3YWd5VkNGN3FCdzJBWHNJbkY5aFliKzlGUC90YkpQKzhnYUoxWUJw?=
 =?utf-8?B?bHZUVGwwalNwcTVXbDZOZlZuYUw3YUU0ZnoxcDNOd09ISkpSTWRPOTBmZFBa?=
 =?utf-8?B?YmZBYW9yWlpRZitIZlljdUpLOVUvdkp6a293WWljclFrNllEYjBBWHdrc0FT?=
 =?utf-8?B?c3k3bG0yM2lZcUs1VXdWdkhURURzb1kzTlpUUVdtQXozSTdaakl5bDVWU09p?=
 =?utf-8?B?WitWWkZHeDNnblhRYXRwU0tPYUxHeHA0NUFMdFNRZExIOXdUbmw4SWloTWdl?=
 =?utf-8?B?Q0J5eUVNc0ZET3RxQjF3MWRTRS94Sk5ZVW5XMlpwQTlMdERMR3l4MUVSVldv?=
 =?utf-8?B?dlJlK1N1ZWVSSkw5OGx5SHdZN0xmUFJXQzB5YTJCRDJrRENsTU5yNnV2SjRZ?=
 =?utf-8?B?a201VGNXeTVuMDJRaXV3cTZwVnkraEhLZHg5UERkaWZSUjZpcHRFOGJqQ1R1?=
 =?utf-8?B?bHJ4cWtpT0dZcVNiUmNFV1c2OTBzbkhxU3ZqajFna2JTVlhlZldTQTRuc3Jq?=
 =?utf-8?B?N1NzNG81OHA1YWNPWjZZR0czcGpIZENGZ2FEY2tUcy9QaXE0VXFvRERTN2s4?=
 =?utf-8?B?a1VqWXVBakkxVGtFKzBvQmpnME5xNDdTTmh4ODU3TUZxTDIrZzlKT3JsMTE5?=
 =?utf-8?B?Z0RRUXlheHovODlUQWd0VTdpUGxEZnBTWnM1ZFZkVmJPN2Uxd2t3R0Vrc1Iv?=
 =?utf-8?B?aWhZdWJwNUNHR3luZmhveVBoamJXRmUzTGYyZHN5OHo4Vkhsd000bkJqemRa?=
 =?utf-8?B?MlhvS1cvT3RCeUg0ZDhUWWFZZ1FZSG5vN0FEdzBLcEx6VkxZK2lwb2JLOUFp?=
 =?utf-8?B?VVUvMVdrNjdVZnloZ1NSZFZrQnBTQXdSMmJyNjNoZ084TlkxNTNXaTM4bHBt?=
 =?utf-8?B?Yi9MdXQ5UjE3U3RWWkJLak1jK2dzSlQvZ0l2Y3RuWWR5UWlSRm1QdnFha2RT?=
 =?utf-8?B?VmViU01kVTJBVmllRFZPa0lvNjZWejNqTGIxa2NRSklWSjNwR3R6Q3g2aUo0?=
 =?utf-8?B?c1k3WWp4b2F1Zi90TFhvaDFKZ3dETVFoNHdsRkFGNG4xZy9iTUJuVUJOUk4y?=
 =?utf-8?B?TWpaaG95Z2c1dlhYOHJQSU9jYm5IZW9lUXR1K2c5WWRYSmtFTWlnMVNWcm5J?=
 =?utf-8?B?YkRQT1FjOXlNM21wdzZTUDVVM0swRi8wTTBKUVByZGVtalZBdHZmeEVPQnZo?=
 =?utf-8?B?MWlpdko4UW1VS3ZmTW5ZTVJ5QTF5cGE5UDdyNmRhc3FsT3kwZGVmemV2L1Q0?=
 =?utf-8?B?VE1pbjg4L2V3SVZ0YmxEK1ZOMU1OMllNZnJjQ2hWMDN2YXNsNDNUcHh3azhr?=
 =?utf-8?B?ZVhzb3VBMmhSTCtkZHlkbzRYb0NzQSt3bHdQQXJGeHE2bEVRekMzSFI1bEZD?=
 =?utf-8?B?YmdSYUJ3dS91TUZJR0Z1WnIzYnRQbEhEczlmOHBaYlVrTzdJci9LSUVNWkxk?=
 =?utf-8?B?SGE1eFpQa3I3OWUyU0hJMVNhdnV2Ulk1dmpJRGZWS0xidkRtT2l3UHVVUyt3?=
 =?utf-8?B?UWV1MXYwV0JqMHM0RE9pWjdNYmVOZ2dUamxXT2Q4Vlh0a0JhQ2xSeXJzb01a?=
 =?utf-8?Q?OTmfXlBoADV+rOKE=3D?=
X-MS-Exchange-CrossTenant-Network-Message-Id: f0d24228-43be-4f1f-45db-08da3820f204
X-MS-Exchange-CrossTenant-AuthSource: BN0PR11MB5744.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 May 2022 16:18:57.9492
 (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: 9/V0BjrxbmbL+/l7uBa3m9aMIiffr8Hs5kXQIEN9nx3jDYKizukgRPjTNzcEfECluyBte3jECYfc8DC65sa5b1TObuNnDE6TVpzg50IKNGk=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR11MB4787
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-6.7 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 4/12/2022 5:44 AM, James Morse wrote:

...

> @@ -3263,6 +3295,7 @@ void resctrl_offline_domain(struct rdt_resource *r, struct rdt_domain *d)
>  		cancel_delayed_work(&d->cqm_limbo);
>  	}
>  
> +	mba_sc_domain_destroy(r, d);
>  	domain_destroy_mon_state(d);
>  }

It is not clear to me how rdt_domain->mbps_val will be released via the above call.

After patch 3/21 and the hunk below resctrl_online_domain() would look like:

resctrl_online_domain() {
	
	int err;

	lockdep_assert_held(&rdtgroup_mutex);

	if (is_mbm_enabled() && r->rid == RDT_RESOURCE_MBA) {
		err = mba_sc_domain_allocate(r, d);
		if (err)
			return err;
	}
	
	if (!r->mon_capable)
		return 0;

	...
}
	
If I understand the above correctly, if MBM is enabled then all domains
of resource RDT_RESOURCE_MBA will have rdt_domain->mbps_val allocated via
resctrl_online_domain().

RDT_RESOURCE_MBA is not mon_capable, so at the time its domains go
offline, the freeing of rdt_domain->mbps_val will be skipped because	
after patch 5/21 resctrl_offline_domain() would look like below so
I do not see how the hunk added above will ever end up cleaning up
allocated memory:

resctrl_offline_domain() {

	lockdep_assert_held(&rdtgroup_mutex);

	if (!r->mon_capable) /* RDT_RESOURCE_MBA is not mon_capable */
		return 0;

	...

	
	mba_sc_domain_destroy(r, d); /* Not reached for rdt_domains of RDT_RESOURCE_MBA */
	domain_destroy_mon_state(d);
}

>  
> @@ -3302,12 +3335,20 @@ int resctrl_online_domain(struct rdt_resource *r, struct rdt_domain *d)
>  
>  	lockdep_assert_held(&rdtgroup_mutex);
>  
> +	if (is_mbm_enabled() && r->rid == RDT_RESOURCE_MBA) {

This introduces only half of the checks that are later replaced in
patch 10 "x86/resctrl: Abstract and use supports_mba_mbps()". Could the
full check be used here for that patch to be cleaner or perhaps patch 10
could be moved to be before this patch?

> +		err = mba_sc_domain_allocate(r, d);
> +		if (err)
> +			return err;
> +	}
> +
>  	if (!r->mon_capable)
>  		return 0;
>  
>  	err = domain_setup_mon_state(r, d);
> -	if (err)
> +	if (err) {
> +		mba_sc_domain_destroy(r, d);
>  		return err;
> +	}

Cleaning up after the error is reasonable but this allocation would only
ever happen if the resource is RDT_RESOURCE_MBA and it is not mon_capable.
Something would thus have gone really wrong if this cleanup is necessary.
Considering that only mon_capable resources are initialized at this point,
why not just exit right after calling mba_sc_domain_allocate()?


>  
>  	if (is_mbm_enabled()) {
>  		INIT_DELAYED_WORK(&d->mbm_over, mbm_handle_overflow);
> diff --git a/include/linux/resctrl.h b/include/linux/resctrl.h
> index 5d283bdd6162..46ab9fb5562e 100644
> --- a/include/linux/resctrl.h
> +++ b/include/linux/resctrl.h
> @@ -15,6 +15,9 @@ int proc_resctrl_show(struct seq_file *m,
>  
>  #endif
>  
> +/* max value for struct rdt_domain's mbps_val */
> +#define MBA_MAX_MBPS   U32_MAX
> +
>  /**
>   * enum resctrl_conf_type - The type of configuration.
>   * @CDP_NONE:	No prioritisation, both code and data are controlled or monitored.
> @@ -53,6 +56,9 @@ struct resctrl_staged_config {
>   * @cqm_work_cpu:	worker CPU for CQM h/w counters
>   * @plr:		pseudo-locked region (if any) associated with domain
>   * @staged_config:	parsed configuration to be applied
> + * @mbps_val:		When mba_sc is enabled, this holds the array of user
> + * 			specified control values for mba_sc in MBps, indexed
> + *			by closid
>   */
>  struct rdt_domain {
>  	struct list_head		list;
> @@ -67,6 +73,7 @@ struct rdt_domain {
>  	int				cqm_work_cpu;
>  	struct pseudo_lock_region	*plr;
>  	struct resctrl_staged_config	staged_config[CDP_NUM_TYPES];
> +	u32				*mbps_val;
>  };
>  
>  /**

Reinette