Re: + perfmon2-reserve-system-calls.patch added to -mm tree

Re: + perfmon2-reserve-system-calls.patch added to -mm tree

[Arnd Bergmann]

On Dinsdag 15 November 2005 00:30, akpm@osdl.org wrote:
> diff -puN include/asm-powerpc/unistd.h~perfmon2-reserve-system-calls include/asm-powerpc/unistd.h
> --- 25/include/asm-powerpc/unistd.h~perfmon2-reserve-system-calls       Mon Nov 14 15:27:32 2005
> +++ 25-akpm/include/asm-powerpc/unistd.h        Mon Nov 14 15:27:32 2005
> @@ -296,8 +296,20 @@
>  #define __NR_inotify_init      275
>  #define __NR_inotify_add_watch 276
>  #define __NR_inotify_rm_watch  277
> +#define __NR_pfm_create_context        278
> +#define __NR_pfm_write_pmcs    279
> +#define __NR_pfm_write_pmds    280
> +#define __NR_pfm_read_pmds     281
> +#define __NR_pfm_load_context  282
> +#define __NR_pfm_start         283
> +#define __NR_pfm_stop          284
> +#define __NR_pfm_restart       285
> +#define __NR_pfm_create_evtsets        286
> +#define __NR_pfm_getinfo_evtsets 287
> +#define __NR_pfm_delete_evtsets 288
> +#define __NR_pfm_unload_context        289
>  
> -#define __NR_syscalls          278
> +#define __NR_syscalls          290
>  
>  #ifdef __KERNEL__
>  #define __NR__exit __NR_exit

Hmm, I had sent an earlier patch to paulus that reserves 278 and
279 for spu_run and spu_create and that apparently got dropped.

Could I have those two numbers or is there already an established
user based for the perfmon2 numbers that would take preference?

	Arnd <><

Re: + perfmon2-reserve-system-calls.patch added to -mm tree

[Paul Mackerras]

Arnd Bergmann writes:

> Hmm, I had sent an earlier patch to paulus that reserves 278 and
> 279 for spu_run and spu_create and that apparently got dropped.

Hmmm, sorry, I thought I had put that in, but evidently not.

> Could I have those two numbers or is there already an established
> user based for the perfmon2 numbers that would take preference?

I don't believe there is any user base of the perfmon2 numbers.
Stephane, could you respin using 280 - 291 for PowerPC?

Thanks,
Paul.

Re: + perfmon2-reserve-system-calls.patch added to -mm tree

[Stephane Eranian]

Paul,

On Tue, Nov 15, 2005 at 11:04:19AM +1100, Paul Mackerras wrote:
> Arnd Bergmann writes:
> 
> > Hmm, I had sent an earlier patch to paulus that reserves 278 and
> > 279 for spu_run and spu_create and that apparently got dropped.
> 
> Hmmm, sorry, I thought I had put that in, but evidently not.
> 
> > Could I have those two numbers or is there already an established
> > user based for the perfmon2 numbers that would take preference?
> 
> I don't believe there is any user base of the perfmon2 numbers.
> Stephane, could you respin using 280 - 291 for PowerPC?
> 
Yes, at this point I am just reserving the syscall numbers, nothing is
hardcoded into any application, so we can certainly change it. I'll do
that and resubmit the powerpc portion. From Andrew, I understand I have
to do this in arch/powerpc and also in arch/ppc.

-- 
-Stephane

Re: + perfmon2-reserve-system-calls.patch added to -mm tree

[Andrew Morton]

Arnd Bergmann <arnd@arndb.de> wrote:
>
> >  #ifdef __KERNEL__
> >  #define __NR__exit __NR_exit
> 
> Hmm, I had sent an earlier patch to paulus that reserves 278 and
> 279 for spu_run and spu_create and that apparently got dropped.
> 
> Could I have those two numbers or is there already an established
> user based for the perfmon2 numbers that would take preference?

aargh.  Any time anyone dinks with the syscall tables I have tons of fun
fixing up rejects.  It doesn't help that both Stephane and Christoph's
patches were fairly broken.

Rules:

a) Keep unistd.h and the syscall tables in sync.

b) Keep ppc32 and powerpc[64] in sync

