I notice that Linux 4.18 has the following changeset which changes the
user visible perf_event.h file
commit 6cbc304f2f360f25cc8607817239d6f4a2fd3dc5
Author: Peter Zijlstra <[email protected]>
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
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 <[email protected]>
> 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?
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
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.
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
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.