From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Zijlstra Subject: Re: [PATCH 1/6] perf: Add new type PERF_TYPE_PROBE Date: Mon, 27 Nov 2017 08:58:50 +0100 Message-ID: <20171127075850.atthbazr4oywwoc6@hirez.programming.kicks-ass.net> References: <20171115172339.1791161-1-songliubraving@fb.com> <20171115172339.1791161-3-songliubraving@fb.com> <20171123100214.qm3s4h5b6i4r2iaa@hirez.programming.kicks-ass.net> <20171124082827.nvgr3bfu3bidfdjx@hirez.programming.kicks-ass.net> <11e688f0-c1bc-8925-225a-5b8e795336c7@fb.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: Song Liu , rostedt@goodmis.org, mingo@redhat.com, davem@davemloft.net, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, daniel@iogearbox.net, kernel-team@fb.com To: Alexei Starovoitov Return-path: Content-Disposition: inline In-Reply-To: <11e688f0-c1bc-8925-225a-5b8e795336c7@fb.com> Sender: linux-kernel-owner@vger.kernel.org List-Id: netdev.vger.kernel.org On Sat, Nov 25, 2017 at 05:59:54PM -0800, Alexei Starovoitov wrote: > If we were poking into 'struct perf_event_attr __user *uptr' > directly like get|put_user(.., &uptr->config) > then 32-bit user space with 4-byte aligned u64s would cause > 64-bit kernel to trap on archs like sparc. But surely archs that have hardware alignment requirements have __u64 == __aligned_u64 ? It's just that the structure layout can change between archs that have __u64 != __aligned_u64 and __u64 == __aligned_u64. But I would argue an architecture that has hardware alignment requirements and has an unaligned __u64 is just plain broken. > But in this case you're right. We can use config[12] as-is, since these > u64 fields are passing the value one way only (into the kernel) and > we do full perf_copy_attr() first and all further accesses are from > copied structure and u64_to_user_ptr(event->attr.config) will be fine. Right. Also note that there are no holes in perf_event_attr, if the structure itself is allocated aligned the individual fields will be aligned. > Do you mind we do > union { > __u64 file_path; > __u64 func_name; > __u64 config; > }; > and similar with config1 ? > Or prefer that we use 'config/config1' to store string+offset there? > I think config/config1 is cleaner than config1/config2 I would prefer you use config1/config2 for this and leave config itself for modifiers (like the retprobe thing). It also better lines up with the BP stuff. A little something like so perhaps: diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h index 362493a2f950..b6e76512f757 100644 --- a/include/uapi/linux/perf_event.h +++ b/include/uapi/linux/perf_event.h @@ -380,10 +380,14 @@ struct perf_event_attr { __u32 bp_type; union { __u64 bp_addr; + __u64 uprobe_path; + __u64 kprobe_func; __u64 config1; /* extension of config */ }; union { __u64 bp_len; + __u64 kprobe_addr; + __u64 probe_offset; __u64 config2; /* extension of config1 */ }; __u64 branch_sample_type; /* enum perf_branch_sample_type */