c) Add prototypes to syscalls.h (When the implementation goes in -
   obviously not relevant when we're just reserving syscall slots)

d) Some architectures have multiple syscall tables.  Stephane, you
   missed arch/ia64/ia32/ia32_entry.S, for example.  But then, that looks
   to be seriously out of date anyway.  No idea what's going on there.

e) review your work carefully.  Grep the tree for, say, `getxattr' (or
   any other syscall name which is unique-looking and which you expect all
   architectures to implement).

Anyway, I have a shower of fixup patches here.   Hopefully it all landed OK.

Re: + perfmon2-reserve-system-calls.patch added to -mm tree

[Stephane Eranian]

Andrew,

On Mon, Nov 14, 2005 at 05:12:26PM -0800, Andrew Morton wrote:
> aargh.  Any time anyone dinks with the syscall tables I have tons of fun
> fixing up rejects.  It doesn't help that both Stephane and Christoph's
> patches were fairly broken.
> 
> Rules:
> 
> a) Keep unistd.h and the syscall tables in sync.
> 
> b) Keep ppc32 and powerpc[64] in sync
> 
> c) Add prototypes to syscalls.h (When the implementation goes in -
>    obviously not relevant when we're just reserving syscall slots)
> 
> d) Some architectures have multiple syscall tables.  Stephane, you
>    missed arch/ia64/ia32/ia32_entry.S, for example.  But then, that looks
>    to be seriously out of date anyway.  No idea what's going on there.
> 

This one corresponds to the system call table for X86 binaries running
on Linux/ia64. This is used in combination with the hardware emulation
in the processor. This is being replaced by full user level translation
by Intel's IA32el libraries. The x86 binary is translated to IA-64 native
code in user mode and then it makes IA-64 system calls. As such, the IA-32
syscall emulation table is becoming obsolete as people upgrade to newer 
Linux distributions using IA32el. Tony correct me if I missed something.

> e) review your work carefully.  Grep the tree for, say, `getxattr' (or
>    any other syscall name which is unique-looking and which you expect all
>    architectures to implement).
> 
> Anyway, I have a shower of fixup patches here.   Hopefully it all landed OK.

I apologize for generating some extra work for you. I saw you fixed the powerpc
request for those two extra syscalls. Thanks.

-- 
-Stephane

Re: + perfmon2-reserve-system-calls.patch added to -mm tree

[Arjan van de Ven]

> > d) Some architectures have multiple syscall tables.  Stephane, you
> >    missed arch/ia64/ia32/ia32_entry.S, for example.  But then, that looks
> >    to be seriously out of date anyway.  No idea what's going on there.
> > 
> 
> This one corresponds to the system call table for X86 binaries running
> on Linux/ia64. This is used in combination with the hardware emulation
> in the processor. This is being replaced by full user level translation
> by Intel's IA32el libraries. The x86 binary is translated to IA-64 native
> code in user mode and then it makes IA-64 system calls. As such, the IA-32
> syscall emulation table is becoming obsolete as people upgrade to newer 
> Linux distributions using IA32el. Tony correct me if I missed something.

... which is a binary only, proprietary application.

Either way, either the emulation is in the kernel or it's not. If it's
there (like it is now) it deserves maintenance. If it's not, it should
be removed from the tree, since the only thing it's otherwise good for
is potential security holes.

Re: + perfmon2-reserve-system-calls.patch added to -mm tree

[Christoph Hellwig]

On Tue, Nov 15, 2005 at 12:50:26AM +0100, Arnd Bergmann wrote:
> > --- 25/include/asm-powerpc/unistd.h~perfmon2-reserve-system-calls???????Mon Nov 14 15:27:32 2005
> > +++ 25-akpm/include/asm-powerpc/unistd.h????????Mon Nov 14 15:27:32 2005
> > @@ -296,8 +296,20 @@
> > ?#define __NR_inotify_init??????275
> > ?#define __NR_inotify_add_watch?276
> > ?#define __NR_inotify_rm_watch??277
> > +#define __NR_pfm_create_context????????278
> > +#define __NR_pfm_write_pmcs????279
> > +#define __NR_pfm_write_pmds????280
> > +#define __NR_pfm_read_pmds?????281
> > +#define __NR_pfm_load_context??282
> > +#define __NR_pfm_start?????????283
> > +#define __NR_pfm_stop??????????284
> > +#define __NR_pfm_restart???????285
> > +#define __NR_pfm_create_evtsets????????286
> > +#define __NR_pfm_getinfo_evtsets 287
> > +#define __NR_pfm_delete_evtsets 288
> > +#define __NR_pfm_unload_context????????289
> > ?
> > -#define __NR_syscalls??????????278
> > +#define __NR_syscalls??????????290

