perfmon3 interface overview

perfmon3 interface overview

[stephane eranian]

Hello,

A few months ago, I started posting on this list a highly simplified version
of the perfmon2 version which was providing only per-thread counting on X86
processors.

The feedback I got was generally positive but people raised two more issues:

    - too many system calls for a single subsystem (12 calls)

    - how to ensure syscalls could be extended without necessarily
      adding new ones.


Since then, I have been working hard on addressing those two issues, in other
words redesign of the perfmon2 syscall API. This message is to announce that I
now have a new proposal for the API. As expected it is called perfmon3 and
it addresses the two issues above while allowing backward compatibility with the
existing v2.81 version through a user level glue layer which could be
implemented
by a library such as libpfm.


The new API now has 8 system calls in its fully-featured version. Many
data structures
shared with user level have been abandoned in favor of explicit
syscall parameters.
Each syscall has a flags parameters which allows the syscalls to be
extended with
new parameters when we need them. Most structures passed to the kernel have
reserved fields for future extensions.

The initial patchset will only support per-thread counting as I did
previously. However,
here I am presenting the details for the fully featured version so
people can get a feel
for it. For each call I show the old way and the new way.

Note that when the syscall is shown twice with a different number of
parameters, this
variation is handled by the user library. The kernel implements the
full call. This is
the same technique used for the open(2) syscall.

 I) session creation

 With v2.81:
    int pfm_create_context(pfarg_ctx_t *ctx, char *smpl_name, void
                                          *smpl_arg, size_t smpl_size);

 With v3.0:
    int pfm_create_session(int flags);
    int pfm_create_session(int flags, char *smpl_name,
                                          void *smpl_arg, size_t smpl_size);

    New Flags:
         PFM_FL_SMPL_FMT : indicate using sampling format and
                                               that 3 additional
parameters are passed

 The pfarg_ctx_t structure has been abandoned. The flags parameter is
 used very much like for the open(2) syscall to indicate that additional
 (optional) parameters are passed.

  All v2.81 flags are preserved.

  The call still returns the file descriptor uniquely identifying the session.

  Just like with context, a session can either be monitoring a thread or a CPU.

 II) programming the registers

 With v2.81:
      int pfm_write_pmcs(int fd, pfarg_pmc_t *pmds, int  n);
      int pfm_write_pmds(int fd, pfarg_pmd_t *pmcs, int n);
      int pfm_read_pmds(int fd, parg_pmd_t *pmds, int n);

 With v3.0:
      int pfm_write_pmrs(int fd, int flags, pfarg_pmr_t *pmrs, int n);
      int pfm_write_pmrs(int fd, int flags, pfarg_pmr_t *pmrs, int n,
                                      pfarg_pmd_attr_t *pmas);

      int pfm_read_pmrs(int fd, int flags, pfarg_pmr_t *pmrs, int n);
      int pfm_read_pmrs(int fd, int flags, parg_pmr_t *pmrs, int n,
                                     pfarg_pmd_attr_t *pmas);

 New structures:

   typedef struct {
       u16 reg_num;
       u16 reg_set;
       u32 reg_flags;
       u64 reg_value;
   } pfarg_pmr_t;

   typedef struct {
       u64 reg_long_reset;
       u64 reg_short_reset;
       u64 reg_random_mask;
       u64 reg_smpl_pmds[PFM_PMD_BV];
       u64 reg_reset_pmds[PFM_PMD_BV];
       u64 reg_ovfl_swcnt;
       u64 reg_smpl_eventid;
       u64 reg_last_value;
       u64 reg_reserved[8];
   } pfarg_pmd_t;

 New flags:
    PFM_RWFL_PMD  : pmrs contains PMD register descriptions
    PFM_RWFL_PMC  : pmrs contains PMC register descriptions
    PFM_RWFL_PMD_ATTR: PFM_RWFL_PMD + attributes

 We now use only 2 system calls to read and write the PMU registers.
 This is possible because we are sharing the same register description
 data structure, pfarg_pmr_t. They key attributes of each register are
 encapsulated into this structure. Additional PMD attributes related to
 sampling and multiplexing are off-loaded into another optional structure,
 pfarg_pmd_attr_t. This structure becomes optional and is only looked at
 by the kernel if the PFM_RWFL_PMD_ATTR flag is passed.

 For all counting applications, using pfarg_pmr_t is enough. The nice
 side effect of this split is that the cost of reading and writing PMD register
 is now reduced because we have less data to copy in and out of the kernel.

 Unlike suggested by some people, I have not merged the notions of
 PMD and PMC registers. I think it is cleaner to separate them out. It
 also makes it much easier to provide backward compatibility with v2.81.

III) attaching and detaching

  With v2.81:
     int pfm_load_context(int fd, pfarg_load_t *load);
     int pfm_unload_context(int fd);

  With v3.0:
     int pfm_attach_session(int fd, int flags, int target);
     int pfm_detach_session(int fd, int flags);

  The pfarg_load_t structure has been abandoned. The information about what
  to attach to is passed as a parameter to the syscall in "target". It
can either be
  a thread id or a CPU id.

  There are currently no flags defined for either call.

   Note that we have lost the ability to specify which event set is
   to be activated first. There was no actual use of this option anyway.

   Some people have suggested that I use 'unsigned long' instead of 'int'
   for target. I am not against it.

 IV) starting and stopping

   With v2.81:
      int pfm_start(int fd, pfarg_start_t *st);
      int pfm_stop(int fd);
      int pfm_restart(int fd);

   With v3.0:
      int pfm_start_session(int fd, int flags);
      int pfm_stop_session(int fd, int flags);

     New flags:
     PFM_STFL_RESTART: resume monitoring after an overflow notification

     The pfarg_start_t structure has been abandoned.

     The pfm_restart() syscall has been merged with pfm_start() by
     using the PFM_STFL_RESTART  flag. It is not possible to just
     use pfm_start_session() and internally determine what to do
     because this is dependent on the sampling format.

      We have lost the ability to specify on which event set to
      start. I don't think this option was ever used.

  V) event set and multiplexing

     With v2.81:
        int pfm_create_evtsets(int fd, pfarg_setdesc_t *s, int n);
        int pfm_getinfo_evtsets(int fd, pfarg_setinfo_t *s, int n);
        int pfm_delete_evtsets(int fd, pfarg_setdesc_t *s, int n);

    With v3.0:
        int pfm_create_sets(int fd, int flags, pfarg_setdesc_t *s, int n);
        int pfm_getinfo_sets(int fd, int flags, pfarg_setinfo_t *s, int n);

    We have kept the same data structures and simply added a flags
    parameters to provide for extensibility of the calls.

    We have removed pfm_delete_evtsets() because it was not used by
    a lot of applications. We could add it back later if there is a good reason
    for it , something stronger than saying it needs to be there for symmetry.


The code for v3.0 has been uploaded into the perfmon GIT tree at kernel.org.
 It is located in the perfmon3 branch.

I am hoping this will lift the last remaining issues and we will be
able to  start
merging perfmon3 into mainline.

