Re: [PATCH 09/13] Move bp_type_idx to kernel/event/hw_breakpoint.c

On Tue, 15 Sep 2015 12:39:10 PDT (-0700), peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org wrote:

On Tue, Sep 15, 2015 at 11:40:11AM -0700, Palmer Dabbelt wrote:

quoted

On Tue, 15 Sep 2015 01:06:07 PDT (-0700), peterz-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org wrote:

quoted

On Mon, Sep 14, 2015 at 03:50:43PM -0700, Palmer Dabbelt wrote:

quoted

This has a "#ifdef CONFIG_*" that used to be exposed to userspace.

The names in here are so generic that I don't think it's a good idea
to expose them to userspace (or even the rest of the kernel).  Since
there's only one kernel user, it's been moved to that file.

Signed-off-by: Palmer Dabbelt <redacted>
Reviewed-by: Andrew Waterman <redacted>
Reviewed-by: Albert Ou <redacted>
---
 include/uapi/linux/hw_breakpoint.h | 10 ----------
 kernel/events/hw_breakpoint.c      | 10 ++++++++++
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/uapi/linux/hw_breakpoint.h b/include/uapi/linux/hw_breakpoint.h
index b04000a2296a..7a6a5a7f9511 100644
--- a/include/uapi/linux/hw_breakpoint.h
+++ b/include/uapi/linux/hw_breakpoint.h

@@ -17,14 +17,4 @@ enum {
 	HW_BREAKPOINT_INVALID   = HW_BREAKPOINT_RW | HW_BREAKPOINT_X,
 };

-enum bp_type_idx {
-	TYPE_INST 	= 0,
-#ifdef CONFIG_HAVE_MIXED_BREAKPOINTS_REGS
-	TYPE_DATA	= 0,
-#else
-	TYPE_DATA	= 1,
-#endif
-	TYPE_MAX
-};

This is rather unfortunate; you are correct that the naming is too
generic (and I tend to agree), but I think these values are required by
userspace to fill out:

  perf_event_attr::bp_type

So removing them will break things.

Frederic?

perf_event_open(2) says

       bp_type (since Linux 2.6.33)
              This chooses the breakpoint type.  It is one of:

              HW_BREAKPOINT_EMPTY
                     No breakpoint.

              HW_BREAKPOINT_R
                     Count when we read the memory location.

              HW_BREAKPOINT_W
                     Count when we write the memory location.

              HW_BREAKPOINT_RW
                     Count when we read or write the memory location.

              HW_BREAKPOINT_X
                     Count when we execute code at the memory location.

              The values can be combined via a bitwise or, but the combination
              of HW_BREAKPOINT_R or HW_BREAKPOINT_W  with  HW_BREAKPOINT_X  is
              not allowed.

so I think removing this enum from userspace is OK.  Did I miss
something?

Nah, could've just been me not being awake. Unless Frederic says
otherwise I'll chalk it up to not having drank enough morning juice.

Well, I'm going to leave this alone, then -- as Arnd pointed out in a
later email that got mis-threaded, this would be super tricky to fix
if userspace actually relied on it (which is why I was scared I was
wrong).

v4 (which is hopefully the final version of the patch set) will leave
this as it is.