[PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Mike Frysinger]

If the port does not support HAVE_PERF_COUNTERS, then they can't support
the perf_counter_open syscall either.  Rather than forcing everyone to add
an ignore (or suffer the warning until they get around to implementing
support), only whine about the syscall when applicable.

Signed-off-by: Mike Frysinger <vapier@gentoo.org>
---
 scripts/checksyscalls.sh |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

diff --git a/scripts/checksyscalls.sh b/scripts/checksyscalls.sh
index 60d00d1..90fb953 100755
--- a/scripts/checksyscalls.sh
+++ b/scripts/checksyscalls.sh
@@ -104,6 +104,11 @@ cat << EOF
 #define __IGNORE_sync_file_range
 #endif
 
+/* if the port doesn't have support for it yet */
+#ifndef CONFIG_HAVE_PERF_COUNTERS
+#define __IGNORE_perf_counter_open
+#endif
+
 /* Unmerged syscalls for AFS, STREAMS, etc. */
 #define __IGNORE_afs_syscall
 #define __IGNORE_getpmsg
-- 
1.6.3.1

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Ingo Molnar]

* Mike Frysinger <vapier@gentoo.org> wrote:

> If the port does not support HAVE_PERF_COUNTERS, then they can't 
> support the perf_counter_open syscall either.  Rather than forcing 
> everyone to add an ignore (or suffer the warning until they get 
> around to implementing support), only whine about the syscall when 
> applicable.

No, this patch is wrong - it's really easy to add support: just hook 
up the syscall. This should happen for every architecture really, so 
the warning is correct and it should not be patched out.

PMU support is not required to get perfcounters support: if an 
architecture hooks up the syscall it will get generic software 
counters and the tools will work as well.

Profiling falls back to a hrtimer-based sampling method - this is a 
much better fallback than oprofile's fall-back to the timer tick. 
This hrtimer based sampling is dynticks/nohz-correct and can go 
beyond HZ if the architecture supports hrtimers.

Thanks,

	Ingo

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Mike Frysinger]

On Fri, Jun 12, 2009 at 08:05, Ingo Molnar wrote:
> * Mike Frysinger <vapier@gentoo.org> wrote:
>> If the port does not support HAVE_PERF_COUNTERS, then they can't
>> support the perf_counter_open syscall either.  Rather than forcing
>> everyone to add an ignore (or suffer the warning until they get
>> around to implementing support), only whine about the syscall when
>> applicable.
>
> No, this patch is wrong - it's really easy to add support: just hook
> up the syscall. This should happen for every architecture really, so
> the warning is correct and it should not be patched out.
>
> PMU support is not required to get perfcounters support: if an
> architecture hooks up the syscall it will get generic software
> counters and the tools will work as well.
>
> Profiling falls back to a hrtimer-based sampling method - this is a
> much better fallback than oprofile's fall-back to the timer tick.
> This hrtimer based sampling is dynticks/nohz-correct and can go
> beyond HZ if the architecture supports hrtimers.

if there is generic support available, why must every arch select
HAVE_PERF_COUNTERS in their Kconfig ?
-mike

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Ingo Molnar]

* Mike Frysinger <vapier.adi@gmail.com> wrote:

> On Fri, Jun 12, 2009 at 08:05, Ingo Molnar wrote:
> > * Mike Frysinger <vapier@gentoo.org> wrote:
> >> If the port does not support HAVE_PERF_COUNTERS, then they can't
> >> support the perf_counter_open syscall either.  Rather than forcing
> >> everyone to add an ignore (or suffer the warning until they get
> >> around to implementing support), only whine about the syscall when
> >> applicable.
> >
> > No, this patch is wrong - it's really easy to add support: just hook
> > up the syscall. This should happen for every architecture really, so
> > the warning is correct and it should not be patched out.
> >
> > PMU support is not required to get perfcounters support: if an
> > architecture hooks up the syscall it will get generic software
> > counters and the tools will work as well.
> >
> > Profiling falls back to a hrtimer-based sampling method - this is a
> > much better fallback than oprofile's fall-back to the timer tick.
> > This hrtimer based sampling is dynticks/nohz-correct and can go
> > beyond HZ if the architecture supports hrtimers.
> 
> if there is generic support available, why must every arch select 
> HAVE_PERF_COUNTERS in their Kconfig ?

Because we only want to enable it on architectures that have tested 
it. It should only need a syscall addition, but nothing beats having 
tested things, hence we have that additional Kconfig symbol.

Thanks,

	Ingo

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Mike Frysinger]

On Fri, Jun 12, 2009 at 08:17, Ingo Molnar wrote:
> * Mike Frysinger <vapier.adi@gmail.com> wrote:
>> On Fri, Jun 12, 2009 at 08:05, Ingo Molnar wrote:
>> > * Mike Frysinger <vapier@gentoo.org> wrote:
>> >> If the port does not support HAVE_PERF_COUNTERS, then they can't
>> >> support the perf_counter_open syscall either.  Rather than forcing
>> >> everyone to add an ignore (or suffer the warning until they get
>> >> around to implementing support), only whine about the syscall when
>> >> applicable.
>> >
>> > No, this patch is wrong - it's really easy to add support: just hook
>> > up the syscall. This should happen for every architecture really, so
>> > the warning is correct and it should not be patched out.
>> >
>> > PMU support is not required to get perfcounters support: if an
>> > architecture hooks up the syscall it will get generic software
>> > counters and the tools will work as well.
>> >
>> > Profiling falls back to a hrtimer-based sampling method - this is a
>> > much better fallback than oprofile's fall-back to the timer tick.
>> > This hrtimer based sampling is dynticks/nohz-correct and can go
>> > beyond HZ if the architecture supports hrtimers.
>>
>> if there is generic support available, why must every arch select
>> HAVE_PERF_COUNTERS in their Kconfig ?
>
> Because we only want to enable it on architectures that have tested
> it. It should only need a syscall addition, but nothing beats having
> tested things, hence we have that additional Kconfig symbol.

that is a pretty weak reason.  nothing changes by default -- people
still have to go in and enable it.  arch people have to jump through
hoops for absolutely no reason.  if it were enabled by default, arches
could simply hook up the syscall and then random people could test it
regardless of the arch people noticing.  if anything, the arch syscall
hookup could be done by someone related to the prof code who really
wanted to see wider arch testing.
-mike

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Ingo Molnar]

* Mike Frysinger <vapier.adi@gmail.com> wrote:

> On Fri, Jun 12, 2009 at 08:17, Ingo Molnar wrote:
> > * Mike Frysinger <vapier.adi@gmail.com> wrote:
> >> On Fri, Jun 12, 2009 at 08:05, Ingo Molnar wrote:
> >> > * Mike Frysinger <vapier@gentoo.org> wrote:
> >> >> If the port does not support HAVE_PERF_COUNTERS, then they can't
> >> >> support the perf_counter_open syscall either.  Rather than forcing
> >> >> everyone to add an ignore (or suffer the warning until they get
> >> >> around to implementing support), only whine about the syscall when
> >> >> applicable.
> >> >
> >> > No, this patch is wrong - it's really easy to add support: just hook
> >> > up the syscall. This should happen for every architecture really, so
> >> > the warning is correct and it should not be patched out.
> >> >
> >> > PMU support is not required to get perfcounters support: if an
> >> > architecture hooks up the syscall it will get generic software
> >> > counters and the tools will work as well.
> >> >
> >> > Profiling falls back to a hrtimer-based sampling method - this is a
> >> > much better fallback than oprofile's fall-back to the timer tick.
> >> > This hrtimer based sampling is dynticks/nohz-correct and can go
> >> > beyond HZ if the architecture supports hrtimers.
> >>
> >> if there is generic support available, why must every arch select
> >> HAVE_PERF_COUNTERS in their Kconfig ?
> >
> > Because we only want to enable it on architectures that have tested
> > it. It should only need a syscall addition, but nothing beats having
> > tested things, hence we have that additional Kconfig symbol.
> 
> that is a pretty weak reason. [...]

