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@xxxxxxxxx> 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
