Re: [PATCH 04/16] perf: Add ability to attach user stack dump to sample

[Jiri Olsa <jolsa@redhat.com>]

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