It isnt - this is proper isolation - dont offer something to the 
user to enable that 1) cannot be used due to the lack of a syscall 
2) has not been tested by anyone on that architecture, ever.

That way say build breakages or runtime failures due to perfcounters 
only become possible on an architecture if the architecture 
maintainer has hooked up the syscall and has provided 
HAVE_PERF_COUNTERS explicitly.

This is a basic engineering principle really.

	Ingo

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Mike Frysinger]

On Fri, Jun 12, 2009 at 08:31, Ingo Molnar wrote:
> * Mike Frysinger <vapier.adi@gmail.com> wrote:
>> On Fri, Jun 12, 2009 at 08:17, Ingo Molnar wrote:
>> > * Mike Frysinger <vapier.adi@gmail.com> wrote:
>> >> On Fri, Jun 12, 2009 at 08:05, Ingo Molnar wrote:
>> >> > * Mike Frysinger <vapier@gentoo.org> wrote:
>> >> >> If the port does not support HAVE_PERF_COUNTERS, then they can't
>> >> >> support the perf_counter_open syscall either.  Rather than forcing
>> >> >> everyone to add an ignore (or suffer the warning until they get
>> >> >> around to implementing support), only whine about the syscall when
>> >> >> applicable.
>> >> >
>> >> > No, this patch is wrong - it's really easy to add support: just hook
>> >> > up the syscall. This should happen for every architecture really, so
>> >> > the warning is correct and it should not be patched out.
>> >> >
>> >> > PMU support is not required to get perfcounters support: if an
>> >> > architecture hooks up the syscall it will get generic software
>> >> > counters and the tools will work as well.
>> >> >
>> >> > Profiling falls back to a hrtimer-based sampling method - this is a
>> >> > much better fallback than oprofile's fall-back to the timer tick.
>> >> > This hrtimer based sampling is dynticks/nohz-correct and can go
>> >> > beyond HZ if the architecture supports hrtimers.
>> >>
>> >> if there is generic support available, why must every arch select
>> >> HAVE_PERF_COUNTERS in their Kconfig ?
>> >
>> > Because we only want to enable it on architectures that have tested
>> > it. It should only need a syscall addition, but nothing beats having
>> > tested things, hence we have that additional Kconfig symbol.
>>
>> that is a pretty weak reason. [...]
>
> It isnt - this is proper isolation - dont offer something to the
> user to enable that 1) cannot be used due to the lack of a syscall
> 2) has not been tested by anyone on that architecture, ever.
>
> That way say build breakages or runtime failures due to perfcounters
> only become possible on an architecture if the architecture
> maintainer has hooked up the syscall and has provided
> HAVE_PERF_COUNTERS explicitly.

except that the syscall presence is trivial to detect in the code by
something like:
#ifndef __NR_perf_counter_open
# error sorry, your arch has not hooked up perf_counter_open syscall yet
#endif

as for "no arch testing yet", there are plenty of drivers which lack
arch depends in the Kconfig specifically so that it can be *easily*
tested on random systems out there without requiring manual twiddling.
 having it not build on an arch results in someone noticing and things
getting fixed.  having it not even be an option means it often times
goes largely unnoticed.

the missing syscall is obvious for arch porters -- they get a warning
on every make.  the need for a useless HAVE_PERF_COUNTERS in the
Kconfig however is completely unknown because there's (yet again) 0
information on this define and what it takes for arch people to add
support for it.  in this case, the define is completely spurious.
-mike

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Ingo Molnar]

* Mike Frysinger <vapier.adi@gmail.com> wrote:

> On Fri, Jun 12, 2009 at 08:31, Ingo Molnar wrote:
> > * Mike Frysinger <vapier.adi@gmail.com> wrote:
> >> On Fri, Jun 12, 2009 at 08:17, Ingo Molnar wrote:
> >> > * Mike Frysinger <vapier.adi@gmail.com> wrote:
> >> >> On Fri, Jun 12, 2009 at 08:05, Ingo Molnar wrote:
> >> >> > * Mike Frysinger <vapier@gentoo.org> wrote:
> >> >> >> If the port does not support HAVE_PERF_COUNTERS, then they can't
> >> >> >> support the perf_counter_open syscall either.  Rather than forcing
> >> >> >> everyone to add an ignore (or suffer the warning until they get
> >> >> >> around to implementing support), only whine about the syscall when
> >> >> >> applicable.
> >> >> >
> >> >> > No, this patch is wrong - it's really easy to add support: just hook
> >> >> > up the syscall. This should happen for every architecture really, so
> >> >> > the warning is correct and it should not be patched out.
> >> >> >
> >> >> > PMU support is not required to get perfcounters support: if an
> >> >> > architecture hooks up the syscall it will get generic software
> >> >> > counters and the tools will work as well.
> >> >> >
> >> >> > Profiling falls back to a hrtimer-based sampling method - this is a
> >> >> > much better fallback than oprofile's fall-back to the timer tick.
> >> >> > This hrtimer based sampling is dynticks/nohz-correct and can go
> >> >> > beyond HZ if the architecture supports hrtimers.
> >> >>
> >> >> if there is generic support available, why must every arch select
> >> >> HAVE_PERF_COUNTERS in their Kconfig ?
> >> >
> >> > Because we only want to enable it on architectures that have tested
> >> > it. It should only need a syscall addition, but nothing beats having
> >> > tested things, hence we have that additional Kconfig symbol.
> >>
> >> that is a pretty weak reason. [...]
> >
> > It isnt - this is proper isolation - dont offer something to the
> > user to enable that 1) cannot be used due to the lack of a syscall
> > 2) has not been tested by anyone on that architecture, ever.
> >
> > That way say build breakages or runtime failures due to perfcounters
> > only become possible on an architecture if the architecture
> > maintainer has hooked up the syscall and has provided
> > HAVE_PERF_COUNTERS explicitly.
> 
> except that the syscall presence is trivial to detect in the code by
> something like:
> #ifndef __NR_perf_counter_open
> # error sorry, your arch has not hooked up perf_counter_open syscall yet
> #endif
> 
> as for "no arch testing yet", there are plenty of drivers which lack
> arch depends in the Kconfig specifically so that it can be *easily*
> tested on random systems out there without requiring manual twiddling.

This is a new kernel subsystem, not just yet another driver.

Which bit of: "we dont want perfcounters to be enabled in the 
Kconfig on architectures that have no syscalls and no testing for 
it" is hard to understand? It is a valid technical concern.

I on the other hand fail to see what specific problem your patch is 
trying to solve.

Anyway - feel free to apply that hack to checksyscalls if you want 
to hide the lack of a feature that could be supported by the 
architecture - we certainly wont enable perfcounters on 
architectures that havent got it tested first.

	Ingo

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Mike Frysinger]

