Message-ID: <c41e65c6-23af-0c3c-ae0a-73234652f981@intel.com>
Date:   Mon, 6 Feb 2023 11:50:23 -0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.1
Subject: Re: [PATCH 5/5] platform/x86/intel/ifs: Trace support for array test
Content-Language: en-US
To:     Steven Rostedt <rostedt@goodmis.org>
CC:     <hdegoede@redhat.com>, <markgross@kernel.org>,
        <tglx@linutronix.de>, <mingo@redhat.com>, <bp@alien8.de>,
        <dave.hansen@linux.intel.com>, <x86@kernel.org>, <hpa@zytor.com>,
        <gregkh@linuxfoundation.org>, <ashok.raj@intel.com>,
        <tony.luck@intel.com>, <linux-kernel@vger.kernel.org>,
        <platform-driver-x86@vger.kernel.org>, <patches@lists.linux.dev>,
        <ravi.v.shankar@intel.com>, <thiago.macieira@intel.com>,
        <athenas.jimenez.gonzalez@intel.com>, <sohil.mehta@intel.com>
References: <20230131234302.3997223-1-jithu.joseph@intel.com>
 <20230131234302.3997223-6-jithu.joseph@intel.com>
 <20230206114053.1c46fddb@rorschach.local.home>
From:   "Joseph, Jithu" <jithu.joseph@intel.com>
In-Reply-To: <20230206114053.1c46fddb@rorschach.local.home>
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?YzJ2czVBVnJvQjljNk8zN0I1TFA2TGk4clI5VDQ0aU1hSE5uMytXM3d0d2l0?=
 =?utf-8?B?Yjg4N002WXQ4L0w1QWRjTWRUWFJnUXg0L3hlNkUybndOcmNka1FOVXc0NWtT?=
 =?utf-8?B?RjJOMkUreUlKTENzU3FEWkJaTXVWdytyRFp3VWxmT28xdjF6Nmp4T1FXTEFs?=
 =?utf-8?B?R0RLUTZzU2NLZTlMaUU0VWc2L1lJT3V3RXlROE85bHhPS2M1ZEVoZEkrbHk1?=
 =?utf-8?B?MGpOZGFQZlJBK3JPM0d4Y1Q1VFFIYlAvcnBVNW9ib3J0Q0JGUENRS3o2bndG?=
 =?utf-8?B?eUU4RkJ3ZGRUY0l3M2poTmZDSkQ1ekUvNnFPeHhoWkJBWU9iYk0wLzlZdGFY?=
 =?utf-8?B?NjVCSHl6bGgzSGM3T0l4SGVYUkl1VHNDaXg2bzEwZG5mMFdJbldmSUpOTTZo?=
 =?utf-8?B?NS9PTm9jRTVobE91Q0JzZzBvWnZYMGhaandidk8rQ3drTG9wR3ZJQ1R1NGZX?=
 =?utf-8?B?bU1LVkg5M0pSZjFYWUpobVB1RUtwdDdoSnNyOXJvbEIrM0tOdkNSaDM2ZlRQ?=
 =?utf-8?B?dm1lYTByRjVVQW5kYWJPV2h5blBhVlRPT2N6b241OTlzb0UrWnJVdG9wNDFz?=
 =?utf-8?B?cWIyRUpGWm0zQ0lpTjJWcEVCMi9uYmJ6K1dJdUlkdFBPNEhZK1pvVitLWWtS?=
 =?utf-8?B?VktrVE9Qa2d2Y1pqTWU4UE1sajAyRVpKR0pHT2hZR3QwNS8xekJtT1RIelBO?=
 =?utf-8?B?Mk1vVFJ0aFNndmhzKytuaWN5NUxvYzRJK21xYUhiTWVFVlg0WFZJSWJOMm83?=
 =?utf-8?B?TklDbUJ2WkhTMUo2OE92UFM2WWJqa3NtKzh6a21saUw1QnRBQm9FOFM2R3NH?=
 =?utf-8?B?dkxXUHBRUWtOQlhkTGhtWXhXcmlzK0R0TkVwYUppeUNhYldGdFVoN3V3L20x?=
 =?utf-8?B?NUZGdFZzYlNac09jZHdkOWk4MGF3Qmw0U0tsOGhFeE9ZaGlEeU16Z2pXWStE?=
 =?utf-8?B?Nnk0clBaTFY1Q1NFMGFMcU0wQnFlOEpLWVhxSHBVc1VQMXFqYVBMcUUyZ2xw?=
 =?utf-8?B?UWRtTVFFQjBXOGt1NEM5RE9TVVl1clRjdEpOQ2h5d09oWlNOT3R2SC9jL0x3?=
 =?utf-8?B?RmRNbVlpMEhVeFlteXFaYkFUekJQL2U4WmxOVXdKQVVNZ0ZzSFppRkVURTVa?=
 =?utf-8?B?Q3c4bGRGSzdHU2dDZFFMVjluV055Sjg2ZldYdzBmTmRQVkNpb1Vid0xIS05F?=
 =?utf-8?B?WDAvTytrcG91Q0paZlR3YUgwcDBrcVhjMnNFM1MzaVpQbzNFaHdyeXAyKzRr?=
 =?utf-8?B?KzN4dE16ZFprVlhVcWRNbHJVZUxPWmsyN2M4ZGxpRmV5TTFKa1JKcnJtTjVh?=
 =?utf-8?B?UjVsUkVtbTNnanB4TU1aeEdDS0p5OGtsNmtEakpTVWZIUUVyVU5pT0ttbCtT?=
 =?utf-8?B?RG5uT1hmWjFiMUlINWpJdVcwOTFlNTVickhlK0xWUXIzdHpobEVVRVJadHpt?=
 =?utf-8?B?Nnpsb2tpUmcrVU01Vno0aDIyeEZ6MW52dXR0OURtKytIYTUrMlVUL1JGUUZr?=
 =?utf-8?B?WGZrYUN3ZWt4QUw5MzRJczJNK2Q3K3pPUitYRVNIcHFkUGNiWU5LeGo5bTdX?=
 =?utf-8?B?NkMrVm9Qc2lKZ1FKWVdKOGNpTU5BcUNoZEFtdTExOThibTNVWEFQT08yN1Nl?=
 =?utf-8?B?S0dURjg2eGxMZVlXVE9MTDdaRDZjYXRCTzd3N1BkU0tmZGExdHhqaXZ5ZkYv?=
 =?utf-8?B?TzVQeFMrNVZMZUxjR0JrMjFrY2c2eXYwc0NJc1pjZlQvcWhMWkorT0RSRlRT?=
 =?utf-8?B?enhub3VOemxvTVdocGtCbHJtbGl3RGRYK2hqemVidnd0MjBNTHJwbEw0dG0w?=
 =?utf-8?B?RGJEK1ZHeDgzT2RIaExzaTUvN1RWc3hMMi9XMWd0WGplbHYweEtoWGdYV1o4?=
 =?utf-8?B?TEJCb1ZvY3YxNzh5dUhXQjE2K2lGWlZpaFFLeERaS0c4bURjV2lEOE1Mb3Z6?=
 =?utf-8?B?THY2UU1EKzgzYlc0UHhpbHhnVm05bVNER3dha2xPVTh0NVk3VVVuTW5uaXRj?=
 =?utf-8?B?ZjB1L2hQYy9BRk5OK1hrU3NEWWZoMUVoZkdyZjBsM3FKSkEvR3JCd09Tai9o?=
 =?utf-8?B?VWUrWmtNM1licmZ6aHBOMUk2RnJvSzBkb0JpMjhwOWtkZWZNb095NHBhTU1P?=
 =?utf-8?B?dDV1aUNsMSswdnNFRFdHaEYzeW81Mm12dzFlQXFLQmIvTzVqK0dWRUJqQTgz?=
 =?utf-8?B?aEE9PQ==?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 2204963a-2517-4e33-a010-08db087b6416
