* [perf] perf_event.h ABI visibility question
@ 2018-08-23 18:25 Vince Weaver
2018-08-24 8:50 ` Peter Zijlstra
0 siblings, 1 reply; 6+ messages in thread
From: Vince Weaver @ 2018-08-23 18:25 UTC (permalink / raw)
To: Peter Zijlstra, linux-kernel
Cc: Josh Poimboeuf, Alexander Shishkin, Andy Lutomirski,
Arnaldo Carvalho de Melo, Arnaldo Carvalho de Melo, Jiri Olsa,
Linus Torvalds, Stephane Eranian, Thomas Gleixner, Ingo Molnar
I notice that Linux 4.18 has the following changeset which changes the
user visible perf_event.h file
commit 6cbc304f2f360f25cc8607817239d6f4a2fd3dc5
Author: Peter Zijlstra <peterz@infradead.org>
Date: Thu May 10 15:48:41 2018 +0200
perf/x86/intel: Fix unwind errors from PEBS entries (mk-II)
which contains
--- a/include/uapi/linux/perf_event.h
+++ b/include/uapi/linux/perf_event.h
@@ -143,6 +143,8 @@ enum perf_event_sample_format {
PERF_SAMPLE_PHYS_ADDR = 1U << 19,
PERF_SAMPLE_MAX = 1U << 20, /* non-ABI */
+
+ __PERF_SAMPLE_CALLCHAIN_EARLY = 1ULL << 63,
};
Is this supposed to be a user-visible interface?
I realize that if the user tries to set anything above PERF_SAMPLE_MAX
it will be caught and flagged as EINVAL.
However even with the double-underscore hint in
__PERF_SAMPLE_CALLCHAIN_EARLY the value is still in the user-visible
header so it's now part of the ABI and I guess the manpage has to document it.
Vince
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [perf] perf_event.h ABI visibility question 2018-08-23 18:25 [perf] perf_event.h ABI visibility question Vince Weaver @ 2018-08-24 8:50 ` Peter Zijlstra 2018-08-24 21:09 ` Vince Weaver 0 siblings, 1 reply; 6+ messages in thread From: Peter Zijlstra @ 2018-08-24 8:50 UTC (permalink / raw) To: Vince Weaver Cc: linux-kernel, Josh Poimboeuf, Alexander Shishkin, Andy Lutomirski, Arnaldo Carvalho de Melo, Arnaldo Carvalho de Melo, Jiri Olsa, Linus Torvalds, Stephane Eranian, Thomas Gleixner, Ingo Molnar On Thu, Aug 23, 2018 at 02:25:06PM -0400, Vince Weaver wrote: > > I notice that Linux 4.18 has the following changeset which changes the > user visible perf_event.h file > > commit 6cbc304f2f360f25cc8607817239d6f4a2fd3dc5 > Author: Peter Zijlstra <peterz@infradead.org> > Date: Thu May 10 15:48:41 2018 +0200 > > perf/x86/intel: Fix unwind errors from PEBS entries (mk-II) > > which contains > > --- a/include/uapi/linux/perf_event.h > +++ b/include/uapi/linux/perf_event.h > @@ -143,6 +143,8 @@ enum perf_event_sample_format { > PERF_SAMPLE_PHYS_ADDR = 1U << 19, > > PERF_SAMPLE_MAX = 1U << 20, /* non-ABI */ > + > + __PERF_SAMPLE_CALLCHAIN_EARLY = 1ULL << 63, > }; > > > Is this supposed to be a user-visible interface? > > I realize that if the user tries to set anything above PERF_SAMPLE_MAX > it will be caught and flagged as EINVAL. > > However even with the double-underscore hint in > __PERF_SAMPLE_CALLCHAIN_EARLY the value is still in the user-visible > header so it's now part of the ABI and I guess the manpage has to document it. Hurphm.. visible yes, but as you say, also quite useless. Does it really make sense to document that? ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [perf] perf_event.h ABI visibility question 2018-08-24 8:50 ` Peter Zijlstra @ 2018-08-24 21:09 ` Vince Weaver 2018-08-27 7:52 ` Peter Zijlstra 0 siblings, 1 reply; 6+ messages in thread From: Vince Weaver @ 2018-08-24 21:09 UTC (permalink / raw) To: Peter Zijlstra Cc: linux-kernel, Josh Poimboeuf, Alexander Shishkin, Andy Lutomirski, Arnaldo Carvalho de Melo, Arnaldo Carvalho de Melo, Jiri Olsa, Linus Torvalds, Stephane Eranian, Thomas Gleixner, Ingo Molnar On Fri, 24 Aug 2018, Peter Zijlstra wrote: > > +++ b/include/uapi/linux/perf_event.h > > @@ -143,6 +143,8 @@ enum perf_event_sample_format { > > PERF_SAMPLE_PHYS_ADDR = 1U << 19, > > > > PERF_SAMPLE_MAX = 1U << 20, /* non-ABI */ > > + > > + __PERF_SAMPLE_CALLCHAIN_EARLY = 1ULL << 63, > > }; > > > > Hurphm.. visible yes, but as you say, also quite useless. Does it really > make sense to document that? Well, it should probably be documented either in the manpage or else in perf_event.h (even if it's just "internal use, don't use") as we can't really expect people to download a git tree and do a git-blame to try to figure out what this mysterious field is all about. Also, this change increased the size of the enum from 32 to 64 bits on 32-bit machines, though that only really matters if the user is doing something really weird with enum variables. Vince ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [perf] perf_event.h ABI visibility question 2018-08-24 21:09 ` Vince Weaver @ 2018-08-27 7:52 ` Peter Zijlstra 2018-08-28 17:51 ` Vince Weaver 0 siblings, 1 reply; 6+ messages in thread From: Peter Zijlstra @ 2018-08-27 7:52 UTC (permalink / raw) To: Vince Weaver Cc: linux-kernel, Josh Poimboeuf, Alexander Shishkin, Andy Lutomirski, Arnaldo Carvalho de Melo, Arnaldo Carvalho de Melo, Jiri Olsa, Linus Torvalds, Stephane Eranian, Thomas Gleixner, Ingo Molnar On Fri, Aug 24, 2018 at 05:09:27PM -0400, Vince Weaver wrote: > On Fri, 24 Aug 2018, Peter Zijlstra wrote: > > > > +++ b/include/uapi/linux/perf_event.h > > > @@ -143,6 +143,8 @@ enum perf_event_sample_format { > > > PERF_SAMPLE_PHYS_ADDR = 1U << 19, > > > > > > PERF_SAMPLE_MAX = 1U << 20, /* non-ABI */ > > > + > > > + __PERF_SAMPLE_CALLCHAIN_EARLY = 1ULL << 63, > > > }; > > > > > > > Hurphm.. visible yes, but as you say, also quite useless. Does it really > > make sense to document that? > > Well, it should probably be documented either in the manpage or else in > perf_event.h (even if it's just "internal use, don't use") as we can't > really expect people to download a git tree and do a git-blame to try to > figure out what this mysterious field is all about. Something like so then? diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h index eeb787b1c53c..f35eb72739c0 100644 --- a/include/uapi/linux/perf_event.h +++ b/include/uapi/linux/perf_event.h @@ -144,7 +144,7 @@ enum perf_event_sample_format { PERF_SAMPLE_MAX = 1U << 20, /* non-ABI */ - __PERF_SAMPLE_CALLCHAIN_EARLY = 1ULL << 63, + __PERF_SAMPLE_CALLCHAIN_EARLY = 1ULL << 63, /* non-ABI; internal use */ }; /* > Also, this change increased the size of the enum from 32 to 64 bits on > 32-bit machines, though that only really matters if the user is doing > something really weird with enum variables. Yeah, since the target variable is a u64 I really can't be bothered with that. ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [perf] perf_event.h ABI visibility question 2018-08-27 7:52 ` Peter Zijlstra @ 2018-08-28 17:51 ` Vince Weaver 2018-08-29 12:13 ` Peter Zijlstra 0 siblings, 1 reply; 6+ messages in thread From: Vince Weaver @ 2018-08-28 17:51 UTC (permalink / raw) To: Peter Zijlstra Cc: Vince Weaver, linux-kernel, Josh Poimboeuf, Alexander Shishkin, Andy Lutomirski, Arnaldo Carvalho de Melo, Arnaldo Carvalho de Melo, Jiri Olsa, Linus Torvalds, Stephane Eranian, Thomas Gleixner, Ingo Molnar On Mon, 27 Aug 2018, Peter Zijlstra wrote: > Something like so then? > > diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h > index eeb787b1c53c..f35eb72739c0 100644 > --- a/include/uapi/linux/perf_event.h > +++ b/include/uapi/linux/perf_event.h > @@ -144,7 +144,7 @@ enum perf_event_sample_format { > > PERF_SAMPLE_MAX = 1U << 20, /* non-ABI */ > > - __PERF_SAMPLE_CALLCHAIN_EARLY = 1ULL << 63, > + __PERF_SAMPLE_CALLCHAIN_EARLY = 1ULL << 63, /* non-ABI; internal use */ > }; yes, something like that would be fine. I am being difficult about this, but from experience when trying to keep the manpage updated, what seems obvious now will not be so obvious 6 months from now and trying to dig through the git logs / mailing list archives to verify the purpose of an addition can be a lot of work sometimes. Vince ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [perf] perf_event.h ABI visibility question 2018-08-28 17:51 ` Vince Weaver @ 2018-08-29 12:13 ` Peter Zijlstra 0 siblings, 0 replies; 6+ messages in thread From: Peter Zijlstra @ 2018-08-29 12:13 UTC (permalink / raw) To: Vince Weaver Cc: linux-kernel, Josh Poimboeuf, Alexander Shishkin, Andy Lutomirski, Arnaldo Carvalho de Melo, Arnaldo Carvalho de Melo, Jiri Olsa, Linus Torvalds, Stephane Eranian, Thomas Gleixner, Ingo Molnar On Tue, Aug 28, 2018 at 01:51:29PM -0400, Vince Weaver wrote: > On Mon, 27 Aug 2018, Peter Zijlstra wrote: > > > Something like so then? > > > > diff --git a/include/uapi/linux/perf_event.h b/include/uapi/linux/perf_event.h > > index eeb787b1c53c..f35eb72739c0 100644 > > --- a/include/uapi/linux/perf_event.h > > +++ b/include/uapi/linux/perf_event.h > > @@ -144,7 +144,7 @@ enum perf_event_sample_format { > > > > PERF_SAMPLE_MAX = 1U << 20, /* non-ABI */ > > > > - __PERF_SAMPLE_CALLCHAIN_EARLY = 1ULL << 63, > > + __PERF_SAMPLE_CALLCHAIN_EARLY = 1ULL << 63, /* non-ABI; internal use */ > > }; > > yes, something like that would be fine. > > I am being difficult about this, but from experience when trying to keep > the manpage updated, what seems obvious now will not be so obvious 6 > months from now and trying to dig through the git logs / mailing list > archives to verify the purpose of an addition can be a lot of work > sometimes. Yeah, no worries. I appreciate you paying attention to these things. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-08-29 12:14 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-08-23 18:25 [perf] perf_event.h ABI visibility question Vince Weaver 2018-08-24 8:50 ` Peter Zijlstra 2018-08-24 21:09 ` Vince Weaver 2018-08-27 7:52 ` Peter Zijlstra 2018-08-28 17:51 ` Vince Weaver 2018-08-29 12:13 ` Peter Zijlstra
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).