On Fri, Jun 12, 2009 at 08:59, Ingo Molnar wrote:
> * Mike Frysinger <vapier.adi@gmail.com> wrote:
>> On Fri, Jun 12, 2009 at 08:31, Ingo Molnar wrote:
>> > * Mike Frysinger <vapier.adi@gmail.com> wrote:
>> >> On Fri, Jun 12, 2009 at 08:17, Ingo Molnar wrote:
>> >> > * Mike Frysinger <vapier.adi@gmail.com> wrote:
>> >> >> On Fri, Jun 12, 2009 at 08:05, Ingo Molnar wrote:
>> >> >> > * Mike Frysinger <vapier@gentoo.org> wrote:
>> >> >> >> If the port does not support HAVE_PERF_COUNTERS, then they can't
>> >> >> >> support the perf_counter_open syscall either.  Rather than forcing
>> >> >> >> everyone to add an ignore (or suffer the warning until they get
>> >> >> >> around to implementing support), only whine about the syscall when
>> >> >> >> applicable.
>> >> >> >
>> >> >> > No, this patch is wrong - it's really easy to add support: just hook
>> >> >> > up the syscall. This should happen for every architecture really, so
>> >> >> > the warning is correct and it should not be patched out.
>> >> >> >
>> >> >> > PMU support is not required to get perfcounters support: if an
>> >> >> > architecture hooks up the syscall it will get generic software
>> >> >> > counters and the tools will work as well.
>> >> >> >
>> >> >> > Profiling falls back to a hrtimer-based sampling method - this is a
>> >> >> > much better fallback than oprofile's fall-back to the timer tick.
>> >> >> > This hrtimer based sampling is dynticks/nohz-correct and can go
>> >> >> > beyond HZ if the architecture supports hrtimers.
>> >> >>
>> >> >> if there is generic support available, why must every arch select
>> >> >> HAVE_PERF_COUNTERS in their Kconfig ?
>> >> >
>> >> > Because we only want to enable it on architectures that have tested
>> >> > it. It should only need a syscall addition, but nothing beats having
>> >> > tested things, hence we have that additional Kconfig symbol.
>> >>
>> >> that is a pretty weak reason. [...]
>> >
>> > It isnt - this is proper isolation - dont offer something to the
>> > user to enable that 1) cannot be used due to the lack of a syscall
>> > 2) has not been tested by anyone on that architecture, ever.
>> >
>> > That way say build breakages or runtime failures due to perfcounters
>> > only become possible on an architecture if the architecture
>> > maintainer has hooked up the syscall and has provided
>> > HAVE_PERF_COUNTERS explicitly.
>>
>> except that the syscall presence is trivial to detect in the code by
>> something like:
>> #ifndef __NR_perf_counter_open
>> # error sorry, your arch has not hooked up perf_counter_open syscall yet
>> #endif
>>
>> as for "no arch testing yet", there are plenty of drivers which lack
>> arch depends in the Kconfig specifically so that it can be *easily*
>> tested on random systems out there without requiring manual twiddling.
>
> This is a new kernel subsystem, not just yet another driver.

so what ?  if it has generic pieces, it is exactly the same as yet
another generic driver.  people should be able to randomly test build
it when possible to discover latent issues that your testing limited
to one arch did not find.

> Which bit of: "we dont want perfcounters to be enabled in the
> Kconfig on architectures that have no syscalls and no testing for
> it" is hard to understand? It is a valid technical concern.

your (1) is valid but i already pointed out a simple fix for that.
your (2) is not.

> I on the other hand fail to see what specific problem your patch is
> trying to solve.

i'm not debating the merit of my patch.  like you said, it is wrong.
i am however debating the validity of a Kconfig option that
masquerades as yet another useless:
depends on X86 || PPC || SH || ....

> Anyway - feel free to apply that hack to checksyscalls if you want
> to hide the lack of a feature that could be supported by the
> architecture - we certainly wont enable perfcounters on
> architectures that havent got it tested first.

no one is asking you to enable it.  that is for the user to do it.
-mike

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Ingo Molnar]

* Mike Frysinger <vapier.adi@gmail.com> wrote:

> On Fri, Jun 12, 2009 at 08:59, Ingo Molnar wrote:
> > * Mike Frysinger <vapier.adi@gmail.com> wrote:
> >> On Fri, Jun 12, 2009 at 08:31, Ingo Molnar wrote:
> >> > * Mike Frysinger <vapier.adi@gmail.com> wrote:
> >> >> On Fri, Jun 12, 2009 at 08:17, Ingo Molnar wrote:
> >> >> > * Mike Frysinger <vapier.adi@gmail.com> wrote:
> >> >> >> On Fri, Jun 12, 2009 at 08:05, Ingo Molnar wrote:
> >> >> >> > * Mike Frysinger <vapier@gentoo.org> wrote:
> >> >> >> >> If the port does not support HAVE_PERF_COUNTERS, then they can't
> >> >> >> >> support the perf_counter_open syscall either.  Rather than forcing
> >> >> >> >> everyone to add an ignore (or suffer the warning until they get
> >> >> >> >> around to implementing support), only whine about the syscall when
> >> >> >> >> applicable.
> >> >> >> >
> >> >> >> > No, this patch is wrong - it's really easy to add support: just hook
> >> >> >> > up the syscall. This should happen for every architecture really, so
> >> >> >> > the warning is correct and it should not be patched out.
> >> >> >> >
> >> >> >> > PMU support is not required to get perfcounters support: if an
> >> >> >> > architecture hooks up the syscall it will get generic software
> >> >> >> > counters and the tools will work as well.
> >> >> >> >
> >> >> >> > Profiling falls back to a hrtimer-based sampling method - this is a
> >> >> >> > much better fallback than oprofile's fall-back to the timer tick.
> >> >> >> > This hrtimer based sampling is dynticks/nohz-correct and can go
> >> >> >> > beyond HZ if the architecture supports hrtimers.
> >> >> >>
> >> >> >> if there is generic support available, why must every arch select
> >> >> >> HAVE_PERF_COUNTERS in their Kconfig ?
> >> >> >
> >> >> > Because we only want to enable it on architectures that have tested
> >> >> > it. It should only need a syscall addition, but nothing beats having
> >> >> > tested things, hence we have that additional Kconfig symbol.
> >> >>
> >> >> that is a pretty weak reason. [...]
> >> >
> >> > It isnt - this is proper isolation - dont offer something to the
> >> > user to enable that 1) cannot be used due to the lack of a syscall
> >> > 2) has not been tested by anyone on that architecture, ever.
> >> >
> >> > That way say build breakages or runtime failures due to perfcounters
> >> > only become possible on an architecture if the architecture
> >> > maintainer has hooked up the syscall and has provided
> >> > HAVE_PERF_COUNTERS explicitly.
> >>
> >> except that the syscall presence is trivial to detect in the code by
> >> something like:
> >> #ifndef __NR_perf_counter_open
> >> # error sorry, your arch has not hooked up perf_counter_open syscall yet
> >> #endif
> >>
> >> as for "no arch testing yet", there are plenty of drivers which lack
> >> arch depends in the Kconfig specifically so that it can be *easily*
> >> tested on random systems out there without requiring manual twiddling.
> >
> > This is a new kernel subsystem, not just yet another driver.
> 
> so what ?  if it has generic pieces, it is exactly the same as yet 
> another generic driver.  people should be able to randomly test 
> build it when possible to discover latent issues that your testing 
> limited to one arch did not find.
> 
> > Which bit of: "we dont want perfcounters to be enabled in the
> > Kconfig on architectures that have no syscalls and no testing for
> > it" is hard to understand? It is a valid technical concern.
> 
> your (1) is valid but i already pointed out a simple fix for that. 
> your (2) is not.

Uhm, your 'fix':

  #ifndef __NR_perf_counter_open
  # error sorry, your arch has not hooked up perf_counter_open syscall yet
  #endif

is completely unacceptable. We dont propagate build failures via 
user-enable config options, we never did. There's a lot of people 
doing randconfig builds - if it randomly failed due to your 'fix' 
that would upset a lot of testing for no good reason.

	Ingo

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Mike Frysinger]

