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
<ma...@linux.vnet.ibm.com> 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