Re: [PATCH V8 1/6] perf: Save PMU specific data in task_struct

["Liang, Kan" <kan.liang@linux.intel.com>]

On 2025-03-12 3:05 p.m., Peter Zijlstra wrote:
> 
> I'm sorry, but since I spotted a bug in the second patch, I'm going to
> reply and suggest some overall changes.

Sure. Thanks.

> 
> On Wed, Mar 12, 2025 at 11:25:20AM -0700, kan.liang@linux.intel.com wrote:
> 
>> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
>> index 3e270822b915..b8442047a2b6 100644
>> --- a/include/linux/perf_event.h
>> +++ b/include/linux/perf_event.h
>> @@ -1021,6 +1021,36 @@ struct perf_event_context {
>>  	local_t				nr_no_switch_fast;
>>  };
>>  
>> +/**
>> + * struct perf_ctx_data - PMU specific data for a task
>> + * @rcu_head:  To avoid the race on free PMU specific data
>> + * @refcount:  To track users
>> + * @global:    To track system-wide users
>> + * @ctx_cache: Kmem cache of PMU specific data
>> + * @data:      PMU specific data
>> + *
>> + * Currently, the struct is only used in Intel LBR call stack mode to
>> + * save/restore the call stack of a task on context switches.
>> + * The data only be allocated when Intel LBR call stack mode is enabled.
>> + * The data will be freed when the mode is disabled. The rcu_head is
>> + * used to prevent the race on free the data.
>> + * The content of the data will only be accessed in context switch, which
>> + * should be protected by rcu_read_lock().
>> + *
>> + * Careful: Struct perf_ctx_data is added as a pointor in struct task_struct.
> 
> pointer
> 
>> + * When system-wide Intel LBR call stack mode is enabled, a buffer with
>> + * constant size will be allocated for each task.
>> + * Also, system memory consumption can further grow when the size of
>> + * struct perf_ctx_data enlarges.
>> + */
>> +struct perf_ctx_data {
>> +	struct rcu_head			rcu_head;
>> +	refcount_t			refcount;
>> +	int				global;
>> +	struct kmem_cache		*ctx_cache;
>> +	void				*data;
>> +};
> 
> I can't remember why this is complicated like this. Why do we have a
> kmemcache and yet another data pointer in there?

The kmem_cache is introduced to address the alignment requirement for
Arch LBR.
https://lore.kernel.org/lkml/159420190705.4006.11190540790919295173.tip-bot2@tip-bot2/

When users do system-wide profiling, perf has to allocate a buffer when
forking a thread or delete a buffer when deleting a thread. The
pmu->task_ctx_cache is required. Perf has to search the perf_event_list
every time to find the proper PMU.
So the *ctx_cache is introduced to avoid the search.

Thanks,
Kan

> 
> Specifically, why can't we do something like:
> 
> struct perf_ctx_data {
> 	struct rcu_head		rcu;
> 	refcount_t		refcount;
> 	int			global;
> 	char			data[];
> };
> 
> and simply allocate the whole thing as a single allocation?
> 
> So then the allocation is something like:
> 
> 	cd = kzalloc(sizeof(*cd) + event->pmu->task_ctx_size, GFP_KERNEL);
> 
>