On Fri, Jun 12, 2009 at 09:09, Ingo Molnar wrote:
> * Mike Frysinger <vapier.adi@gmail.com> wrote:
>> On Fri, Jun 12, 2009 at 08:59, Ingo Molnar wrote:
>> > * Mike Frysinger <vapier.adi@gmail.com> wrote:
>> >> On Fri, Jun 12, 2009 at 08:31, Ingo Molnar wrote:
>> >> > * Mike Frysinger <vapier.adi@gmail.com> wrote:
>> >> >> On Fri, Jun 12, 2009 at 08:17, Ingo Molnar wrote:
>> >> >> > * Mike Frysinger <vapier.adi@gmail.com> wrote:
>> >> >> >> On Fri, Jun 12, 2009 at 08:05, Ingo Molnar wrote:
>> >> >> >> > * Mike Frysinger <vapier@gentoo.org> wrote:
>> >> >> >> >> If the port does not support HAVE_PERF_COUNTERS, then they can't
>> >> >> >> >> support the perf_counter_open syscall either.  Rather than forcing
>> >> >> >> >> everyone to add an ignore (or suffer the warning until they get
>> >> >> >> >> around to implementing support), only whine about the syscall when
>> >> >> >> >> applicable.
>> >> >> >> >
>> >> >> >> > No, this patch is wrong - it's really easy to add support: just hook
>> >> >> >> > up the syscall. This should happen for every architecture really, so
>> >> >> >> > the warning is correct and it should not be patched out.
>> >> >> >> >
>> >> >> >> > PMU support is not required to get perfcounters support: if an
>> >> >> >> > architecture hooks up the syscall it will get generic software
>> >> >> >> > counters and the tools will work as well.
>> >> >> >> >
>> >> >> >> > Profiling falls back to a hrtimer-based sampling method - this is a
>> >> >> >> > much better fallback than oprofile's fall-back to the timer tick.
>> >> >> >> > This hrtimer based sampling is dynticks/nohz-correct and can go
>> >> >> >> > beyond HZ if the architecture supports hrtimers.
>> >> >> >>
>> >> >> >> if there is generic support available, why must every arch select
>> >> >> >> HAVE_PERF_COUNTERS in their Kconfig ?
>> >> >> >
>> >> >> > Because we only want to enable it on architectures that have tested
>> >> >> > it. It should only need a syscall addition, but nothing beats having
>> >> >> > tested things, hence we have that additional Kconfig symbol.
>> >> >>
>> >> >> that is a pretty weak reason. [...]
>> >> >
>> >> > It isnt - this is proper isolation - dont offer something to the
>> >> > user to enable that 1) cannot be used due to the lack of a syscall
>> >> > 2) has not been tested by anyone on that architecture, ever.
>> >> >
>> >> > That way say build breakages or runtime failures due to perfcounters
>> >> > only become possible on an architecture if the architecture
>> >> > maintainer has hooked up the syscall and has provided
>> >> > HAVE_PERF_COUNTERS explicitly.
>> >>
>> >> except that the syscall presence is trivial to detect in the code by
>> >> something like:
>> >> #ifndef __NR_perf_counter_open
>> >> # error sorry, your arch has not hooked up perf_counter_open syscall yet
>> >> #endif
>> >>
>> >> as for "no arch testing yet", there are plenty of drivers which lack
>> >> arch depends in the Kconfig specifically so that it can be *easily*
>> >> tested on random systems out there without requiring manual twiddling.
>> >
>> > This is a new kernel subsystem, not just yet another driver.
>>
>> so what ?  if it has generic pieces, it is exactly the same as yet
>> another generic driver.  people should be able to randomly test
>> build it when possible to discover latent issues that your testing
>> limited to one arch did not find.
>>
>> > Which bit of: "we dont want perfcounters to be enabled in the
>> > Kconfig on architectures that have no syscalls and no testing for
>> > it" is hard to understand? It is a valid technical concern.
>>
>> your (1) is valid but i already pointed out a simple fix for that.
>> your (2) is not.
>
> Uhm, your 'fix':
>
>  #ifndef __NR_perf_counter_open
>  # error sorry, your arch has not hooked up perf_counter_open syscall yet
>  #endif
>
> is completely unacceptable. We dont propagate build failures via
> user-enable config options, we never did. There's a lot of people
> doing randconfig builds - if it randomly failed due to your 'fix'
> that would upset a lot of testing for no good reason.

accept that is a valid bug: the arch is missing the syscall and it
should hook it up
-mike

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Mike Frysinger]

On Fri, Jun 12, 2009 at 08:31, Ingo Molnar wrote:
> That way say build breakages or runtime failures due to perfcounters
> only become possible on an architecture if the architecture
> maintainer has hooked up the syscall

also, this syscall approach is not how any other syscall is handled.
a runtime error along the lines of ENOSYS is trivial to diagnosis and
explain.  you get the exact same behavior on supported architectures
running an older kernel or any kernel with the profiling code
disabled, so any userland code should already be sanely handling this.
-mike

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Ingo Molnar]

* Mike Frysinger <vapier.adi@gmail.com> wrote:

> On Fri, Jun 12, 2009 at 09:09, Ingo Molnar wrote:
> > * Mike Frysinger <vapier.adi@gmail.com> wrote:
> >> On Fri, Jun 12, 2009 at 08:59, Ingo Molnar wrote:
> >> > * Mike Frysinger <vapier.adi@gmail.com> wrote:
> >> >> On Fri, Jun 12, 2009 at 08:31, Ingo Molnar wrote:
> >> >> > * Mike Frysinger <vapier.adi@gmail.com> wrote:
> >> >> >> On Fri, Jun 12, 2009 at 08:17, Ingo Molnar wrote:
> >> >> >> > * Mike Frysinger <vapier.adi@gmail.com> wrote:
> >> >> >> >> On Fri, Jun 12, 2009 at 08:05, Ingo Molnar wrote:
> >> >> >> >> > * Mike Frysinger <vapier@gentoo.org> wrote:
> >> >> >> >> >> If the port does not support HAVE_PERF_COUNTERS, then they can't
> >> >> >> >> >> support the perf_counter_open syscall either.  Rather than forcing
> >> >> >> >> >> everyone to add an ignore (or suffer the warning until they get
> >> >> >> >> >> around to implementing support), only whine about the syscall when
> >> >> >> >> >> applicable.
> >> >> >> >> >
> >> >> >> >> > No, this patch is wrong - it's really easy to add support: just hook
> >> >> >> >> > up the syscall. This should happen for every architecture really, so
> >> >> >> >> > the warning is correct and it should not be patched out.
> >> >> >> >> >
> >> >> >> >> > PMU support is not required to get perfcounters support: if an
> >> >> >> >> > architecture hooks up the syscall it will get generic software
> >> >> >> >> > counters and the tools will work as well.
> >> >> >> >> >
> >> >> >> >> > Profiling falls back to a hrtimer-based sampling method - this is a
> >> >> >> >> > much better fallback than oprofile's fall-back to the timer tick.
> >> >> >> >> > This hrtimer based sampling is dynticks/nohz-correct and can go
> >> >> >> >> > beyond HZ if the architecture supports hrtimers.
> >> >> >> >>
> >> >> >> >> if there is generic support available, why must every arch select
> >> >> >> >> HAVE_PERF_COUNTERS in their Kconfig ?
> >> >> >> >
> >> >> >> > Because we only want to enable it on architectures that have tested
> >> >> >> > it. It should only need a syscall addition, but nothing beats having
> >> >> >> > tested things, hence we have that additional Kconfig symbol.
> >> >> >>
> >> >> >> that is a pretty weak reason. [...]
> >> >> >
> >> >> > It isnt - this is proper isolation - dont offer something to the
> >> >> > user to enable that 1) cannot be used due to the lack of a syscall
> >> >> > 2) has not been tested by anyone on that architecture, ever.
> >> >> >
> >> >> > That way say build breakages or runtime failures due to perfcounters
> >> >> > only become possible on an architecture if the architecture
> >> >> > maintainer has hooked up the syscall and has provided
> >> >> > HAVE_PERF_COUNTERS explicitly.
> >> >>
> >> >> except that the syscall presence is trivial to detect in the code by
> >> >> something like:
> >> >> #ifndef __NR_perf_counter_open
> >> >> # error sorry, your arch has not hooked up perf_counter_open syscall yet
> >> >> #endif
> >> >>
> >> >> as for "no arch testing yet", there are plenty of drivers which lack
> >> >> arch depends in the Kconfig specifically so that it can be *easily*
> >> >> tested on random systems out there without requiring manual twiddling.
> >> >
> >> > This is a new kernel subsystem, not just yet another driver.
> >>
> >> so what ?  if it has generic pieces, it is exactly the same as yet
> >> another generic driver.  people should be able to randomly test
> >> build it when possible to discover latent issues that your testing
> >> limited to one arch did not find.
> >>
> >> > Which bit of: "we dont want perfcounters to be enabled in the
> >> > Kconfig on architectures that have no syscalls and no testing for
> >> > it" is hard to understand? It is a valid technical concern.
> >>
> >> your (1) is valid but i already pointed out a simple fix for that.
> >> your (2) is not.
> >
> > Uhm, your 'fix':
> >
> >  #ifndef __NR_perf_counter_open
> >  # error sorry, your arch has not hooked up perf_counter_open syscall yet
> >  #endif
> >
> > is completely unacceptable. We dont propagate build failures via
> > user-enable config options, we never did. There's a lot of people
> > doing randconfig builds - if it randomly failed due to your 'fix'
> > that would upset a lot of testing for no good reason.
> 
> accept that is a valid bug: the arch is missing the syscall and it 
> should hook it up

