Date:	Fri, 3 Oct 2008 12:58:12 +0200
From:	"stephane eranian" <eranian@...glemail.com>
To:	"David Gibson" <david@...son.dropbear.id.au>, eranian@...il.com,
	linux-kernel@...r.kernel.org
Cc:	perfmon2-devel <perfmon2-devel@...ts.sourceforge.net>
Subject: Re: perfmon3 interface overview

David,

On Fri, Oct 3, 2008 at 8:17 AM, David Gibson
<david@...son.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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists