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

Thanks Peter!

On Jan 8, 2019, at 10:31 AM, Peter Zijlstra [off-list ref] wrote:

On Thu, Dec 20, 2018 at 10:28:58AM -0800, Song Liu wrote:

quoted

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

quoted

+
/*
 * 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