Uhm, that's ridiculous, observe lkml for a few weeks and see what 
happens when any subsystem fails to build in a user-configurable 
variation. Even if it's "just" because something like a syscall 
definition is missing.

Anyway, i have no time to teach you about kernel mainteinance basics 
really so i probably wont follow up on future emails.

Thanks,

	Ingo

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Mike Frysinger]

On Fri, Jun 12, 2009 at 08:05, Ingo Molnar wrote:
> * Mike Frysinger <vapier@gentoo.org> wrote:
>> If the port does not support HAVE_PERF_COUNTERS, then they can't
>> support the perf_counter_open syscall either.  Rather than forcing
>> everyone to add an ignore (or suffer the warning until they get
>> around to implementing support), only whine about the syscall when
>> applicable.
>
> No, this patch is wrong - it's really easy to add support: just hook
> up the syscall. This should happen for every architecture really, so
> the warning is correct and it should not be patched out.
>
> PMU support is not required to get perfcounters support: if an
> architecture hooks up the syscall it will get generic software
> counters and the tools will work as well.
>
> Profiling falls back to a hrtimer-based sampling method - this is a
> much better fallback than oprofile's fall-back to the timer tick.
> This hrtimer based sampling is dynticks/nohz-correct and can go
> beyond HZ if the architecture supports hrtimers.

these statements are actually incorrect.  the perf counter code
explicitly requires:
 - asm/perf_counter.h
 - support for atomic64 types (unless i missed something, x86 is the
only 32bit system that supports these)
 - some perf stubs (like set_perf_counter_pending() -- prototype
really should be in common perf_counters headers rather than forcing
the arch to copy & paste the exact same line)

not that any of this is documented ...
-mike

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Ingo Molnar]

* Mike Frysinger <vapier.adi@gmail.com> wrote:

> On Fri, Jun 12, 2009 at 08:05, Ingo Molnar wrote:
> > * Mike Frysinger <vapier@gentoo.org> wrote:
> >> If the port does not support HAVE_PERF_COUNTERS, then they can't
> >> support the perf_counter_open syscall either.  Rather than forcing
> >> everyone to add an ignore (or suffer the warning until they get
> >> around to implementing support), only whine about the syscall when
> >> applicable.
> >
> > No, this patch is wrong - it's really easy to add support: just hook
> > up the syscall. This should happen for every architecture really, so
> > the warning is correct and it should not be patched out.
> >
> > PMU support is not required to get perfcounters support: if an
> > architecture hooks up the syscall it will get generic software
> > counters and the tools will work as well.
> >
> > Profiling falls back to a hrtimer-based sampling method - this is a
> > much better fallback than oprofile's fall-back to the timer tick.
> > This hrtimer based sampling is dynticks/nohz-correct and can go
> > beyond HZ if the architecture supports hrtimers.
> 
> these statements are actually incorrect.  the perf counter code
> explicitly requires:
>  - asm/perf_counter.h

An empty stub suffices.

>  - support for atomic64 types (unless i missed something, x86 is the
> only 32bit system that supports these)

A wrapper suffices - should probably be librarized into lib/.

>  - some perf stubs (like set_perf_counter_pending() -- prototype
> really should be in common perf_counters headers rather than forcing
> the arch to copy & paste the exact same line)

Agreed.

> not that any of this is documented ...

Patches are welcome :-)

You are right that the requirements are not necessarily trivial for 
every arch - so i guess our original patch is correct.

Acked-by: Ingo Molnar <mingo@elte.hu>

	Ingo

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Alan Cox]

> > On Fri, Jun 12, 2009 at 09:09, Ingo Molnar wrote:
> > > * Mike Frysinger <vapier.adi@gmail.com> wrote:
> > >> On Fri, Jun 12, 2009 at 08:59, Ingo Molnar wrote:
> > >> > * Mike Frysinger <vapier.adi@gmail.com> wrote:
> > >> >> On Fri, Jun 12, 2009 at 08:31, Ingo Molnar wrote:
> > >> >> > * Mike Frysinger <vapier.adi@gmail.com> wrote:
> > >> >> >> On Fri, Jun 12, 2009 at 08:17, Ingo Molnar wrote:
> > >> >> >> > * Mike Frysinger <vapier.adi@gmail.com> wrote:
> > >> >> >> >> On Fri, Jun 12, 2009 at 08:05, Ingo Molnar wrote:
> > >> >> >> >> > * Mike Frysinger <vapier@gentoo.org> wrote:
>

> Anyway, i have no time to teach you about kernel mainteinance basics 
> really so i probably wont follow up on future emails.

So yet again you resort to personal attacks when someone objects to the
world according to Ingo. Please stop it, it's not as if you can't find
good technical reasons in many of these cases.

And please both of you - trim the emails you are replying to ..

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Mike Frysinger]

On Fri, Jun 12, 2009 at 11:21, Ingo Molnar wrote:
> * Mike Frysinger <vapier.adi@gmail.com> wrote:
>> the perf counter code explicitly requires:
>> ...
>>  - support for atomic64 types (unless i missed something, x86 is the
>> only 32bit system that supports these)
>
> A wrapper suffices - should probably be librarized into lib/.

wonder if we can trick Arnd into doing that ...

>>  - some perf stubs (like set_perf_counter_pending() -- prototype
>> really should be in common perf_counters headers rather than forcing
>> the arch to copy & paste the exact same line)
>
> Agreed.

can i assume you'll take care of that ?

>> not that any of this is documented ...
>
> Patches are welcome :-)

i cant find anything in Documentation/ covering perf counters.  did i
miss some file ?

also, i was going just by a quick compile test.  dont know what else
is actually required since there's no chance of me trying to get this
to work on Blackfin anytime soon ...

> You are right that the requirements are not necessarily trivial for
> every arch - so i guess our original patch is correct.