I thought we didn't reserve syscall numbers?

anyway, this is an awfull lot of syscalls numbers for what essentially
is a driver not core kernel functionality.  I think we should do an API
review first.

and why didn't this patch get sent to lkml for review?

Re: + perfmon2-reserve-system-calls.patch added to -mm tree

[Andi Kleen]

> ... which is a binary only, proprietary application.

The IA32 emulation part of it is actually shipped in source. 
Somehow they manage to link a binary only object to it though.

> 
> Either way, either the emulation is in the kernel or it's not. If it's
> there (like it is now) it deserves maintenance. If it's not, it should
> be removed from the tree, since the only thing it's otherwise good for
> is potential security holes.

I suppose it's still useful for all current IA64 users (Montecito
is not shipping yet and older CPUs support x86 in hardware) who don't like 
binary only software.

-Andi
> 

RE: + perfmon2-reserve-system-calls.patch added to -mm tree

[Luck, Tony]

>> ... which is a binary only, proprietary application.
>
>The IA32 emulation part of it is actually shipped in source. 
>Somehow they manage to link a binary only object to it though.

There are two parts to the ia32 emulation ... the part that handles
all the Linux syscall emulation is open (LGPL I think).  The part
that handles translation of x86 instructions into ia64 instructions
is the binary only part ... a shared library that gets attached by
the Linux layer ... this same binary blob is used on all operating
systems.

>> Either way, either the emulation is in the kernel or it's not. If it's
>> there (like it is now) it deserves maintenance. If it's not, it should
>> be removed from the tree, since the only thing it's otherwise good for
>> is potential security holes.
>
>I suppose it's still useful for all current IA64 users (Montecito
>is not shipping yet and older CPUs support x86 in hardware) who don't like 
>binary only software.

I was planning on asking who still depends on the emulation code
a while after Montecito is shipping.  Until then I'll try to do
what makes sense in keeping the ia32 emulation system call table
up to date.

The perfmon syscalls would be an example of something that should
*NOT* go into the ia32 emulation syscall table.  It makes no sense
whatever to put them there.  I don't believe that the h/w emulation
provides any performance counter emulation, and even if it did a
user who cared about the performance of their application would do
far better to re-compile it as native ia64 than to mess around
trying to optimize their x86 binary.

-Tony

Re: + perfmon2-reserve-system-calls.patch added to -mm tree

[Stephane Eranian]

tony,

On Tue, Nov 15, 2005 at 07:28:35AM -0800, Luck, Tony wrote:
> 
> >> Either way, either the emulation is in the kernel or it's not. If it's
> >> there (like it is now) it deserves maintenance. If it's not, it should
> >> be removed from the tree, since the only thing it's otherwise good for
> >> is potential security holes.
> >
> >I suppose it's still useful for all current IA64 users (Montecito
> >is not shipping yet and older CPUs support x86 in hardware) who don't like 
> >binary only software.
> 
> I was planning on asking who still depends on the emulation code
> a while after Montecito is shipping.  Until then I'll try to do
> what makes sense in keeping the ia32 emulation system call table
> up to date.
> 
> The perfmon syscalls would be an example of something that should
> *NOT* go into the ia32 emulation syscall table.  It makes no sense
> whatever to put them there.  I don't believe that the h/w emulation
> provides any performance counter emulation, and even if it did a
> user who cared about the performance of their application would do
> far better to re-compile it as native ia64 than to mess around
> trying to optimize their x86 binary.
> 
I agree. The PMU between, let's a P4, and IA-64 is totally
different. It does not make sense to try and emulate the P4
PMU on an Ia-64 system. Similarly, it  does not make sense
to develop a monitoring tool for IA-64 compiled in X86 mode
and relying on the emulation layer. 

