Re: [perfmon2] perfmon3 interface overview

[stephane e. <er...@go...>]

David,


On Fri, Oct 3, 2008 at 8:17 AM, David Gibson <da...@gi...>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.



>
> --
> 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 <http://www.ozlabs.org/%7Edgibson>
>