there is also some ptrace type piece missing for the Blackfin arch,
but that looks like something that i should just implement as everyone
else is doing it.
-mike

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Ingo Molnar]

* Mike Frysinger <vapier.adi@gmail.com> wrote:

> i cant find anything in Documentation/ covering perf counters.  
> did i miss some file ?

it's in tools/perf/design.txt.

	Ingo

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Ingo Molnar]

* Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:

> > > On Fri, Jun 12, 2009 at 09:09, Ingo Molnar wrote:
> > > > * Mike Frysinger <vapier.adi@gmail.com> wrote:
> > > >> On Fri, Jun 12, 2009 at 08:59, Ingo Molnar wrote:
> > > >> > * Mike Frysinger <vapier.adi@gmail.com> wrote:
> > > >> >> On Fri, Jun 12, 2009 at 08:31, Ingo Molnar wrote:
> > > >> >> > * Mike Frysinger <vapier.adi@gmail.com> wrote:
> > > >> >> >> On Fri, Jun 12, 2009 at 08:17, Ingo Molnar wrote:
> > > >> >> >> > * Mike Frysinger <vapier.adi@gmail.com> wrote:
> > > >> >> >> >> On Fri, Jun 12, 2009 at 08:05, Ingo Molnar wrote:
> > > >> >> >> >> > * Mike Frysinger <vapier@gentoo.org> wrote:
> >
> 
> > Anyway, i have no time to teach you about kernel mainteinance basics 
> > really so i probably wont follow up on future emails.
> 
> So yet again you resort to personal attacks when someone objects to the
> world according to Ingo. [...]

You snipped out the proper context. The thing i object to above was 
a technically completely unacceptable solution proposed by Mike:

|  #ifndef __NR_perf_counter_open
|  # error sorry, your arch has not hooked up perf_counter_open syscall yet
|  #endif
|
| is completely unacceptable. We dont propagate build failures via 
| user-enable config options, we never did. There's a lot of people 
| doing randconfig builds - if it randomly failed due to your 'fix' 
| that would upset a lot of testing for no good reason.

and that opinion still holds. (If you disagree then go try to push 
such a solution upstream, and please share with me the various 
replies you will get, it will be an entertaining read.)

Mike made valid technical points later on though and i replied to 
them and accepted most of them. We can certainly annotate out that 
warning message from scripts/checksyscalls.sh.

	Ingo

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Ingo Molnar]

* Ingo Molnar <mingo@elte.hu> wrote:

> 
> * Mike Frysinger <vapier.adi@gmail.com> wrote:
> 
> > On Fri, Jun 12, 2009 at 09:09, Ingo Molnar wrote:
> > > * Mike Frysinger <vapier.adi@gmail.com> wrote:
> > >> On Fri, Jun 12, 2009 at 08:59, Ingo Molnar wrote:
> > >> > * Mike Frysinger <vapier.adi@gmail.com> wrote:
> > >> >> On Fri, Jun 12, 2009 at 08:31, Ingo Molnar wrote:
> > >> >> > * Mike Frysinger <vapier.adi@gmail.com> wrote:
> > >> >> >> On Fri, Jun 12, 2009 at 08:17, Ingo Molnar wrote:
> > >> >> >> > * Mike Frysinger <vapier.adi@gmail.com> wrote:
> > >> >> >> >> On Fri, Jun 12, 2009 at 08:05, Ingo Molnar wrote:
> > >> >> >> >> > * Mike Frysinger <vapier@gentoo.org> wrote:
> > >> >> >> >> >> If the port does not support HAVE_PERF_COUNTERS, then they can't
> > >> >> >> >> >> support the perf_counter_open syscall either.  Rather than forcing
> > >> >> >> >> >> everyone to add an ignore (or suffer the warning until they get
> > >> >> >> >> >> around to implementing support), only whine about the syscall when
> > >> >> >> >> >> applicable.
> > >> >> >> >> >
> > >> >> >> >> > No, this patch is wrong - it's really easy to add support: just hook
> > >> >> >> >> > up the syscall. This should happen for every architecture really, so
> > >> >> >> >> > the warning is correct and it should not be patched out.
> > >> >> >> >> >
> > >> >> >> >> > PMU support is not required to get perfcounters support: if an
> > >> >> >> >> > architecture hooks up the syscall it will get generic software
> > >> >> >> >> > counters and the tools will work as well.
> > >> >> >> >> >
> > >> >> >> >> > Profiling falls back to a hrtimer-based sampling method - this is a
> > >> >> >> >> > much better fallback than oprofile's fall-back to the timer tick.
> > >> >> >> >> > This hrtimer based sampling is dynticks/nohz-correct and can go
> > >> >> >> >> > beyond HZ if the architecture supports hrtimers.
> > >> >> >> >>
> > >> >> >> >> if there is generic support available, why must every arch select
> > >> >> >> >> HAVE_PERF_COUNTERS in their Kconfig ?
> > >> >> >> >
> > >> >> >> > Because we only want to enable it on architectures that have tested
> > >> >> >> > it. It should only need a syscall addition, but nothing beats having
> > >> >> >> > tested things, hence we have that additional Kconfig symbol.
> > >> >> >>
> > >> >> >> that is a pretty weak reason. [...]
> > >> >> >
> > >> >> > It isnt - this is proper isolation - dont offer something to the
> > >> >> > user to enable that 1) cannot be used due to the lack of a syscall
> > >> >> > 2) has not been tested by anyone on that architecture, ever.
> > >> >> >
> > >> >> > That way say build breakages or runtime failures due to perfcounters
> > >> >> > only become possible on an architecture if the architecture
> > >> >> > maintainer has hooked up the syscall and has provided
> > >> >> > HAVE_PERF_COUNTERS explicitly.
> > >> >>
> > >> >> except that the syscall presence is trivial to detect in the code by
> > >> >> something like:
> > >> >> #ifndef __NR_perf_counter_open
> > >> >> # error sorry, your arch has not hooked up perf_counter_open syscall yet
> > >> >> #endif
> > >> >>
> > >> >> as for "no arch testing yet", there are plenty of drivers which lack
> > >> >> arch depends in the Kconfig specifically so that it can be *easily*
> > >> >> tested on random systems out there without requiring manual twiddling.
> > >> >
> > >> > This is a new kernel subsystem, not just yet another driver.
> > >>
> > >> so what ?  if it has generic pieces, it is exactly the same as yet
> > >> another generic driver.  people should be able to randomly test
> > >> build it when possible to discover latent issues that your testing
> > >> limited to one arch did not find.
> > >>
> > >> > Which bit of: "we dont want perfcounters to be enabled in the
> > >> > Kconfig on architectures that have no syscalls and no testing for
> > >> > it" is hard to understand? It is a valid technical concern.
> > >>
> > >> your (1) is valid but i already pointed out a simple fix for that.
> > >> your (2) is not.
> > >
> > > Uhm, your 'fix':
> > >
> > >  #ifndef __NR_perf_counter_open
> > >  # error sorry, your arch has not hooked up perf_counter_open syscall yet
> > >  #endif
> > >
> > > is completely unacceptable. We dont propagate build failures via
> > > user-enable config options, we never did. There's a lot of people
> > > doing randconfig builds - if it randomly failed due to your 'fix'
> > > that would upset a lot of testing for no good reason.
> > 
> > accept that is a valid bug: the arch is missing the syscall and it 
> > should hook it up
> 
> Uhm, that's ridiculous, observe lkml for a few weeks and see what 
> happens when any subsystem fails to build in a user-configurable 
> variation. Even if it's "just" because something like a syscall 
> definition is missing.
> 
> Anyway, i have no time to teach you about kernel mainteinance 
> basics really so i probably wont follow up on future emails.