Thanks.

Re: perfmon3 interface overview

[David Gibson]

On Thu, Sep 25, 2008 at 11:48:27PM +0200, stephane eranian wrote:
> Hello,
> 
> A few months ago, I started posting on this list a highly simplified version
> of the perfmon2 version which was providing only per-thread counting on X86
> processors.
> 
> The feedback I got was generally positive but people raised two more issues:
> 
>     - too many system calls for a single subsystem (12 calls)
> 
>     - how to ensure syscalls could be extended without necessarily
>       adding new ones.
> 
> 
> Since then, I have been working hard on addressing those two issues,
> in other words redesign of the perfmon2 syscall API. This message is
> to announce that I now have a new proposal for the API. As expected
> it is called perfmon3 and it addresses the two issues above while
> allowing backward compatibility with the existing v2.81 version
> through a user level glue layer which could be implemented by a
> library such as libpfm.

> The new API now has 8 system calls in its fully-featured
> version. Many data structures shared with user level have been
> abandoned in favor of explicit syscall parameters.  Each syscall has

Ah, excellent, big structures passed through an interface are often a
pain to deal with, so I'm glad these are going.

> a flags parameters which allows the syscalls to be extended with new
> parameters when we need them. Most structures passed to the kernel
> have reserved fields for future extensions.
> 
> The initial patchset will only support per-thread counting as I did
> previously. However, here I am presenting the details for the fully
> featured version so people can get a feel for it. For each call I
> show the old way and the new way.
> 
> Note that when the syscall is shown twice with a different number of
> parameters, this variation is handled by the user library. The
> kernel implements the full call. This is the same technique used for
> the open(2) syscall.

I, at least, like the changes that have been made here.  I do have a
few suggestions for further simplifications along the same lines
though, see below.

[snip]
>  II) programming the registers
> 
>  With v2.81:
>       int pfm_write_pmcs(int fd, pfarg_pmc_t *pmds, int  n);
>       int pfm_write_pmds(int fd, pfarg_pmd_t *pmcs, int n);
>       int pfm_read_pmds(int fd, parg_pmd_t *pmds, int n);
> 
>  With v3.0:
>       int pfm_write_pmrs(int fd, int flags, pfarg_pmr_t *pmrs, int n);
>       int pfm_write_pmrs(int fd, int flags, pfarg_pmr_t *pmrs, int n,
>                                       pfarg_pmd_attr_t *pmas);
> 
>       int pfm_read_pmrs(int fd, int flags, pfarg_pmr_t *pmrs, int n);
>       int pfm_read_pmrs(int fd, int flags, parg_pmr_t *pmrs, int n,
>                                      pfarg_pmd_attr_t *pmas);

I would suggest adding 'type' and 'size' (int) parameters, and folding
the pmrs and pmas arrays into one freeform array.  'size' gives the
size in bytes of the array elements, 'type' selects the structure of
each array element.  This means:
	- if you ever have some sort of registers to access that are
neither PMCs nor PMDs, you can access those two, using a new type
value.
	- if a new method of operation wants to supply different
information for PMCs or PMDs, you can define a new structure and type
value (one obvious example would be a minimal (num,value) only
structure for minimum latency on simple applications).
	- an explicit size value means things can marshal / copy /
whatnot the parameters without having to know what size each type
implies -  the code will stay valid when new types / structures are
defined.

[snip]
> III) attaching and detaching
> 
>   With v2.81:
>      int pfm_load_context(int fd, pfarg_load_t *load);
>      int pfm_unload_context(int fd);
> 
>   With v3.0:
>      int pfm_attach_session(int fd, int flags, int target);
>      int pfm_detach_session(int fd, int flags);

Couldn't you get rid of one more syscall here by making detach a
special case of attach with a special "null" value for target, or a
special flag?

>  IV) starting and stopping
> 
>    With v2.81:
>       int pfm_start(int fd, pfarg_start_t *st);
>       int pfm_stop(int fd);
>       int pfm_restart(int fd);
> 
>    With v3.0:
>       int pfm_start_session(int fd, int flags);
>       int pfm_stop_session(int fd, int flags);

Likewise, couldn't you cut this down by one more syscall by making it
	int pfm_set_session_state(int fd, int flags);
and having a 'RUNNING' flag, which selects start or stop behaviour?

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: perfmon3 interface overview

[stephane eranian]

David,

On Fri, Oct 3, 2008 at 8:17 AM, David Gibson
<david@gibson.dropbear.id.au> wrote:
>
> > The new API now has 8 system calls in its fully-featured
> > version. Many data structures shared with user level have been
> > abandoned in favor of explicit syscall parameters.  Each syscall has
>
> Ah, excellent, big structures passed through an interface are often a
> pain to deal with, so I'm glad these are going.

 Thanks.

>
>
> I, at least, like the changes that have been made here.  I do have a
> few suggestions for further simplifications along the same lines
> though, see below.
>
> [snip]
> >  II) programming the registers
> >
> >  With v2.81:
> >       int pfm_write_pmcs(int fd, pfarg_pmc_t *pmds, int  n);
> >       int pfm_write_pmds(int fd, pfarg_pmd_t *pmcs, int n);
> >       int pfm_read_pmds(int fd, parg_pmd_t *pmds, int n);
> >
> >  With v3.0:
> >       int pfm_write_pmrs(int fd, int flags, pfarg_pmr_t *pmrs, int n);
> >       int pfm_write_pmrs(int fd, int flags, pfarg_pmr_t *pmrs, int n,
> >                                       pfarg_pmd_attr_t *pmas);
> >
> >       int pfm_read_pmrs(int fd, int flags, pfarg_pmr_t *pmrs, int n);
> >       int pfm_read_pmrs(int fd, int flags, parg_pmr_t *pmrs, int n,
> >                                      pfarg_pmd_attr_t *pmas);
>
> I would suggest adding 'type' and 'size' (int) parameters, and folding
> the pmrs and pmas arrays into one freeform array.  'size' gives the
> size in bytes of the array elements, 'type' selects the structure of
> each array element.  This means:
>        - if you ever have some sort of registers to access that are
> neither PMCs nor PMDs, you can access those two, using a new type
> value.
>        - if a new method of operation wants to supply different
> information for PMCs or PMDs, you can define a new structure and type
> value (one obvious example would be a minimal (num,value) only
> structure for minimum latency on simple applications).
>        - an explicit size value means things can marshal / copy /
> whatnot the parameters without having to know what size each type
> implies -  the code will stay valid when new types / structures are
> defined.

 So you are suggesting something along the lines:

   int pfm_read_pmrs(int fd, int flags, int type, void *tab, size_t sz);
   int pfm_write_pmrs(int fd, int flags, int type, void *tab, size_t sz);

 I have already introduced a type flag (PFM_RWFL_PMD, PFM_RWFL_PMC).
 Why separate the type into its own parameter?

 As for the freeform array, isn't that what people do not like because
of  void *
 and thus weak type checking?

 I will look at switching to size instead of count. I think it does make sense.

>
> [snip]
> > III) attaching and detaching
> >
> >   With v2.81:
> >      int pfm_load_context(int fd, pfarg_load_t *load);
> >      int pfm_unload_context(int fd);
> >
> >   With v3.0:
> >      int pfm_attach_session(int fd, int flags, int target);
> >      int pfm_detach_session(int fd, int flags);
>
> Couldn't you get rid of one more syscall here by making detach a
> special case of attach with a special "null" value for target, or a
> special flag?


 We could combine the two and use the flag field to indicate attach/detach.
 The target is not a pointer but an int. Some people suggested I use an
 unsigned long instead. In anycase, we could not use 0 to indicate "detach"
 because CPU0 is a valid choice for system-wide. Thus we would have to
 pick another value to mean "nothing", e.g, -1.

 >  IV) starting and stopping
 >
 >    With v2.81:
 >       int pfm_start(int fd, pfarg_start_t *st);
 >       int pfm_stop(int fd);
 >       int pfm_restart(int fd);
 >
 >    With v3.0:
 >       int pfm_start_session(int fd, int flags);
 >       int pfm_stop_session(int fd, int flags);

> Likewise, couldn't you cut this down by one more syscall by making it
>        int pfm_set_session_state(int fd, int flags);
> and having a 'RUNNING' flag, which selects start or stop behaviour?

 That one we can certainly do. That's a good idea.

 Thanks for your feedback, keep it coming.

Re: perfmon3 interface overview

[stephane eranian]

David,
>> [snip]
>> > III) attaching and detaching
>> >
>> >   With v2.81:
>> >      int pfm_load_context(int fd, pfarg_load_t *load);
>> >      int pfm_unload_context(int fd);
>> >
>> >   With v3.0:
>> >      int pfm_attach_session(int fd, int flags, int target);
>> >      int pfm_detach_session(int fd, int flags);
>>
>> Couldn't you get rid of one more syscall here by making detach a
>> special case of attach with a special "null" value for target, or a
>> special flag?
>
>
>  We could combine the two and use the flag field to indicate attach/detach.
>  The target is not a pointer but an int. Some people suggested I use an
>  unsigned long instead. In anycase, we could not use 0 to indicate "detach"
>  because CPU0 is a valid choice for system-wide. Thus we would have to
>  pick another value to mean "nothing", e.g, -1.
>
>  >  IV) starting and stopping
>  >
>  >    With v2.81:
>  >       int pfm_start(int fd, pfarg_start_t *st);
>  >       int pfm_stop(int fd);
>  >       int pfm_restart(int fd);
>  >
>  >    With v3.0:
>  >       int pfm_start_session(int fd, int flags);
>  >       int pfm_stop_session(int fd, int flags);
>
>> Likewise, couldn't you cut this down by one more syscall by making it
>>        int pfm_set_session_state(int fd, int flags);
>> and having a 'RUNNING' flag, which selects start or stop behaviour?
>
>  That one we can certainly do. That's a good idea.

Some more thoughts on this.

If we wanted to go even further, we could combine start/stop, attach/detach
into  a single syscall:

   int pfm_control_session(int fd, int flags, int target);
   With flags:
       PFM_CTFL_START  : start monitoring
       PFM_CTFL_STOP    : stop monitoring
       PFM_CTFL_RESTART: resume after overflow notification

       PFM_CTFL_ATTACH: attach to thread or cpu designated by 'target'
       PFM_CTFL_DETACH: detach session

But then, this is a form of ioctl() which people don't like....

Re: perfmon3 interface overview

[stephane eranian]

Hello,

Based on the comments from David Gibson, here is another version of
the v3.0 API. This one
collapses the system calls to the maximum while still providing a lot
of flexbility. We are down
to  5 system calls from 12. Of course, each syscall has more of an
ioctl()-style to it but types of
actions are still well separated.

I) session creation

  With v2.81:
    int pfm_create_context(pfarg_ctx_t *ctx,
                           char *smpl_name,
                           void *smpl_arg,
                           size_t smpl_size);
 With v3.0:
    int pfm_create_session(int flags, pfarg_sinfo_t *sif);

    int pfm_create_session(int flags, pfarg_sinfo_t *sif,
                           char *smpl_name,
                           void *smpl_arg,
                           size_t smpl_size);

    Flags:
        PFM_FL_SMPL_FMT    : using sampling format (3 extra params passed)
        PFM_FL_SYSTEM_WIDE : create a system-wide (cpu-wide) session
        PFM_FL_OVFL_NO_MSG  : do not send overflow message (just signal)

    typedef {
        __u64 sif_avail_pmcs[PFM_PMC_BV];/* out: available PMCs */
        __u64 sif_avail_pmds[PFM_PMD_BV];/* out: available PMDs */
        __u64 sif_reserved1[4];          /* for future use */
    } pfarg_sinfo_t;

    The sinfo structure is systematically returned. It contains bitmasks
    showing which registers are available to the session. The PMU may be
    shared with other subsystems, so tools cannot assume they have access
    to all registers. Furthermore, some registers may not be available to
    both types of sessions (per-thread, system-wide).

    If PFM_FL_SMPL_FMT is present, then the kernel expects the extra 3
    paramters to describe which kernel sampling format to use, and its
    optional parameters.

 II) programming the registers

 With v2.81:
      int pfm_write_pmcs(int fd, pfarg_pmc_t *pmds, int  n);
      int pfm_write_pmds(int fd, pfarg_pmd_t *pmcs, int n);
      int pfm_read_pmds(int fd, parg_pmd_t *pmds, int n);
 With v3.0:
      int pfm_write_pmrs(int fd, int flags, void *pmrs, size_t sz);
      int pfm_read_pmrs(int fd, int flags, void **pmrs, size_t sz);

 New structures:

   typedef struct {
       u16 reg_num;
       u16 reg_set;
       u32 reg_flags;
       u64 reg_value;
   } pfarg_pmr_t;

   typedef struct {
       u16 reg_num;
       u16 reg_set;
       u32 reg_flags;
       u64 reg_value;
       u64 reg_long_reset;
       u64 reg_short_reset;
       u64 reg_random_mask;
       u64 reg_smpl_pmds[PFM_PMD_BV];
       u64 reg_reset_pmds[PFM_PMD_BV];
       u64 reg_ovfl_swcnt;
       u64 reg_smpl_eventid;
       u64 reg_last_value;
       u64 reg_reserved[8];
   } pfarg_pmd_attr_t;

 New flags:
    PFM_RWFL_PMD  : passing simplified PMD registers in pfarg_pmr_t
    PFM_RWFL_PMC  : passing PMD registers in pfarg_pmr_t
    PFM_RWFL_PMD_ATTR: passing full PMD registers in pfarg_pmd_attr_t

 We now use only 2 system calls to read and write the PMU registers.

 The vector is now void *. That gives us the ability to pass new types
 of structure in the future if we have to. Also we now use size instead
 of count.

 The calls provides two modes for passing PMD registers. A simplified mode
 used mostly for counting where no attributes are passed. A full mode, where
 extended attributes are passed for kernel-level sampling and multiplexing.
 That way, counting sessions do not pay the price of copying in/out the
 large set of attributes.

  There is only one mode to pass PMC registers.

III) attaching/detaching, starting/stopping

  With v2.81:
     int pfm_load_context(int fd, pfarg_load_t *load);
     int pfm_unload_context(int fd);
     int pfm_start(int fd, pfarg_start_t *st);
     int pfm_stop(int fd);
     int pfm_restart(int fd);

  With v3.0:
     int pfm_control_session(int fd, int flags, int target);

     New flags:
       PFM_CTFL_START: start monitoring
       PFM_CTFL_STOP : stop monitoring
       PFM_CTFL_RESTART: resume monitoring after a notification
       PFM_CTFL_ATTACH: attach to thread or CPU designated by 'target'
       PFM_CTFL_DETACH: detach from thread or CPU (target not used)

   This is a combined syscall, because all those operations have something
   in common, they control the state of the session.


  V) event set and multiplexing
     With v2.81:
        int pfm_create_evtsets(int fd, pfarg_setdesc_t *s, int n);
        int pfm_getinfo_evtsets(int fd, pfarg_setinfo_t *s, int n);
        int pfm_delete_evtsets(int fd, pfarg_setdesc_t *s, int n);

    With v3.0:
        int pfm_control_sets(int fd, int flags, void *s, size_t sz;

        typedef struct {
        __u16 set_id;             /* which set */
        __u16 set_reserved1;      /* for future use */
        __u32 set_flags;          /* SETFL flags  */
        __u64 set_timeout;        /* switch timeout in nsecs */
        __u64 reserved[6];        /* for future use */
        } pfarg_set_desc;

        typedef struct {
        __u16 set_id;                   /* which set */
        __u16 set_reserved1;            /* for future use */
        __u32 set_reserved2;            /* for future use */
        __u64 set_ovfl_pmds[PFM_PMD_BV];/* out: last ovfl PMDs */
        __u64 set_runs;         /* out: #times the set was active */
        __u64 set_timeout;      /* out: leftover timeout (nsecs) */
        __u64 set_duration;     /* out: time set was active in nsecs */
        __u64 set_reserved3[4]; /* for future use */
        } pfarg_set_info;

    New flags:
      PFM_STFL_CREAT : create new event sets using pfarg_set_desc_t
      PFM_STFL_INFO  : get info about event sets using pfarg_set_info_t


   Here again we combine syscall because they all control events sets.
   The pfm_delete_evtsets() has no equivalent, but it could easily be added
   if we have a strong need for it.


Any opinions on this revised v3.0 interface?

Thanks

Re: perfmon3 interface overview

[David Gibson]

On Fri, Oct 03, 2008 at 01:12:09PM +0200, stephane eranian wrote:
> David,
> >> [snip]
> >> > III) attaching and detaching
> >> >
> >> >   With v2.81:
> >> >      int pfm_load_context(int fd, pfarg_load_t *load);
> >> >      int pfm_unload_context(int fd);
> >> >
> >> >   With v3.0:
> >> >      int pfm_attach_session(int fd, int flags, int target);
> >> >      int pfm_detach_session(int fd, int flags);
> >>
> >> Couldn't you get rid of one more syscall here by making detach a
> >> special case of attach with a special "null" value for target, or a
> >> special flag?
> >
> >
> >  We could combine the two and use the flag field to indicate attach/detach.
> >  The target is not a pointer but an int. Some people suggested I use an
> >  unsigned long instead. In anycase, we could not use 0 to indicate "detach"
> >  because CPU0 is a valid choice for system-wide. Thus we would have to
> >  pick another value to mean "nothing", e.g, -1.
> >
> >  >  IV) starting and stopping
> >  >
> >  >    With v2.81:
> >  >       int pfm_start(int fd, pfarg_start_t *st);
> >  >       int pfm_stop(int fd);
> >  >       int pfm_restart(int fd);
> >  >
> >  >    With v3.0:
> >  >       int pfm_start_session(int fd, int flags);
> >  >       int pfm_stop_session(int fd, int flags);
> >
> >> Likewise, couldn't you cut this down by one more syscall by making it
> >>        int pfm_set_session_state(int fd, int flags);
> >> and having a 'RUNNING' flag, which selects start or stop behaviour?
> >
> >  That one we can certainly do. That's a good idea.
> 
> Some more thoughts on this.
> 
> If we wanted to go even further, we could combine start/stop, attach/detach
> into  a single syscall:

Well, you could.  But the attach/detach take a parameter which
start/stop don't, making it a less obvious merge to make.

>    int pfm_control_session(int fd, int flags, int target);
>    With flags:
>        PFM_CTFL_START  : start monitoring
>        PFM_CTFL_STOP    : stop monitoring
>        PFM_CTFL_RESTART: resume after overflow notification
> 
>        PFM_CTFL_ATTACH: attach to thread or cpu designated by 'target'
>        PFM_CTFL_DETACH: detach session
> 
> But then, this is a form of ioctl() which people don't like....

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: perfmon3 interface overview

[stephane eranian]

David,

On Sat, Oct 4, 2008 at 8:05 AM, David Gibson
<david@gibson.dropbear.id.au> wrote:
>>
>> If we wanted to go even further, we could combine start/stop, attach/detach
>> into  a single syscall:
>
> Well, you could.  But the attach/detach take a parameter which
> start/stop don't, making it a less obvious merge to make.
>
Unless you make the 3rd argument optional and hide this in a
user library. This is how this is handled for pfm_create_session()
for instance. The library would define this as follows:

   int pfm_control_session(int fd, int flags, ...);

Based upon flags, it would use va_arg() to get to the 3rd argument and
pass it to the syscall which implements the version below. A dummy value
would be passed with the flags is not equal to PFM_CTFL_ATTACH.

>>    int pfm_control_session(int fd, int flags, int target);
>>    With flags:
>>        PFM_CTFL_START  : start monitoring
>>        PFM_CTFL_STOP    : stop monitoring
>>        PFM_CTFL_RESTART: resume after overflow notification
>>
>>        PFM_CTFL_ATTACH: attach to thread or cpu designated by 'target'
>>        PFM_CTFL_DETACH: detach session
>>

Re: perfmon3 interface overview

[David Gibson]

On Sat, Oct 04, 2008 at 09:20:00AM +0200, stephane eranian wrote:
> David,
> 
> On Sat, Oct 4, 2008 at 8:05 AM, David Gibson
> <david@gibson.dropbear.id.au> wrote:
> >>
> >> If we wanted to go even further, we could combine start/stop, attach/detach
> >> into  a single syscall:
> >
> > Well, you could.  But the attach/detach take a parameter which
> > start/stop don't, making it a less obvious merge to make.
> >
> Unless you make the 3rd argument optional and hide this in a
> user library. This is how this is handled for pfm_create_session()
> for instance. The library would define this as follows:
> 
>    int pfm_control_session(int fd, int flags, ...);
> 
> Based upon flags, it would use va_arg() to get to the 3rd argument and
> pass it to the syscall which implements the version below. A dummy value
> would be passed with the flags is not equal to PFM_CTFL_ATTACH.

Yeah, that's what I'm saying.  You could do it, but it feels like
you're forcing it, at least to me.  As you say it starts to look a bit
like ioctl() again.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: perfmon3 interface overview

[David Gibson]

On Fri, Oct 03, 2008 at 12:58:12PM +0200, stephane eranian wrote:
> David,
> 
> On Fri, Oct 3, 2008 at 8:17 AM, David Gibson
> <david@gibson.dropbear.id.au> wrote:
> >
> > > The new API now has 8 system calls in its fully-featured
> > > version. Many data structures shared with user level have been
> > > abandoned in favor of explicit syscall parameters.  Each syscall has
> >
> > Ah, excellent, big structures passed through an interface are often a
> > pain to deal with, so I'm glad these are going.

[snip]
>  So you are suggesting something along the lines:
> 
>    int pfm_read_pmrs(int fd, int flags, int type, void *tab, size_t sz);
>    int pfm_write_pmrs(int fd, int flags, int type, void *tab, size_t sz);

Uh.. maybe.. there are actually several possible variants all of which
would meet the general idea I had in mind.

>  I have already introduced a type flag (PFM_RWFL_PMD, PFM_RWFL_PMC).
>  Why separate the type into its own parameter?

Combining the type into the flags is certainly a possibility.  I was
just a bit worried that if you eventually have enough actual flag
flags, in addition to the type values, you might start running out of
bits.

>  As for the freeform array, isn't that what people do not like because
> of  void *
>  and thus weak type checking?

Yeah; this is an interesting tradeoff of flexibility versus
predictability.  Actually, what I originally had in mind was
seperately passing both 'n' and a per-element size.  That provides a
bit more defined structure to the void * - it must be an array of n
identical, fixed-size elements.  The internal structure of each
element is defined only by type, but it is assumed to contain no
pointers to further chained structures (i.e. its safe for wrapper code
to do shallow copies of the array).

>  I will look at switching to size instead of count. I think it does
>  make sense.

Yeah.  As I said I was originally thinking of keeping the 'n'
parameter, and adding an element size parameter.  But just having an
overall size is also a possibility - it gives less definition to the
internal structure but at least things can marshal or copy the whole
array parameter without having to know its internals.

[snip]
> > > III) attaching and detaching
> > >
> > >   With v2.81:
> > >      int pfm_load_context(int fd, pfarg_load_t *load);
> > >      int pfm_unload_context(int fd);
> > >
> > >   With v3.0:
> > >      int pfm_attach_session(int fd, int flags, int target);
> > >      int pfm_detach_session(int fd, int flags);
> >
> > Couldn't you get rid of one more syscall here by making detach a
> > special case of attach with a special "null" value for target, or a
> > special flag?
> 
>  We could combine the two and use the flag field to indicate attach/detach.
>  The target is not a pointer but an int. Some people suggested I use an
>  unsigned long instead. In anycase, we could not use 0 to indicate "detach"
>  because CPU0 is a valid choice for system-wide. Thus we would have to
>  pick another value to mean "nothing", e.g, -1.

Sorry, I didn't express myself well.  I realise it's an integer, and
didn't mean an actual NULL, but as you say a special integer value
with a function similar to NULL.  -1 is the most obvious choice.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: perfmon3 interface overview

[stephane eranian]

David,

On Sun, Oct 5, 2008 at 7:53 AM, David Gibson
<david@gibson.dropbear.id.au> wrote:
> [snip]
>>  So you are suggesting something along the lines:
>>
>>    int pfm_read_pmrs(int fd, int flags, int type, void *tab, size_t sz);
>>    int pfm_write_pmrs(int fd, int flags, int type, void *tab, size_t sz);
>
> Uh.. maybe.. there are actually several possible variants all of which
> would meet the general idea I had in mind.
>
>>  I have already introduced a type flag (PFM_RWFL_PMD, PFM_RWFL_PMC).
>>  Why separate the type into its own parameter?
>
> Combining the type into the flags is certainly a possibility.  I was
> just a bit worried that if you eventually have enough actual flag
> flags, in addition to the type values, you might start running out of
> bits.
>
I see, so you were just decoupling to double the number of available bits.
I guess we have a minimum of 32 bits. It seems overkill to support up to
32 'types'. We could force flags to uint64_t. But then we would have to do
the same for all other syscalls to be consistent. Defining flags as long
won't work because of 32-bit vs. 64-bit ABI compatibility issues.




>>  As for the freeform array, isn't that what people do not like because
>> of  void *
>>  and thus weak type checking?
>
> Yeah; this is an interesting tradeoff of flexibility versus
> predictability.  Actually, what I originally had in mind was
> seperately passing both 'n' and a per-element size.  That provides a
> bit more defined structure to the void * - it must be an array of n
> identical, fixed-size elements.  The internal structure of each
> element is defined only by type, but it is assumed to contain no
> pointers to further chained structures (i.e. its safe for wrapper code
> to do shallow copies of the array).
>

Ok, that reminds me of the API of calloc(). It certainly forces you to
think it terms of 'array of elements'. Yet it can be perverted very easily
with the number of elements at 1.

>>  I will look at switching to size instead of count. I think it does
>>  make sense.
>
> Yeah.  As I said I was originally thinking of keeping the 'n'
> parameter, and adding an element size parameter.  But just having an
> overall size is also a possibility - it gives less definition to the
> internal structure but at least things can marshal or copy the whole
> array parameter without having to know its internals.
>
Yes, that it true. It also simplifies the checking on size. With count
we had to check for multiply overflow because internall we had to
check the size anyway.

> [snip]
>> > > III) attaching and detaching
>> > >
>> > >   With v2.81:
>> > >      int pfm_load_context(int fd, pfarg_load_t *load);
>> > >      int pfm_unload_context(int fd);
>> > >
>> > >   With v3.0:
>> > >      int pfm_attach_session(int fd, int flags, int target);
>> > >      int pfm_detach_session(int fd, int flags);
>> >
>> > Couldn't you get rid of one more syscall here by making detach a
>> > special case of attach with a special "null" value for target, or a
>> > special flag?
>>
>>  We could combine the two and use the flag field to indicate attach/detach.
>>  The target is not a pointer but an int. Some people suggested I use an
>>  unsigned long instead. In anycase, we could not use 0 to indicate "detach"
>>  because CPU0 is a valid choice for system-wide. Thus we would have to
>>  pick another value to mean "nothing", e.g, -1.
>
> Sorry, I didn't express myself well.  I realise it's an integer, and
> didn't mean an actual NULL, but as you say a special integer value
> with a function similar to NULL.  -1 is the most obvious choice.
>
Yes, -1 would work.

If I summarize our discussion. It seems we can define the API as follows:

    int pfm_create_session(int fd, uint64_t flags, pfarg_sinfo_t *sif,
[ char *smpl_name, void *smpl_arg, size_t arg_size]);
    int pfm_read_pmrs(int fd, uint64_t flags, void *tab, size_t sz);
    int pfm_write_pmrs(int fd, uint64_t flags, void *tab, size_t sz);
    int pfm_attach_session(int fd, uint64_t flags, int target);   /*
attach, detach with target=-1 */
    int pfm_control_session(int fd, uint64_t flags);   /* for start/stop */
    int pfm_control_sets(int fd, uint64_t flags, void *sets, size_t sz);

Does this look reasonable?

Re: perfmon3 interface overview

[David Gibson]

On Sun, Oct 05, 2008 at 11:23:15PM +0200, stephane eranian wrote:
> David,
> 
> On Sun, Oct 5, 2008 at 7:53 AM, David Gibson
> <david@gibson.dropbear.id.au> wrote:
> > [snip]
> >>  So you are suggesting something along the lines:
> >>
> >>    int pfm_read_pmrs(int fd, int flags, int type, void *tab, size_t sz);
> >>    int pfm_write_pmrs(int fd, int flags, int type, void *tab, size_t sz);
> >
> > Uh.. maybe.. there are actually several possible variants all of which
> > would meet the general idea I had in mind.
> >
> >>  I have already introduced a type flag (PFM_RWFL_PMD, PFM_RWFL_PMC).
> >>  Why separate the type into its own parameter?
> >
> > Combining the type into the flags is certainly a possibility.  I was
> > just a bit worried that if you eventually have enough actual flag
> > flags, in addition to the type values, you might start running out of
> > bits.
> >
> I see, so you were just decoupling to double the number of available bits.
> I guess we have a minimum of 32 bits. It seems overkill to support up to
> 32 'types'. We could force flags to uint64_t. But then we would have to do
> the same for all other syscalls to be consistent. Defining flags as long
> won't work because of 32-bit vs. 64-bit ABI compatibility issues.

I guess so.  I guess there's also the possibility that you might later
have some flags that can apply to many of the syscalls, and it would
be nice to have them in a standard bit position for all calls.  That
might make bit allocation a bit interesting if you're squeezing type
into the same field.

My opinion is not really strong either way though.

> >>  As for the freeform array, isn't that what people do not like because
> >> of  void *
> >>  and thus weak type checking?
> >
> > Yeah; this is an interesting tradeoff of flexibility versus
> > predictability.  Actually, what I originally had in mind was
> > seperately passing both 'n' and a per-element size.  That provides a
> > bit more defined structure to the void * - it must be an array of n
> > identical, fixed-size elements.  The internal structure of each
> > element is defined only by type, but it is assumed to contain no
> > pointers to further chained structures (i.e. its safe for wrapper code
> > to do shallow copies of the array).
> 
> Ok, that reminds me of the API of calloc(). It certainly forces you to
> think it terms of 'array of elements'. Yet it can be perverted very easily
> with the number of elements at 1.

That's true.  Like I say, it's a tradeoff.

> >>  I will look at switching to size instead of count. I think it does
> >>  make sense.
> >
> > Yeah.  As I said I was originally thinking of keeping the 'n'
> > parameter, and adding an element size parameter.  But just having an
> > overall size is also a possibility - it gives less definition to the
> > internal structure but at least things can marshal or copy the whole
> > array parameter without having to know its internals.
> >
> Yes, that it true. It also simplifies the checking on size. With count
> we had to check for multiply overflow because internall we had to
> check the size anyway.

That's a nice extra bonus, yes.

> > [snip]
> >> > > III) attaching and detaching
> >> > >
> >> > >   With v2.81:
> >> > >      int pfm_load_context(int fd, pfarg_load_t *load);
> >> > >      int pfm_unload_context(int fd);
> >> > >
> >> > >   With v3.0:
> >> > >      int pfm_attach_session(int fd, int flags, int target);
> >> > >      int pfm_detach_session(int fd, int flags);
> >> >
> >> > Couldn't you get rid of one more syscall here by making detach a
> >> > special case of attach with a special "null" value for target, or a
> >> > special flag?
> >>
> >>  We could combine the two and use the flag field to indicate attach/detach.
> >>  The target is not a pointer but an int. Some people suggested I use an
> >>  unsigned long instead. In anycase, we could not use 0 to indicate "detach"
> >>  because CPU0 is a valid choice for system-wide. Thus we would have to
> >>  pick another value to mean "nothing", e.g, -1.
> >
> > Sorry, I didn't express myself well.  I realise it's an integer, and
> > didn't mean an actual NULL, but as you say a special integer value
> > with a function similar to NULL.  -1 is the most obvious choice.
> >
> Yes, -1 would work.
> 
> If I summarize our discussion. It seems we can define the API as follows:
> 
>     int pfm_create_session(int fd, uint64_t flags, pfarg_sinfo_t *sif,
> [ char *smpl_name, void *smpl_arg, size_t arg_size]);
>     int pfm_read_pmrs(int fd, uint64_t flags, void *tab, size_t sz);
>     int pfm_write_pmrs(int fd, uint64_t flags, void *tab, size_t sz);
>     int pfm_attach_session(int fd, uint64_t flags, int target);   /*
> attach, detach with target=-1 */
>     int pfm_control_session(int fd, uint64_t flags);   /* for start/stop */

Unless you have other functions in mind for this, I'd suggest a name
that's a little less generic here.  pfm_set_running() maybe.

>     int pfm_control_sets(int fd, uint64_t flags, void *sets, size_t
>     sz);

Hrm.  This now has identical signature to the {read,write}_pmrs.  I
wonder if these could be sensibly combined.  Maybe have
pfm_get_info(), pfm_set_info() which can get/set control structures
for several different namespaces: PMCs PMDs and now event sets.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [perfmon2] perfmon3 interface overview

[Arnd Bergmann]

On Sunday 05 October 2008, stephane eranian wrote:
> If I summarize our discussion. It seems we can define the API as follows:
> 
>     int pfm_create_session(int fd, uint64_t flags, pfarg_sinfo_t *sif,
> [ char *smpl_name, void *smpl_arg, size_t arg_size]);
>     int pfm_read_pmrs(int fd, uint64_t flags, void *tab, size_t sz);
>     int pfm_write_pmrs(int fd, uint64_t flags, void *tab, size_t sz);
>     int pfm_attach_session(int fd, uint64_t flags, int target);   /*
> attach, detach with target=-1 */
>     int pfm_control_session(int fd, uint64_t flags);   /* for start/stop */
>     int pfm_control_sets(int fd, uint64_t flags, void *sets, size_t sz);


There are two problems with uint64_t arguments to system calls:

1. You have to mandate the use of stdint.h before including the
perfmon header file. This is a minor problem as long as all callers
go through libpfm, but you can avoid it entirely by using the kernel
__u32 instead of uint32_t style types everywhere in your interface.

2. The calling conventions for passing 64 bit values on 32 bit
architecture are complex, every architecture enforces different
rules here. On the syscall interface, bettwe always use native
types like 'unsigned long' instead of '__u64'. This is different
from the case where you pass a pointer to data, in which case
a '__u64 *' is much preferred over a 'unsigned long *'.

I did not understand the reason for going to a 64 bit flags parameter,
but I think that you would do everyone a favour if you can instead
use two 32-bit flags or a unsigned long flags parameter.

	Arnd <><

Re: perfmon3 interface overview

[stephane eranian]

David,

On Tue, Oct 7, 2008 at 5:56 AM, David Gibson
<david@gibson.dropbear.id.au> wrote:
> On Sun, Oct 05, 2008 at 11:23:15PM +0200, stephane eranian wrote:
>> I see, so you were just decoupling to double the number of available bits.
>> I guess we have a minimum of 32 bits. It seems overkill to support up to
>> 32 'types'. We could force flags to uint64_t. But then we would have to do
>> the same for all other syscalls to be consistent. Defining flags as long
>> won't work because of 32-bit vs. 64-bit ABI compatibility issues.
>
> I guess so.  I guess there's also the possibility that you might later
> have some flags that can apply to many of the syscalls, and it would
> be nice to have them in a standard bit position for all calls.  That
> might make bit allocation a bit interesting if you're squeezing type
> into the same field.
>
I can see that for subsets of the calls, e.g., read and write of registers.
It is not clear to me what kind of 'service' would apply to all calls.

As Corey suggested yesteday, the flags for read and write operations
are not just single bit. A portion of the 64 bits, 8 in his proposal, are used
to encode the 'type' of register (PMD, PMC, PMD_ATTR). That simplifies
checking for valid bits in this portion of flags.

OTOH, if I go back to your original proposal:
   int pfm_write_pmrs(int fd, int flags, int type, void *v, size_t);

The 'type' field would have the same encoding as proposed by Corey.
32 bits would be plentyful for types. Flags could remain at 32-bits
across the board, thereby avoiding the problems outlined by Arnd
in his follow-up message. Separating the type also makes it look
less like an ioctl(). So I think we should go with that proposal.

[snip]
>>
>> If I summarize our discussion. It seems we can define the API as follows:
>>
>>     int pfm_create_session(int fd, uint64_t flags, pfarg_sinfo_t *sif,
>> [ char *smpl_name, void *smpl_arg, size_t arg_size]);
>>     int pfm_read_pmrs(int fd, uint64_t flags, void *tab, size_t sz);
>>     int pfm_write_pmrs(int fd, uint64_t flags, void *tab, size_t sz);
>>     int pfm_attach_session(int fd, uint64_t flags, int target);   /*
>> attach, detach with target=-1 */
>>     int pfm_control_session(int fd, uint64_t flags);   /* for start/stop */
>
> Unless you have other functions in mind for this, I'd suggest a name
> that's a little less generic here.  pfm_set_running() maybe.
>
What about pfm_set_state() or pfm_set_session_state()?

>>     int pfm_control_sets(int fd, uint64_t flags, void *sets, size_t
>>     sz);
>
> Hrm.  This now has identical signature to the {read,write}_pmrs.  I
> wonder if these could be sensibly combined.  Maybe have
> pfm_get_info(), pfm_set_info() which can get/set control structures
> for several different namespaces: PMCs PMDs and now event sets.
>
I think this would be too generic as it would be mixing very different
data types. This is not the case for pfm_write_pmrs() and pfm_read_pmrs()
because you are always passing 'register descriptions'.

There is a fundamental difference between pfm_control_sets() and
pfm_write_pmrs()/pfm_read_pmrs(). For the latter, the 'action' is hardcoded
in the name of the syscall, i.e., read or write, what varies is whether or not
we are passing PMD or PMC. For the former, the 'action', i.e., create new
set or get info about a set, is hardcoded in the flags, and it determines the
type of structure passed in the third argument. In that sense,
pfm_control_sets()
has more the look and feel of ioctl(), but as David points out in his follow-up
message, the difference is that we have size and that provides a basis for
sanity checking in the kernel, unlike ioctl() which only has 'flags'.

In summary, here is the current proposal:

   int pfm_create_session(int fd, int flags, pfarg_sinfo_t *sif,
                                         [ char *smpl_name,
                                           void *smpl_arg,
                                           size_t arg_size]);

int pfm_read_pmrs(int fd, int flags, int type, void *tab, size_t sz);

int pfm_write_pmrs(int fd, int flags, int type, void *tab, size_t sz);

int pfm_attach_session(int fd, int flags, int target);
/* detach with target=-1 */

int pfm_set_session_state(int fd, int flags);
/* for start/stop */

int pfm_control_sets(int fd, int flags, void *sets, size_t sz);

Re: perfmon3 interface overview

[David Gibson]

On Tue, Oct 07, 2008 at 11:46:53AM +0200, stephane eranian wrote:
> David,
> 
> On Tue, Oct 7, 2008 at 5:56 AM, David Gibson
> <david@gibson.dropbear.id.au> wrote:
> > On Sun, Oct 05, 2008 at 11:23:15PM +0200, stephane eranian wrote:
> >> I see, so you were just decoupling to double the number of available bits.
> >> I guess we have a minimum of 32 bits. It seems overkill to support up to
> >> 32 'types'. We could force flags to uint64_t. But then we would have to do
> >> the same for all other syscalls to be consistent. Defining flags as long
> >> won't work because of 32-bit vs. 64-bit ABI compatibility issues.
> >
> > I guess so.  I guess there's also the possibility that you might later
> > have some flags that can apply to many of the syscalls, and it would
> > be nice to have them in a standard bit position for all calls.  That
> > might make bit allocation a bit interesting if you're squeezing type
> > into the same field.
> >
> I can see that for subsets of the calls, e.g., read and write of registers.
> It is not clear to me what kind of 'service' would apply to all calls.
> 
> As Corey suggested yesteday, the flags for read and write operations
> are not just single bit. A portion of the 64 bits, 8 in his proposal, are used
> to encode the 'type' of register (PMD, PMC, PMD_ATTR). That simplifies
> checking for valid bits in this portion of flags.

Yes, that was always what I had in mind if flags and type were
combined.

> OTOH, if I go back to your original proposal:
>    int pfm_write_pmrs(int fd, int flags, int type, void *v, size_t);
> 
> The 'type' field would have the same encoding as proposed by Corey.
> 32 bits would be plentyful for types. Flags could remain at 32-bits
> across the board, thereby avoiding the problems outlined by Arnd
> in his follow-up message. Separating the type also makes it look
> less like an ioctl(). So I think we should go with that proposal.

Works for me.

