Re: [PATCH v5 perf, bpf-next 1/7] perf, bpf: Introduce PERF_RECORD_KSYMBOL

[Song Liu]

Thanks Peter!

> On Jan 8, 2019, at 10:31 AM, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> 
> On Thu, Dec 20, 2018 at 10:28:58AM -0800, Song Liu wrote:
> 
>> @@ -648,11 +649,18 @@ struct perf_event_mmap_page {
>>  *   PERF_RECORD_MISC_COMM_EXEC  - PERF_RECORD_COMM event
>>  *   PERF_RECORD_MISC_FORK_EXEC  - PERF_RECORD_FORK event (perf internal)
>>  *   PERF_RECORD_MISC_SWITCH_OUT - PERF_RECORD_SWITCH* events
>> + *   PERF_RECORD_MISC_KSYMBOL_*  - PERF_RECORD_KSYMBOL event
>>  */
>> #define PERF_RECORD_MISC_MMAP_DATA		(1 << 13)
>> #define PERF_RECORD_MISC_COMM_EXEC		(1 << 13)
>> #define PERF_RECORD_MISC_FORK_EXEC		(1 << 13)
>> #define PERF_RECORD_MISC_SWITCH_OUT		(1 << 13)
>> +
>> +#define PERF_RECORD_MISC_KSYMBOL_UNREGISTER	(1 << 3)
>> +#define PERF_RECORD_MISC_KSYMBOL_TYPE_MASK	(7 << 4)
>> +#define PERF_RECORD_MISC_KSYMBOL_TYPE_UNKNOWN	(0 << 4)
>> +#define PERF_RECORD_MISC_KSYMBOL_TYPE_BPF	(1 << 4)
> 
> So this gives us 8 possible types, of which 2 are claimed.
> 
> I suppose we already know of FTRACE_TRAMPOLINE and MODULE, which
> accounts for half the space.
> 
>> +
>> /*
>>  * These PERF_RECORD_MISC_* flags below are safely reused
>>  * for the following events:
>> @@ -965,6 +973,19 @@ enum perf_event_type {
>> 	 */
>> 	PERF_RECORD_NAMESPACES			= 16,
>> 
>> +	/*
>> +	 * Record ksymbol register/unregister events:
>> +	 *
>> +	 * struct {
>> +	 *	struct perf_event_header	header;
>> +	 *	u64				addr;
>> +	 *	u64				len;
>> +	 *	char				name[];
>> +	 *	struct sample_id		sample_id;
>> +	 * };
>> +	 */
>> +	PERF_RECORD_KSYMBOL			= 17,
> 
> Do we really need u64 length? That is, are we willing to consider
> symbols larger than 4G ?
> 
> Could we not reclaim the top 32 or 16 bits thereof for that type thing
> instead of using a few misc bits?

I don't think we need u64 length. 32 bit should be more than enough. 

How about we revise it as:

+       /*
+        * Record ksymbol register/unregister events:
+        *
+        * struct {
+        *      struct perf_event_header        header;
+        *      u64                             addr;
+        *      u32                             len;
+        *      u16                             type;
+        *      u16                             reserved;
+        *      char                            name[];
+        *      struct sample_id                sample_id;
+        * };
+        */
+       PERF_RECORD_KSYMBOL                     = 17,

[...]

+#define PERF_RECORD_MISC_KSYMBOL_TYPE_UNKNOWN  0
+#define PERF_RECORD_MISC_KSYMBOL_TYPE_BPF      1
+#define PERF_RECORD_MISC_KSYMBOL_TYPE_MODULE   2
+#define PERF_RECORD_MISC_KSYMBOL_TYPE_FTRACE_TRAMPOLINE      3

Thanks again,
Song