Mike, i'd like to apologize for the tone of this reply - it was 
certainly over the top, especially considering the fact that you 
were right with your original patch :)

Merge window stress is not making me particularly patient i guess
:-/

	Ingo

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Mike Frysinger]

On Fri, Jun 12, 2009 at 12:57, Ingo Molnar wrote:
> Mike, i'd like to apologize for the tone of this reply - it was
> certainly over the top, especially considering the fact that you
> were right with your original patch :)
>
> Merge window stress is not making me particularly patient i guess
> :-/

all is copacetic
-mike

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Paul Mackerras]

Ingo Molnar writes:

> PMU support is not required to get perfcounters support: if an 
> architecture hooks up the syscall it will get generic software 
> counters and the tools will work as well.

It looks to me that to work properly, each architecture has to provide
set_perf_counter_pending() and some way to arrange to call
perf_counter_do_pending() once interrupts get re-enabled.  We need
this because some software counters (the context-switch counter at
least, as well as all the tracepoint counters) use nmi = 1 because
their events happen at times when we can't do a wakeup.

Actually, as far as the tracepoints are concerned, I can't see any
reason to be sure that interrupts will be disabled when a tracepoint
event occurs.  On powerpc, calling set_perf_counter_pending() when
interrupts are enabled won't result in perf_counter_do_pending()
getting called right away; the perf_counter_do_pending() call won't
happen until something disables and re-enables interrupts.

Paul.

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Mike Frysinger]

On Fri, Jun 12, 2009 at 07:29, Mike Frysinger wrote:
> If the port does not support HAVE_PERF_COUNTERS, then they can't support
> the perf_counter_open syscall either.  Rather than forcing everyone to add
> an ignore (or suffer the warning until they get around to implementing
> support), only whine about the syscall when applicable.
>
> Signed-off-by: Mike Frysinger <vapier@gentoo.org>

Andrew: could you pick this up since Ingo acked it now ?
-mike

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Ingo Molnar]

* Mike Frysinger <vapier.adi@gmail.com> wrote:

> On Fri, Jun 12, 2009 at 11:21, Ingo Molnar wrote:
> > * Mike Frysinger <vapier.adi@gmail.com> wrote:
> >> the perf counter code explicitly requires:
> >> ...
> >>  - support for atomic64 types (unless i missed something, x86 is the
> >> only 32bit system that supports these)
> >
> > A wrapper suffices - should probably be librarized into lib/.
> 
> wonder if we can trick Arnd into doing that ...

FYI, Paul has posted the patches for generic atomic64_t today.

So the barrier of entry to perfcounters enablement is really low 
now, even for 32-bit architectures without 64-bit atomic ops.

	Ingo

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Paul Mundt]

On Sat, Jun 13, 2009 at 06:48:52AM -0400, Mike Frysinger wrote:
> On Fri, Jun 12, 2009 at 07:29, Mike Frysinger wrote:
> > If the port does not support HAVE_PERF_COUNTERS, then they can't support
> > the perf_counter_open syscall either. ??Rather than forcing everyone to add
> > an ignore (or suffer the warning until they get around to implementing
> > support), only whine about the syscall when applicable.
> >
> > Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> 
> Andrew: could you pick this up since Ingo acked it now ?

I fail to see why this is necessary? cond_syscall() takes care of this in
the not implemented case, the same as every other syscall backing some
feature that has yet to be implemented.

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Mike Frysinger]

On Sun, Jun 14, 2009 at 05:37, Paul Mundt wrote:
> On Sat, Jun 13, 2009 at 06:48:52AM -0400, Mike Frysinger wrote:
>> On Fri, Jun 12, 2009 at 07:29, Mike Frysinger wrote:
>> > If the port does not support HAVE_PERF_COUNTERS, then they can't support
>> > the perf_counter_open syscall either. ??Rather than forcing everyone to add
>> > an ignore (or suffer the warning until they get around to implementing
>> > support), only whine about the syscall when applicable.
>> >
>> > Signed-off-by: Mike Frysinger <vapier@gentoo.org>
>>
>> Andrew: could you pick this up since Ingo acked it now ?

btw, Sam said he would pick it up via the kbuild tree

> I fail to see why this is necessary? cond_syscall() takes care of this in
> the not implemented case, the same as every other syscall backing some
> feature that has yet to be implemented.

i dont think we should go hassling every arch maintainer when a new
syscall is added that requires arch-specific support for optional
features (especially when said features are debug in nature).  if
wiring up the syscall is the only work because the code is all common
(like the pread/pwrite functions), then np of course.
-mike

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Paul Mundt]

On Sun, Jun 14, 2009 at 05:55:44AM -0400, Mike Frysinger wrote:
> On Sun, Jun 14, 2009 at 05:37, Paul Mundt wrote:
> > On Sat, Jun 13, 2009 at 06:48:52AM -0400, Mike Frysinger wrote:
> >> On Fri, Jun 12, 2009 at 07:29, Mike Frysinger wrote:
> >> > If the port does not support HAVE_PERF_COUNTERS, then they can't support
> >> > the perf_counter_open syscall either. ??Rather than forcing everyone to add
> >> > an ignore (or suffer the warning until they get around to implementing
> >> > support), only whine about the syscall when applicable.
> >> >
> >> > Signed-off-by: Mike Frysinger <vapier@gentoo.org>
> >>
> >> Andrew: could you pick this up since Ingo acked it now ?
> 
> btw, Sam said he would pick it up via the kbuild tree
> 
> > I fail to see why this is necessary? cond_syscall() takes care of this in
> > the not implemented case, the same as every other syscall backing some
> > feature that has yet to be implemented.
> 
> i dont think we should go hassling every arch maintainer when a new
> syscall is added that requires arch-specific support for optional
> features (especially when said features are debug in nature).  if
> wiring up the syscall is the only work because the code is all common
> (like the pread/pwrite functions), then np of course.

Perhaps not, but I do prefer to have the script whine at me when a new
syscall pops up, just so I know when I have to start caring about a new
feature. New syscalls that are handled by cond_syscall() are trivially
dropped in to the syscall table to get rid of these warnings, regardless
of whether you have any intention of really supporting the feature or
not. If a generic implementation becomes available, then it can be
supported without having to backtrack and update place-holders.

These are not things I want to see silenced just because you don't
presently feel compelled to wire up the entry on your platform.

The fact the perf counter stuff has no real generic support, or even the
present infrastructure to support it on pretty much every 32-bit platform
that isn't x86 is more an issue with -tip development methodology than
anything to do with the syscall bits.

Of course if it had been handled properly then the generic software
counters would have been actually implemented generically and
subsequently made available from the stub and the HAVE_xxx would be
reserved for architecture-specific counters. Unfortunately these days
"generic" generally seems to imply "can be made generic if someone else
bothers to actually do the work, assuming they can find any documentation
in the first place".

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Mike Frysinger]

On Sun, Jun 14, 2009 at 06:11, Paul Mundt wrote:
> On Sun, Jun 14, 2009 at 05:55:44AM -0400, Mike Frysinger wrote:
>> On Sun, Jun 14, 2009 at 05:37, Paul Mundt wrote:
>> >> On Fri, Jun 12, 2009 at 07:29, Mike Frysinger wrote:
>> >> If the port does not support HAVE_PERF_COUNTERS, then they can't support
>> >> the perf_counter_open syscall either. ??Rather than forcing everyone to add
>> >> an ignore (or suffer the warning until they get around to implementing
>> >> support), only whine about the syscall when applicable.
>> >
>> > I fail to see why this is necessary? cond_syscall() takes care of this in
>> > the not implemented case, the same as every other syscall backing some
>> > feature that has yet to be implemented.
>>
>> i dont think we should go hassling every arch maintainer when a new
>> syscall is added that requires arch-specific support for optional
>> features (especially when said features are debug in nature).  if
>> wiring up the syscall is the only work because the code is all common
>> (like the pread/pwrite functions), then np of course.
>
> Perhaps not, but I do prefer to have the script whine at me when a new
> syscall pops up, just so I know when I have to start caring about a new
> feature.

