Received-SPF: pass (google.com: domain of linux-kernel+bounces-102612-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <c8822e06-a3df-4f05-8625-b2bb011887b7@intel.com>
Date: Wed, 13 Mar 2024 15:47:26 -0700
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v5 2/2] x86/resctrl: Add tracepoint for llc_occupancy
 tracking
Content-Language: en-US
To: Haifeng Xu <haifeng.xu@shopee.com>, <james.morse@arm.com>
CC: <fenghua.yu@intel.com>, <babu.moger@amd.com>, <tglx@linutronix.de>,
	<mingo@redhat.com>, <bp@alien8.de>, <dave.hansen@linux.intel.com>,
	<hpa@zytor.com>, <peternewman@google.com>, <x86@kernel.org>,
	<linux-kernel@vger.kernel.org>, <corbet@lwn.net>, <linux-doc@vger.kernel.org>
References: <20240308074132.409107-1-haifeng.xu@shopee.com>
 <20240308074132.409107-3-haifeng.xu@shopee.com>
From: Reinette Chatre <reinette.chatre@intel.com>
In-Reply-To: <20240308074132.409107-3-haifeng.xu@shopee.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: 7bit
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?S1lIZWFvRFdab2dNUXZCbVFyK3pNZkxDRlBrM25kZk1IRk5pdDBtR29FNDBG?=
 =?utf-8?B?RnMwK0p4MFpVMXNzOEhEeisyWEZuaHd0WUk0UTBFSzhlenBZVE5PQmgyRFlM?=
 =?utf-8?B?ZHBUNEFWWDQrK2VPKzlCUjJBWDMyV3N2RjR5Vk5NWlF4bk5ZTVdadGY3T29p?=
 =?utf-8?B?R1ZJSWU3YUN1QmhpY1N5Tm44ckpPTWh5OEdmYVAyWWRid0E3NG16UnplSjNu?=
 =?utf-8?B?bHN6TmM3TVQramFxK0FoYkRLVktqRytrZG9FVzJEMThiOXA0VmZwNUZyOFZo?=
 =?utf-8?B?MW8xWmdZcjEyUTdja1hpbjQwMzErOXZ3dkdwN001V29TY25rN0syNVpPMWtZ?=
 =?utf-8?B?cU5xSEh3dFhSODNub2I4eTNDY0hUUmF1NC9NUGNLNENZcFBQMWN2WHdZV1pr?=
 =?utf-8?B?WEduL2hNS3hlbHlPWXJYbTJhcXV5V1l3ZnRBUjN3OERadE9UNTlYSXNCdVMw?=
 =?utf-8?B?ZmJtdkU3SVNoVGdEQ3B0eEJOM0ZYTkt0UjdpdCs1VzI0NkFyeXJVMWtWc0Zx?=
 =?utf-8?B?RHZ1SXdYQ0FJeDJ5d1l0bk1vQVlMZngvZW5VTkt2ZDV1SmJaMHp5ZnY1T1g2?=
 =?utf-8?B?eTN4WUNGeUduaklHRFBCem9BdndUWWwreTk4RzNvQXRCTGJHaUM5S1V6OEdt?=
 =?utf-8?B?SFYzWFZIWm93ajhyTXI0MUN1ZjdvUkxxRzdnNjlIeld1ZEkyTU5GdFJTSWli?=
 =?utf-8?B?QmlwTmxqTE0xV3Y3K0JEdWJFVFpSNDF0UTYrcmYrZkZQWWNPMGRDWThaVVJQ?=
 =?utf-8?B?TER1V3dadkptVWplU0QvY0YwNG1ndUhWMXArN21hTnJhWHNnSHA5M2l3Slhw?=
 =?utf-8?B?Ti9TdFZZek9ieFFrbWUxTXc1YVc4VVpzQWFTd0RQNDk0Q2RRZ2JNYnd2YWZI?=
 =?utf-8?B?L3RqOVZkZmZ0eVZpV2REQWdqZ2FLQXVHNlZESkkwUm8vYUpLSHA5Mlp5SHk2?=
 =?utf-8?B?SHZzU1JsMlBodUU1cGwvamhMSFAxaEtKWlhGQ3hRRHIwbnU3aFF1MS9scjhB?=
 =?utf-8?B?c1diM1VTU242aWw1OVdFQXh4QUt4ZGMxamJKbkxjT3RtZlBZRU9BRjIzU0kv?=
 =?utf-8?B?SGNaWDE3WXZwZjRPT2tzdktEazJSNjkyOG5TQTlPVXlBWG5vU0tOTktyMDls?=
 =?utf-8?B?UHRFenpmNkgrR2kvbHlXTE1kVHd6M3psM1EyNHZabU1GaDlnZ0V6ZG9GSUo5?=
 =?utf-8?B?ZmFkbDg5Y0hLMDF1bWpLbWxWYnM2c1V5T3I2ay92dm9sc3d4N2NONk5oZEti?=
 =?utf-8?B?Y3VMNEdMTitnTmNFYlpIYUw5VTZwZG0wR0UzQ3FGY0V6SnRCRlhVQy9GTXMv?=
 =?utf-8?B?YnRHdmhJeTJYc09qQmJBM0lyOFRISXRGekU0cWEvMUZDaGdIKzVrM0loNUlN?=
 =?utf-8?B?VkdaWVg0eERIQzJUSExycmx3NXZlK0hqSnNLVi9MQ2dESW56SUMyQ2xoaVRU?=
 =?utf-8?B?SFJLSzljVDdWODJrYzJuT2NWR3ErYmFSak4vNjVlRUVnUlpPdEJIMkEyN0tN?=
 =?utf-8?B?QUlrTGNnSDd3Tkx5SXhPV0o0VS83NzR6OG5zYXpXaktIQUZEbjRXR2hSNURE?=
 =?utf-8?B?Mzg4UUk0TXl0VjFlOTFaZU9IVnQySmVBeTNETlBBcE45THluTWVlTldtT3BQ?=
 =?utf-8?B?dEF0eHQ0QlJ2dkhYWncrL2xHQ3kzQ1VXdHVCN0RDdGFicUUyY0FQV2lSMENG?=
 =?utf-8?B?aEl0K1VmZjBSS1BSU2VFZnpJbUFjSnFjYjRjUXpaeXFBc29lUUNCSUd6c3lH?=
 =?utf-8?B?RGk5aFBkamc3S0dUaFRILzlaTHdBbjZJVTdaV2NWVC9kdjE3eDJVMVFCbzFC?=
 =?utf-8?B?YWQrclk4UFJxS1VHOFJSbENITkZiY1c1Ukg0ZXBVc0wzdDV2UzZrZUppWHBu?=
 =?utf-8?B?R0kvQzNlRFltQUFET3p0V2hwMm1xOEE2aW1pc1lXbnBxYVU5S01CRGs1eW15?=
 =?utf-8?B?SzQ1eUNmajZNK0wyQ2p6OU5LaXhOS1JTdzVFbUw3VzV2QnYvWGRZRUpwTWlM?=
 =?utf-8?B?VS9uQlc2S0l1dC9PMWFpRlovVURkK3JTUEwzdFdib3RScy8rdHdMZzV0KzUx?=
 =?utf-8?B?QnBtSDRyaEFMN0Z3MXBDZlhFTE4wczlrdzZUYWpIa2srVnZTZTJBaWw0Qm5R?=
 =?utf-8?B?TzRPZytWZWdTWXQ2RHpmNFJWUGEzVERZa3MyUFd3R3p2eVZuQTZORnBDZHpI?=
 =?utf-8?B?Nmc9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: e42265e3-a84a-4489-c49f-08dc43af8eb3