The only thing we need to ensure is that those system calls
don't do anything, i.e., if ever invoked in IA-32 HW emulation
they would need to go to sys_ni().

-- 

-Stephane

RE: + perfmon2-reserve-system-calls.patch added to -mm tree

[Luck, Tony]

It does look like I'm a bit behind with syscalls in the
32-bit emulation table.  Ones that may need to be added are:

274  sys_mbind
275  sys_get_mempolicy
276  sys_set_mempolicy
286  sys_add_key
287  sys_request_key
288  sys_keyctl
289  sys_ioprio_set
290  sys_ioprio_get
291  sys_inotify_init
292  sys_inotify_add_watch
293  sys_inotify_rm_watch

Plus a general whitespace cleanup (lots of lines start with
<space>+<tab>).

-Tony

Re: + perfmon2-reserve-system-calls.patch added to -mm tree

[Stephane Eranian]

Christoph,

On Tue, Nov 15, 2005 at 09:06:12AM +0000, Christoph Hellwig wrote:
> On Tue, Nov 15, 2005 at 12:50:26AM +0100, Arnd Bergmann wrote:
> > > --- 25/include/asm-powerpc/unistd.h~perfmon2-reserve-system-calls???????Mon Nov 14 15:27:32 2005
> > > +++ 25-akpm/include/asm-powerpc/unistd.h????????Mon Nov 14 15:27:32 2005
> > > @@ -296,8 +296,20 @@
> > > ?#define __NR_inotify_init??????275
> > > ?#define __NR_inotify_add_watch?276
> > > ?#define __NR_inotify_rm_watch??277
> > > +#define __NR_pfm_create_context????????278
> > > +#define __NR_pfm_write_pmcs????279
> > > +#define __NR_pfm_write_pmds????280
> > > +#define __NR_pfm_read_pmds?????281
> > > +#define __NR_pfm_load_context??282
> > > +#define __NR_pfm_start?????????283
> > > +#define __NR_pfm_stop??????????284
> > > +#define __NR_pfm_restart???????285
> > > +#define __NR_pfm_create_evtsets????????286
> > > +#define __NR_pfm_getinfo_evtsets 287
> > > +#define __NR_pfm_delete_evtsets 288
> > > +#define __NR_pfm_unload_context????????289
> > > ?
> > > -#define __NR_syscalls??????????278
> > > +#define __NR_syscalls??????????290
> 
> 
> anyway, this is an awfull lot of syscalls numbers for what essentially
> is a driver not core kernel functionality.  I think we should do an API
> review first.

I have cut the number of system calls as much as I could. Initially
this started out as a single multiplexing system call. Given the feedback
I received, I switched it over to multiple system calls. In retrospect,
I think this is better than the initial design.

As for your driver question, it is true that if we were only interested in
accessing the performance counters on a system-wide basis, like OProfile,
we could have implemented a device driver.

However we want both the ability to measure on a system-wide basis, i.e.,
across all kernel threads running on a processor, but also on a per kernel thread
basis. Both modes are important. Certain measurements do need to be applied
to a single thread, e.g., counting the number of retired instructions for /bin/ls.

The per-thread mode implies that, on context switch, the counters be saved
and restored like the rest of the machine state. I don't believe it is allowed
for a device driver to access to the context switch code.

Furthermore we want to have the monitoring interface be builtin to make it easier
for tools developers. A system call interface also gives us more flexibility
as to how the interface looks like, i.e., number and types of arguments.

Fro all those reasons, we felt that the syscall approach was the best.


> and why didn't this patch get sent to lkml for review?

For the last few months, I have been posting announces on lkml, perfctr-devel
and also linux-ia64-kernel about the new releases of the multi-platform perfmon
code base to sollicate feedback. I don't claim to know all the kernel internal
semantics and interfaces nor each hardware platofrm intricacies. Thus I welcome
any comments, complaints especially if they are constructive and poinpoint precise
problems in the code. This way I think we all make forward progress.

I will be releasing a new version of the patch this week. I would certainly welcome
your comments.

-- 
-Stephane