assuming you can find any useful info about said feature ;)

> If a generic implementation becomes available, then it can be
> supported without having to backtrack and update place-holders.

this is a good convincing point.  Sam: please drop this patch if you
did get a chance to queue it up.

> These are not things I want to see silenced just because you don't
> presently feel compelled to wire up the entry on your platform.

i disagree, but i guess it doesnt matter if the arch maintainers dont
all agree here.

> Of course if it had been handled properly then the generic software
> counters would have been actually implemented generically and
> subsequently made available from the stub and the HAVE_xxx would be
> reserved for architecture-specific counters. Unfortunately these days
> "generic" generally seems to imply "can be made generic if someone else
> bothers to actually do the work, assuming they can find any documentation
> in the first place".

if we can agree on centralizing arch/Kconfig for HAVE_xxx stubs, we
could add a checkpatch that errors out if said stub lacks a help line
-- the assumption being the help text would point to arch details and
not some other document (design rational, user interface, methodology,
etc...).  of course, this also relies on the assumption of people
filtering via checkpatch.pl ...
-mike

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Paul Mundt]

On Sun, Jun 14, 2009 at 06:55:45AM -0400, Mike Frysinger wrote:
> On Sun, Jun 14, 2009 at 06:11, Paul Mundt wrote:
> > Perhaps not, but I do prefer to have the script whine at me when a new
> > syscall pops up, just so I know when I have to start caring about a new
> > feature.
> 
> assuming you can find any useful info about said feature ;)
> 
> > These are not things I want to see silenced just because you don't
> > presently feel compelled to wire up the entry on your platform.
> 
> i disagree, but i guess it doesnt matter if the arch maintainers dont
> all agree here.
> 
That is a secondary issue. For most people the script whining about an
unimplemented syscall is the first indication that anything new has been
added that needs to be looked at. This has been an invaluable addition
and I will oppose any attempts to restrict that functionality. If it
whines, you do need to look at it, whether you care about supporting it
or not. This script whines for everyone and requires action by every
architecture maintainer, and simply disabling it in the script because
you don't care about it is not helpful.

As far as new features and syscalls go, documentation is the exception,
rather than the norm. And this is really not an issue, so long as a
the generic implementation exists and does something useful. You have
some HAVE_xxx stub for when you have some degree of support beyond what
it can handle generically.

This distinction is only blurry in the perf counter subsystem because no
one bothered devising a generic implementation for it, instead suggesting
that platforms need to do special enabling logic for something that
should theoretically be generic. This is a total departure from what the
HAVE_xx symbols have meant in the past, and I don't like the fact that
this just happened silently without any discussion. But this also seems
to be a pretty common occurence for things coming in from -tip when
applied in a non-x86 context.

> if we can agree on centralizing arch/Kconfig for HAVE_xxx stubs, we
> could add a checkpatch that errors out if said stub lacks a help line
> -- the assumption being the help text would point to arch details and
> not some other document (design rational, user interface, methodology,
> etc...).  of course, this also relies on the assumption of people
> filtering via checkpatch.pl ...

arch/Kconfig exists for architecture-specific HAVE_xxx stubs, and ought
to have some mention of where to look for wiring up said stub. Individual
features that may or may not be supported by the entire architecture are
best off defined within the Kconfig they are used in.

Having to hunt down new HAVE_xxx stubs is almost as tedious as having to
hunt down the required support interface that goes along with them. In
practice I generally just select HAVE_foo and see where the build blows
up to figure out what I have to do first. But this of course assumes you
have some visibility of new HAVE_xxx stubs.

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Sam Ravnborg]

On Sun, Jun 14, 2009 at 06:55:45AM -0400, Mike Frysinger wrote:
> On Sun, Jun 14, 2009 at 06:11, Paul Mundt wrote:
> > On Sun, Jun 14, 2009 at 05:55:44AM -0400, Mike Frysinger wrote:
> >> On Sun, Jun 14, 2009 at 05:37, Paul Mundt wrote:
> >> >> On Fri, Jun 12, 2009 at 07:29, Mike Frysinger wrote:
> >> >> If the port does not support HAVE_PERF_COUNTERS, then they can't support
> >> >> the perf_counter_open syscall either. ??Rather than forcing everyone to add
> >> >> an ignore (or suffer the warning until they get around to implementing
> >> >> support), only whine about the syscall when applicable.
> >> >
> >> > I fail to see why this is necessary? cond_syscall() takes care of this in
> >> > the not implemented case, the same as every other syscall backing some
> >> > feature that has yet to be implemented.
> >>
> >> i dont think we should go hassling every arch maintainer when a new
> >> syscall is added that requires arch-specific support for optional
> >> features (especially when said features are debug in nature).  if
> >> wiring up the syscall is the only work because the code is all common
> >> (like the pread/pwrite functions), then np of course.
> >
> > Perhaps not, but I do prefer to have the script whine at me when a new
> > syscall pops up, just so I know when I have to start caring about a new
> > feature.
> 
> assuming you can find any useful info about said feature ;)
> 
> > If a generic implementation becomes available, then it can be
> > supported without having to backtrack and update place-holders.
> 
> this is a good convincing point.  Sam: please drop this patch if you
> did get a chance to queue it up.

OK - dropped.

	Sam

Re: [PATCH] scripts/checksyscalls.sh: only whine perf_counter_open when supported

[Mike Frysinger]

On Sun, Jun 14, 2009 at 07:20, Paul Mundt wrote:
> On Sun, Jun 14, 2009 at 06:55:45AM -0400, Mike Frysinger wrote:
>> if we can agree on centralizing arch/Kconfig for HAVE_xxx stubs, we
>> could add a checkpatch that errors out if said stub lacks a help line
>> -- the assumption being the help text would point to arch details and
>> not some other document (design rational, user interface, methodology,
>> etc...).  of course, this also relies on the assumption of people
>> filtering via checkpatch.pl ...
>
> arch/Kconfig exists for architecture-specific HAVE_xxx stubs, and ought
> to have some mention of where to look for wiring up said stub. Individual
> features that may or may not be supported by the entire architecture are
> best off defined within the Kconfig they are used in.

off the top of my head, i cant think of any HAVE_xxx stub that doesnt
require some level of arch support.  and since these are things arch
people should be looking at, having them all in arch/Kconfig makes
sense.  although atm, that file seems to also have oprofile options
... but considering what a mess oprofile is in terms of integration
with any piece of the build system, perhaps we should ignore it and
heed it as a warning of how to not do things.

scanning the myriad of options out there shows some variety that
should probably be condensed.  for example, we have:
 - HAVE_xxx - the arch has support for xxx
 - GENERIC_xxx - dual meaning here -- generic support for xxx exists
if you want it, but sometimes it requires the arch to have support for
some piece of xxx first
 - xxx_SUPPORT - the arch has support for xxx
 - ARCH_WANT_xxx - feature xxx exists, iff the arch has support for it
and wants it

i cant think of any place where xxx_SUPPORT shouldnt actually be HAVE_xxx

> Having to hunt down new HAVE_xxx stubs is almost as tedious as having to
> hunt down the required support interface that goes along with them. In
> practice I generally just select HAVE_foo and see where the build blows
> up to figure out what I have to do first. But this of course assumes you
> have some visibility of new HAVE_xxx stubs.

you can search for it with some kconfig interfaces, but in general it
does suck currently.
-mike