X-MS-Exchange-CrossTenant-AuthSource: SJ2PR11MB7573.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 13 Mar 2024 22:47:27.3553
 (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: HhbkxFypJG7QjnxNwQL9oj5LUnjuSNBhn61QURPadLFdUTst2yDTdA3M4bWePz7cvQVC3q/GzXdew6rjIoJabSnxdDLP265/5/14990xuBQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM3PR11MB8683
X-OriginatorOrg: intel.com

Hi Haifeng,

On 3/7/2024 11:41 PM, Haifeng Xu wrote:
> In our production environment, after removing monitor groups, those unused
> RMIDs get stuck in the limbo list forever because their llc_occupancy are
> always larger than the threshold. But the unused RMIDs can be successfully
> freed by turning up the threshold.
> 
> In order to know how much the threshold should be, perf can be used to
> acquire the llc_occupancy of RMIDs in each rdt domain.
> 
> Instead of using perf tool to track llc_occupancy and filter the log
> manually, it is more convenient for users to use tracepoint to do this
> work. So add a new tracepoint that shows the llc_occupancy of busy RMIDs
> when scanning the limbo list.
> 
> Signed-off-by: Haifeng Xu <haifeng.xu@shopee.com>
> Suggested-by: Reinette Chatre <reinette.chatre@intel.com>
> Suggested-by: James Morse <james.morse@arm.com>
> Reviewed-by: James Morse <james.morse@arm.com>
> ---
>  Documentation/arch/x86/resctrl.rst    |  8 ++++++++
>  arch/x86/kernel/cpu/resctrl/monitor.c |  9 +++++++++
>  arch/x86/kernel/cpu/resctrl/trace.h   | 16 ++++++++++++++++
>  3 files changed, 33 insertions(+)
> 
> diff --git a/Documentation/arch/x86/resctrl.rst b/Documentation/arch/x86/resctrl.rst
> index a6279df64a9d..dd3507dc765c 100644
> --- a/Documentation/arch/x86/resctrl.rst
> +++ b/Documentation/arch/x86/resctrl.rst
> @@ -478,6 +478,14 @@ if non-contiguous 1s value is supported. On a system with a 20-bit mask
>  each bit represents 5% of the capacity of the cache. You could partition
>  the cache into four equal parts with masks: 0x1f, 0x3e0, 0x7c00, 0xf8000.
>  
> +Tracepoint - mon_llc_occupancy_limbo
> +------------------------------------

I think that the below paragraph would fit nicely as a new paragraph in the
existing "max_threshold_occupancy - generic concepts" section. To support that
just one change to text below ...

> +This tracepoint gives you the precise occupancy values for a subset of RMID

The mon_llc_occupancy_limbo tracepoint gives the precise occupancy in bytes
for a subset of RMID ...

> +that are not immediately available for allocation. This can't be relied on
> +to produce output every second, it may be necessary to attempt to create an
> +empty monitor group to force an update. Output may only be produced if creation
> +of a control or monitor group fails.
> +
>  Memory bandwidth Allocation and monitoring
>  ==========================================
>  
> diff --git a/arch/x86/kernel/cpu/resctrl/monitor.c b/arch/x86/kernel/cpu/resctrl/monitor.c
> index c34a35ec0f03..60b6a29a9e29 100644
> --- a/arch/x86/kernel/cpu/resctrl/monitor.c
> +++ b/arch/x86/kernel/cpu/resctrl/monitor.c
> @@ -24,6 +24,7 @@
>  #include <asm/resctrl.h>
>  
>  #include "internal.h"
> +#include "trace.h"
>  
>  /**
>   * struct rmid_entry - dirty tracking for all RMID.
> @@ -354,6 +355,14 @@ void __check_limbo(struct rdt_domain *d, bool force_free)
>  			rmid_dirty = true;
>  		} else {
>  			rmid_dirty = (val >= resctrl_rmid_realloc_threshold);
> +
> +			/* x86's CLOSID and RMID are independent numbers, so the entry's
> +			 * closid is a invalid CLOSID. But on arm64, the RMID value isn't
> +			 * a unique number for each CLOSID. It's necessary to track both
> +			 * CLOSID and RMID because there may be dependencies between each
> +			 * other on some architectures.
> +			 */

Please watch for proper formatting of multi-line comment and consistent capitalization.
I also think comment can be more accurate, for example:

	/*
	 * x86's CLOSID and RMID are independent numbers, so the entry's
 	 * CLOSID is an empty CLOSID (X86_RESCTRL_EMPTY_CLOSID). On Arm the
	 * RMID (PMG) extends the CLOSID (PARTID) space with bits that aren't used
	 * to select the configuration. It is thus necessary to track both
	 * CLOSID and RMID because there may be dependencies between them
	 * on some architectures.
	 */

> +			trace_mon_llc_occupancy_limbo(entry->closid, entry->rmid, d->id, val);
>  		}
>  
>  		if (force_free || !rmid_dirty) {
> diff --git a/arch/x86/kernel/cpu/resctrl/trace.h b/arch/x86/kernel/cpu/resctrl/trace.h
> index ed5c66b8ab0b..b310b4985b94 100644
> --- a/arch/x86/kernel/cpu/resctrl/trace.h
> +++ b/arch/x86/kernel/cpu/resctrl/trace.h
> @@ -35,6 +35,22 @@ TRACE_EVENT(pseudo_lock_l3,
>  	    TP_printk("hits=%llu miss=%llu",
>  		      __entry->l3_hits, __entry->l3_miss));
>  
> +TRACE_EVENT(mon_llc_occupancy_limbo,
> +	    TP_PROTO(u32 ctrl_hw_id, u32 mon_hw_id, int domain_id, u64 llc_occupancy_bytes),
> +	    TP_ARGS(ctrl_hw_id, mon_hw_id, domain_id, llc_occupancy_bytes),
> +	    TP_STRUCT__entry(__field(u32, ctrl_hw_id)
> +			     __field(u32, mon_hw_id)
> +			     __field(int, domain_id)
> +			     __field(u64, llc_occupancy_bytes)),
> +	    TP_fast_assign(__entry->ctrl_hw_id = ctrl_hw_id;
> +			   __entry->mon_hw_id = mon_hw_id;
> +			   __entry->domain_id = domain_id;
> +			   __entry->llc_occupancy_bytes = llc_occupancy_bytes;),
> +	    TP_printk("ctrl_hw_id=%u mon_hw_id=%u domain_d=%d llc_occupancy_bytes=%llu",

domain_d -> domain_id

> +		      __entry->ctrl_hw_id, __entry->mon_hw_id, __entry->domain_id,
> +		      __entry->llc_occupancy_bytes)
> +	   );
> +
>  #endif /* _TRACE_RESCTRL_H */
>  
>  #undef TRACE_INCLUDE_PATH


Reinette