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: <03cd7ac4-b58d-c7a8-7cb9-ebcc770d21f0@intel.com>
Date:   Wed, 9 Aug 2023 15:33:55 -0700
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.14.0
Subject: Re: [PATCH v5 06/24] x86/resctrl: Track the number of dirty RMID a
 CLOSID has
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>
References: <20230728164254.27562-1-james.morse@arm.com>
 <20230728164254.27562-7-james.morse@arm.com>
From:   Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20230728164254.27562-7-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?WjhSZEJva2V6ajF2NXAxMk8vVzY0MWpGY0c2ZVB6bkkrSVA0dWE4RUtmTktW?=
 =?utf-8?B?SGZpKzRrZGRVMmJUQmM5RmI0UjdaeEhvTmdNZ2lmWHhXcUlIN3NWOGlSMktB?=
 =?utf-8?B?Y0t6cTY0RFVKZWxYYjJhaGh2QnpYMGw1Qmk2L0lvRWdCbHRMUEUrU0xSR2Y0?=
 =?utf-8?B?SkNuUzYrQW43QkpWdUJVNEMwVlVwbTFzc1RCSWVSUEZwQkdrc0pkY2JwM3BU?=
 =?utf-8?B?dlFpaTZZQXRRREJnZEQvQm9xZlJMekdlUTV2T0xyTEN5a0tIYXBxVnVQU09Y?=
 =?utf-8?B?QkxuR2prbDRmQTlOQ0RUTjQzV1g3Ujh1Sk5lcXhUSTc1K0pSS1FiR3o3THY1?=
 =?utf-8?B?R09ESVVZaW5US1hjdDFrMjZMbVVyR2s3VTBoVW0rb1FtRTBLc29mY2hWZHdp?=
 =?utf-8?B?S21TMzFWZHB4SVNaU1ZoVXdIWU1RK1psbWtuQ1hUNE8yMmVPMjlCNHZhcnlJ?=
 =?utf-8?B?MmVBR3VpbHZrTUhqcldKVDZTR3NCK0xaN3VrTFREbGRzb1p2K0I4RnBmanRP?=
 =?utf-8?B?WDFCYkNxRjdrakU1Z1F5SmRWM0NOSzU0OVZFc1BFQUtzQjF2MUtpY2Zacllp?=
 =?utf-8?B?YkI5RUJ6U0tIYnovTlU1YmNlWm1lUzZHZzNwNmEybkdVUjBMWFRlTEJxNVNs?=
 =?utf-8?B?ZG12eGxzQ0RrSExVR1Q3UWgwdkxoYW85SlRCc1dONzRqVkZ5Y0NoZ3V0clRD?=
 =?utf-8?B?alMrclBCNVN3RklEWUFNS2ZhdFozNTVWZW9NRnJ2NTh4ajAzRFUzdzRqelhn?=
 =?utf-8?B?YWRjVkZoYzNIV2pWYVJoZy9ucVpyVFJCZGE0Mm43bnhlT0lFRGNGMTNRaWlu?=
 =?utf-8?B?MHVyd2tGY2lqMmNSdUI3UmdTUHF1RGZQNmlxbWE4NUxzWEtyblc4TVlBQmkw?=
 =?utf-8?B?cEtFME1rZ01rWXRXejc2UE9Qc0xJOCtIZmNOelQrSFp4clh1Z0g5NXFFdEpC?=
 =?utf-8?B?cFNlVWpHY2NOSERPY254WEgxdEYwcmEzLzd2eE1uOXBHekQxV1RRUzgrVnpH?=
 =?utf-8?B?SnA0YVZsYWI2Z29SR043ZFN4ZVpETWNpbFcza3ZTZ2labWwxT0t0RE5meERS?=
 =?utf-8?B?Sk5lbGZBVzlwZzhoUnpCOXF0V1FnZGJKRHk3ZlhGR1ptMlo5OElMWlJDQk5z?=
 =?utf-8?B?VjRUR05tUlQ4SGcvY0diNjM1ZWs3K01YVFd1bE9FdUdHR3cwb1UrRXV4L2p3?=
 =?utf-8?B?MWd0emlUeGxuWXo3ZENGM3FHRzY3NzBUdlZqWVFETm9YR3VDYmdKWENoK1p1?=
 =?utf-8?B?YjRMb0I5emphdWYyaVpBNDdpMHNmQ01DWWs0aTZubTBXRE1qS2JvRURCaXBM?=
 =?utf-8?B?YnJ5UDRla2RSbHBxY2hFZDhLQnFTd0J2V0JBaU5qejZQS2YwZVRaZ0ZOalJH?=
 =?utf-8?B?dWc3VGUzdk41dFNaQ3NXNisyWndDaGJkVW1NemhKVUhKcmhrZVlZeHJKampP?=
 =?utf-8?B?ZWI5RUpVd2J5MWFUbG9Vem5ZNkFhOWJUNm5wTm9xODkzL1hBandVOGhUVmZq?=
 =?utf-8?B?MC83aWVhcTNkbGk2dks2YXBUWU15RDBOUCtPZlhUbzZaSzAzZHZaRkZnVDly?=
 =?utf-8?B?TUgrOS8vd3VOdVZwbzZtaUxIV3JsTTdlQlgrVTFLeE5XK3hDc21UVnhxKytV?=
 =?utf-8?B?YjVScGdHcllVM254ZGNWU3YrTXYvUW9zTnhQa0NOUnNjY3cvVzRJY1Ezb3E1?=
 =?utf-8?B?RUYwMk9QUjcvQkJDOXd0VVJrbGZSNXVTQzNseG5qQkpoYUxVT0NiV3JGYWRD?=
 =?utf-8?B?T2FyeEoyVUtvaUNFMllsVFJrUGtFYUN6K0hSYVlUb1ZwWGFWdzhNN01kZ3Bm?=
 =?utf-8?B?S1hFejdEenQrZkVyWWpHbTVhb1F1UFRFODBLc29ra09QUkUvQ3h2TmFybjhW?=
 =?utf-8?B?UFUxZGxGVnEwSFh0WC8vYlp1SURQNXUzNEMrWmJGMDgrVk9OMkx3b1NmUW5F?=
 =?utf-8?B?UTZvUmRXeUxteWl3QURpOWsrUUplVUNrUnNGNk8zWWt4S2Fudy9qdGxnOVpC?=
 =?utf-8?B?VVhvRkhYb1JhRXZmVmZNOFc5Tm9vcFo0RUF4N1JOZ1dnSG8yNERTcFpqYlBE?=
 =?utf-8?B?YTNPbXZ5ZiswdVZuUS9QQnBDQXF6Uk4wcC8wRWJ2WG1aM0JoOHo3eUM3bVlr?=
 =?utf-8?B?Tkl5eWE4aTBXQ2F1WE52OFZpOHFMOU5QYURUYTJSZ3hJY1J0dTRJZ3hNcXRw?=
 =?utf-8?B?TUE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 88371f76-163e-4c32-8ab2-08db9928b8e0
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 09 Aug 2023 22:33:58.4411
 (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: HFV7zJdBAf+UO4J4pEYHBJYEP0BXj9BSfky18Pc2hgzy7GfDY1u1Cr93RhskRJKPpGwXJIzukXhmAIUZU0j0+0+jpNEZEeNf8r2eyZQ3oDI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA3PR11MB7413
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-2.9 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_BLOCKED,SPF_HELO_NONE,SPF_NONE,URIBL_BLOCKED
        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 7/28/2023 9:42 AM, James Morse wrote:
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index de91ca781d9f..44addc0126fc 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -43,6 +43,13 @@ struct rmid_entry {
>   */
>  static LIST_HEAD(rmid_free_lru);
>  
> +/**
> + * @closid_num_dirty_rmid    The number of dirty RMID each CLOSID has.
> + * Only allocated when CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID is defined.
> + * Indexed by CLOSID. Protected by rdtgroup_mutex.
> + */
> +static int *closid_num_dirty_rmid;
> +

Will the values ever be negative?

>  /**
>   * @rmid_limbo_count     count of currently unused but (potentially)
>   *     dirty RMIDs.
> @@ -285,6 +292,17 @@ int resctrl_arch_rmid_read(struct rdt_resource *r, struct rdt_domain *d,
>  	return 0;
>  }
>  
> +static void limbo_release_entry(struct rmid_entry *entry)
> +{
> +	lockdep_assert_held(&rdtgroup_mutex);
> +
> +	rmid_limbo_count--;
> +	list_add_tail(&entry->list, &rmid_free_lru);
> +
> +	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID))
> +		closid_num_dirty_rmid[entry->closid]--;
> +}
> +
>  /*
>   * Check the RMIDs that are marked as busy for this domain. If the
>   * reported LLC occupancy is below the threshold clear the busy bit and
> @@ -321,10 +339,8 @@ void __check_limbo(struct rdt_domain *d, bool force_free)
>  
>  		if (force_free || !rmid_dirty) {
>  			clear_bit(idx, d->rmid_busy_llc);
> -			if (!--entry->busy) {
> -				rmid_limbo_count--;
> -				list_add_tail(&entry->list, &rmid_free_lru);
> -			}
> +			if (!--entry->busy)
> +				limbo_release_entry(entry);
>  		}
>  		cur_idx = idx + 1;
>  	}
> @@ -391,6 +407,8 @@ static void add_rmid_to_limbo(struct rmid_entry *entry)
>  	u64 val = 0;
>  	u32 idx;
>  
> +	lockdep_assert_held(&rdtgroup_mutex);
> +
>  	idx = resctrl_arch_rmid_idx_encode(entry->closid, entry->rmid);
>  
>  	entry->busy = 0;
> @@ -416,9 +434,11 @@ static void add_rmid_to_limbo(struct rmid_entry *entry)
>  	}
>  	put_cpu();
>  
> -	if (entry->busy)
> +	if (entry->busy) {
>  		rmid_limbo_count++;
> -	else
> +		if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID))
> +			closid_num_dirty_rmid[entry->closid]++;
> +	} else
>  		list_add_tail(&entry->list, &rmid_free_lru);
>  }

This new addition breaks the coding style with the last statement
now also needing a brace.

>  
> @@ -782,13 +802,28 @@ void mbm_setup_overflow_handler(struct rdt_domain *dom, unsigned long delay_ms)
>  static int dom_data_init(struct rdt_resource *r)
>  {
>  	u32 idx_limit = resctrl_arch_system_num_rmid_idx();
> +	u32 num_closid = resctrl_arch_get_num_closid(r);
>  	struct rmid_entry *entry = NULL;
>  	u32 idx;
>  	int i;
>  
> +	if (IS_ENABLED(CONFIG_RESCTRL_RMID_DEPENDS_ON_CLOSID)) {
> +		int *tmp;
> +
> +		tmp = kcalloc(num_closid, sizeof(int), GFP_KERNEL);
> +		if (!tmp)
> +			return -ENOMEM;
> +
> +		mutex_lock(&rdtgroup_mutex);
> +		closid_num_dirty_rmid = tmp;
> +		mutex_unlock(&rdtgroup_mutex);
> +	}
> +

It does no harm but I cannot see why the mutex is needed here. 

>  	rmid_ptrs = kcalloc(idx_limit, sizeof(struct rmid_entry), GFP_KERNEL);
> -	if (!rmid_ptrs)
> +	if (!rmid_ptrs) {
> +		kfree(closid_num_dirty_rmid);
>  		return -ENOMEM;
> +	}
>  
>  	for (i = 0; i < idx_limit; i++) {
>  		entry = &rmid_ptrs[i];

How will this new memory be freed? Actually I cannot find where
rmid_ptrs is freed either .... is a "dom_data_free()" needed?

Reinette