> [snip]
> >>
> >> If I summarize our discussion. It seems we can define the API as follows:
> >>
> >>     int pfm_create_session(int fd, uint64_t flags, pfarg_sinfo_t *sif,
> >> [ char *smpl_name, void *smpl_arg, size_t arg_size]);
> >>     int pfm_read_pmrs(int fd, uint64_t flags, void *tab, size_t sz);
> >>     int pfm_write_pmrs(int fd, uint64_t flags, void *tab, size_t sz);
> >>     int pfm_attach_session(int fd, uint64_t flags, int target);   /*
> >> attach, detach with target=-1 */
> >>     int pfm_control_session(int fd, uint64_t flags);   /* for start/stop */
> >
> > Unless you have other functions in mind for this, I'd suggest a name
> > that's a little less generic here.  pfm_set_running() maybe.
> >
> What about pfm_set_state() or pfm_set_session_state()?

I guess so.

> >>     int pfm_control_sets(int fd, uint64_t flags, void *sets, size_t
> >>     sz);
> >
> > Hrm.  This now has identical signature to the {read,write}_pmrs.  I
> > wonder if these could be sensibly combined.  Maybe have
> > pfm_get_info(), pfm_set_info() which can get/set control structures
> > for several different namespaces: PMCs PMDs and now event sets.
> >
> I think this would be too generic as it would be mixing very different
> data types. This is not the case for pfm_write_pmrs() and pfm_read_pmrs()
> because you are always passing 'register descriptions'.
> 
> There is a fundamental difference between pfm_control_sets() and
> pfm_write_pmrs()/pfm_read_pmrs(). For the latter, the 'action' is hardcoded
> in the name of the syscall, i.e., read or write, what varies is whether or not
> we are passing PMD or PMC. For the former, the 'action', i.e., create new
> set or get info about a set, is hardcoded in the flags, and it determines the

Uh.. I was assuming if combined we'd drop the selecting flag from
control_sets() and instead make the pfm_read_stuff() call be the
get_info() equivalent when used on the eventsets space and
pfm_write_stuff() be the create/modify sets call.

The fact that the structure is different for read/write in this case
is a bit funny.  In fact it would probably be best to have different
'type' fields for read and write too (EVTSET_INFOm only usable for
reads , and EVTSET_CONTROL only for writes, maybe).


I'm not dead-set on combining these calls, but it seems to me we've
already just about made the read/write calls into a read/write array
of control structures in some namespace call, which pretty much is
what the event set call is too.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: perfmon3 interface overview

[stephane eranian]

David,


On Wed, Oct 8, 2008 at 2:53 AM, David Gibson
<david@gibson.dropbear.id.au> wrote:
David,

>> >>     int pfm_control_sets(int fd, uint64_t flags, void *sets, size_t
>> >>     sz);
>> >
>> > Hrm.  This now has identical signature to the {read,write}_pmrs.  I
>> > wonder if these could be sensibly combined.  Maybe have
>> > pfm_get_info(), pfm_set_info() which can get/set control structures
>> > for several different namespaces: PMCs PMDs and now event sets.
>> >
>> I think this would be too generic as it would be mixing very different
>> data types. This is not the case for pfm_write_pmrs() and pfm_read_pmrs()
>> because you are always passing 'register descriptions'.
>>
>> There is a fundamental difference between pfm_control_sets() and
>> pfm_write_pmrs()/pfm_read_pmrs(). For the latter, the 'action' is hardcoded
>> in the name of the syscall, i.e., read or write, what varies is whether or not
>> we are passing PMD or PMC. For the former, the 'action', i.e., create new
>> set or get info about a set, is hardcoded in the flags, and it determines the
>
> Uh.. I was assuming if combined we'd drop the selecting flag from
> control_sets() and instead make the pfm_read_stuff() call be the
> get_info() equivalent when used on the eventsets space and
> pfm_write_stuff() be the create/modify sets call.
>
There is something I'd like o clarify about event sets. People may be
wondering why we need a separate call to create new event sets. Why
not create the sets when the session is created, e.g., 'create a
session with 5 sets'.

There are several reasons for not doing this. Each set has some attributes which
describes how and when to switch to another set. Switching can be done based
on timeout or based on the number of overflows of counters, each chosen on a
per set basis, i.e,, it is possible to combine overflow and timeout switching.
Furthermore, the timeout is defined per set and can thus vary from one
set to the
other. Overflow switching is controlled by a threshold passed as an
attribute to the
PMD registers.

If we were to create sets with the sessions, it would be more than a matter of
passing the number of sets. We would actually have to pass the set attributes
(pfarg_set_desc_t) and a size parameter.

We could do this in v3.0 but it would have to work incrementally as initially we
won't support event sets in mainline. The call would look as follows:

int pfm_create_session(int flags, pfarg_sinfo_t *sif);
int pfm_create_session(int flags, pfarg_sinfo_t *sif, char *smpl_name,
void *smpl_arg, size_t arg_size);
int pfm_create_session(int flags, pfarg_sinfo_t *sif, char *smpl_name,
void *smpl_arg, size_t arg_size, pfarg_set_desc_t *s, size_t sz);

I think this call is a bit too complicated. It does a lot of things.
Furthermore, you lose the ability to add sets on the fly. Deleting may
be possible via another call.

So my inclination is to keep this as a separate call. By default, one
set is created (set0), with no switching attribute. It is possible
to change the attributes by recreating the set. That means, unless you
need more sets, you have nothing to do.


> The fact that the structure is different for read/write in this case
> is a bit funny.  In fact it would probably be best to have different
> 'type' fields for read and write too (EVTSET_INFOm only usable for
> reads , and EVTSET_CONTROL only for writes, maybe).
>
>
> I'm not dead-set on combining these calls, but it seems to me we've
> already just about made the read/write calls into a read/write array
> of control structures in some namespace call, which pretty much is
> what the event set call is too.
>
I see, you are pushing it a bit further here. If I understand, you're saying:

int pfm_read_session(int fd, int flags, int type, void *t, size_t sz);
int pfm_write_session(int fd, int flags, int type, void *t, size_t sz);

Where type could be:
   PFM_RWFL_PMD           : write simplified pmds registers (pfarg_pmr_t)
   PFM_RWFL_PMD_ATTR: read or write pmds registers with extended attributes.
   PFM_RWFL_PMC           : write pmcs register (pfarg_pmr_t), note:
this is for writing only.
   PFM_RWFL_SETINFO    : read set information (pfarg_set_info_t). Only
supported by pfm_read_session
   PFM_RWFL_CREATSET: create new event sets (pfarg_set_desc_t). Only
supported by pfm_write_session

I am a bit worried this is too generic, especially for creating sets.
Yet it offers the advantage of being
very compact and extensible. I'd like to combine pfarg_set_info_t and
pfarg_set_desc_t if possible, even
if some fields are used in only one of the calls.

In summary, here is the current proposal:

   int pfm_create_session(int flags, pfarg_sinfo_t *sif,
                                         [char *smpl_name,
                                          void *smpl_arg,
                                          size_t arg_sz]);

  int pfm_read_session(int fd, int flags, int type, void *t, size_t sz);

  int pfm_write_session(int fd, int flags, int type, void *t, size_t sz);

  int pfm_attach_session((int fd, int flags, int target); /* target =
-1 for detach */

  int pfm_set_state_session(int fd, int flags);

  No other calls are necessary to support event sets and multiplexing.