X-MS-Exchange-CrossTenant-AuthSource: BYAPR11MB3768.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 06 Feb 2023 19:50:25.8760
 (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: FAShBGXs/a/fmCchZ0L0+ktj1RODQn/Opk6jk3ih4rpusJ20KdEkYqg1pv2Y73F1jZAPMHHiGt6L7BoMBWXiIw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SJ0PR11MB5631
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Thanks for the review

On 2/6/2023 8:40 AM, Steven Rostedt wrote:
> On Tue, 31 Jan 2023 15:43:02 -0800
> Jithu Joseph <jithu.joseph@intel.com> wrote:
> 
>> diff --git a/include/trace/events/intel_ifs.h b/include/trace/events/intel_ifs.h
>> index d7353024016c..db43df4139a2 100644
>> --- a/include/trace/events/intel_ifs.h
>> +++ b/include/trace/events/intel_ifs.h
>> @@ -35,6 +35,33 @@ TRACE_EVENT(ifs_status,
>>  		__entry->status)
>>  );
>>  
>> +TRACE_EVENT(ifs_array,
>> +
>> +	TP_PROTO(int cpu, union ifs_array activate, union ifs_array status),
>> +
>> +	TP_ARGS(cpu, activate, status),
>> +
>> +	TP_STRUCT__entry(
>> +		__field(	u64,	status	)
>> +		__field(	int,	cpu	)
>> +		__field(	u32,	arrays	)
>> +		__field(	u16,	bank	)
>> +	),
>> +
>> +	TP_fast_assign(
>> +		__entry->cpu	= cpu;
>> +		__entry->arrays	= activate.array_bitmask;
>> +		__entry->bank	= activate.array_bank;
> 
> Regardless of the "bitfield" discussion on the other patches, this part
> is considered a fast path (although if where it is called, then it may
> not be). I would just have:
> 
> 		__field(	u64,	data	)
> 
> 		__entry->data = status.data;

Will modify it as given below (in-line with your suggestion)

	TP_STRUCT__entry(
		__field(	u64,	activate	)
		__field(	u64,	status	)
		__field(	int,	cpu	)
	),

	TP_fast_assign(
		__entry->activate	= activate.data;
		__entry->status	= status.data;
		__entry->cpu	= cpu;
	),

	TP_printk("cpu: %d, array_list: %.8llx, array_bank: %.4llx, status: %.16llx",
		__entry->cpu,
		__entry->activate & 0xffffffff,
		(__entry->activate >> 32) & 0xffff,
		__entry->status)

In general this is not called from a fast path within, i.e rate of triggering the tests is likely to be
"per minute" (Though in certain scenarios of driver retries it can be more frequent than that)


> 
> 		__entry->data >> 32,
> 		(__entry->data >> 16) & 0xffff,
> 
> Or something similar. That is, move the parsing of the bits to the
> output. libtraceevent should still be able to handle this.
> 

Jithu