Re: [PATCH 04/13] perf/core: Extend perf_output_sample_regs() to include perf_arch_regs

[Madhavan Srinivasan]

On Tuesday 30 August 2016 09:41 PM, Nilay Vaish wrote:
> On 28 August 2016 at 16:00, Madhavan Srinivasan
> <maddy@xxxxxxxxxxxxxxxxxx> wrote:
> > diff --git a/kernel/events/core.c b/kernel/events/core.c
> > index 274288819829..e16bf4d057d1 100644
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -5371,16 +5371,24 @@ u64 __attribute__((weak)) perf_arch_reg_value(struct perf_arch_regs *regs,
> >
> >   static void
> >   perf_output_sample_regs(struct perf_output_handle *handle,
> > -                       struct pt_regs *regs, u64 mask)
> > +                               struct perf_regs *regs, u64 mask)
> >   {
> >          int bit;
> >          DECLARE_BITMAP(_mask, 64);
> > +       u64 arch_regs_mask = regs->arch_regs_mask;
> >
> >          bitmap_from_u64(_mask, mask);
> >          for_each_set_bit(bit, _mask, sizeof(mask) * BITS_PER_BYTE) {
> >                  u64 val;
> >
> > -               val = perf_reg_value(regs, bit);
> > +               val = perf_reg_value(regs->regs, bit);
> > +               perf_output_put(handle, val);
> > +       }
> > +
> > +       bitmap_from_u64(_mask, arch_regs_mask);
> > +       for_each_set_bit(bit, _mask, sizeof(mask) * BITS_PER_BYTE) {
> > +               u64 val;
> > +               val = perf_arch_reg_value(regs->arch_regs, bit);
> >                  perf_output_put(handle, val);
> >          }
> >   }
> > @@ -5792,7 +5800,7 @@ void perf_output_sample(struct perf_output_handle *handle,
> >                  if (abi) {
> >                          u64 mask = event->attr.sample_regs_user;
> >                          perf_output_sample_regs(handle,
> > -                                               data->regs_user.regs,
> > +                                               &data->regs_user,
> >                                                  mask);
> >                  }
> >          }
> > @@ -5827,7 +5835,7 @@ void perf_output_sample(struct perf_output_handle *handle,
> >                          u64 mask = event->attr.sample_regs_intr;
> >
> >                          perf_output_sample_regs(handle,
> > -                                               data->regs_intr.regs,
> > +                                               &data->regs_intr,
> >                                                  mask);
> >                  }
> >          }
> > --
> > 2.7.4
> I would like to suggest a slightly different version.  Would it make
> more sense to have something like following:

I agree we are outputting two different structures, but since we use the
INTR_REG infrastructure to dump the arch pmu registers, I preferred to
extend perf_output_sample_regs. But I guess I can break it up.

Maddy

> @@ -5792,7 +5800,7 @@ void perf_output_sample(struct perf_output_handle *handle,
>                   if (abi) {
>                          u64 mask = event->attr.sample_regs_user;
>                          perf_output_sample_regs(handle,
>                                                  data->regs_user.regs,
>                                                  mask);
>                  }
> +
> +              if (arch_regs_mask) {
> +                   perf_output_pmu_regs(handle,
> data->regs_users.arch_regs, arch_regs_mask);
> +              }
>          }
>
>
> Somehow I don't like outputting the two sets of registers through the
> same function call.
>
> --
> Nilay