Messages in this thread | | | Date | Thu, 24 May 2012 14:16:57 +0200 | From | Jiri Olsa <> | Subject | Re: [PATCH 04/16] perf: Add ability to attach user stack dump to sample |
| |
On Thu, May 24, 2012 at 12:51:20PM +0200, Peter Zijlstra wrote: > On Wed, 2012-05-23 at 21:32 +0200, Jiri Olsa wrote: > > +static void
SNIP
> > + /* What couldn't be dumped is zero padded */ > > + while (rem--) { > > + char zero = 0; > > + perf_output_put(handle, zero); > > + } > > Does this matter? If we don't write it the worst that can happen is that > we leave previous ring-bugger content around, but since we already are > privileged to read that (and very likely already have) there's no > problem with that.. > > I know not zero-ing is ugly, but its also faster.. and do we care about > them silly zeros? hm, I dont think we care.. seems like this can go out
> > + if (sample_type & PERF_SAMPLE_STACK) { > > + u64 mode = event->attr.sample_stack; > > + > > + if (mode & PERF_SAMPLE_STACK_USER) { > > + u64 dump_size = event->attr.sample_stack_user; > > + > > + perf_output_sample_ustack(handle, dump_size, > > + data->regs_user); > > OK, so that function is called _ustack() I read that as userstack, so > why this strange split up? ook
> > > + } > > + } > > } > > > > void perf_prepare_sample(struct perf_event_header *header, > > @@ -4135,6 +4185,39 @@ void perf_prepare_sample(struct perf_event_header *header, > > > > header->size += size; > > } > > + > > + if (sample_type & PERF_SAMPLE_STACK) { > > + u64 mode = event->attr.sample_stack; > > + int size = 0; > > + > > + if (mode & PERF_SAMPLE_STACK_USER) { > > This is very much similar to ->sample_stack_user, since a non-zero size > usually means you want something.
ok, same case as for the regs bitmask stuf then.. we can use the size to check the presence in sample
> > > + if (!data->regs_user) > > + data->regs_user = perf_sample_regs_user(regs); > > + > > + /* > > + * A first field that tells the _static_ size of the > > + * dump. 0 if there is nothing to dump (ie: we are in > > + * a kernel thread) otherwise the requested size. > > + */ > > + size += sizeof(u64); > > + > > + /* > > + * If there is something to dump, add space for the > > + * dump itself and for the field that tells the > > + * dynamic size, which is how many have been actually > > + * dumped. What couldn't be dumped will be zero-padded. > > + */ > > + if (data->regs_user) { > > + u64 user_size = event->attr.sample_stack_user; > > + > > + user_size = round_up(user_size, sizeof(u64)); > > Right, and here we go again.. so how about you either reject sizes that > aren't properly aligned in perf_copy_attr() or just fix it up there.
right, we can do that in the attr check
| |