Re: [PATCH 6/18] 2.6.17.9 perfmon2 patch for review: sampling format support

[Stephane Eranian <eranian@hpl.hp.com>]

Andrew,

On Wed, Aug 23, 2006 at 03:35:37PM -0700, Andrew Morton wrote:
> On Wed, 23 Aug 2006 01:05:57 -0700
> Stephane Eranian <eranian@frankl.hpl.hp.com> wrote:
> 
> > This files contains the sampling format support.
> > 
> > Perfmon2 supports an in-kernel sampling buffer for performance
> > reasons. Yet to ensure maximum flexibility to applications,
> > the formats is which infmration is recorded into the kernel
> > buffer is not specified by the interface. Instead it is
> > delegated to a kernel plug-in modules called sampling formats.
> > 
> > Each formats controls:
> > 	- what is recorded in the the sampling buffer
> > 	- how the information is recorded
> > 	- when to notify the application to extract the information
> > 	- how the buffer is exported to user level
> > 	- hoe the buffer is allocated
> > 
> > Each format is identified via a 128-bit UUID which can be requested
> > when the context is created with pfm_create_context().
> > 
> > The interface comes with a simple default sampling format. It records
> > information sequentially in the buffer. Each entry, called sample,
> > is composed of a fixed size header and a variable size body where
> > the values of PMDS can be recorded based upon the user's request.
> > 
> > Sampling formats can be dynamically registered with perfmon. The management
> > of sampling formats is implemented in perfmon_fmt.c:
> > 
> > pfm_register_smpl_fmt(struct pfm_smpl_fmt *fmt):
> > 	- register a new sampling format
> > 		
> > pfm_unregister_smpl_fmt(pfm_uuid_t uuid):
> > 	- unregister a sampling format
> > 
> > It is possible to list the available formats by looking at /sys/kernel/perfmon/formats.
> > 
> 
> Why identify a format with a UUID rather than via a nice human-readable name?
> 

Although a UUID is slightly more difficult to manipulate than a clear text string, it
offers several advantages:
	- is guaranteed unique
	- generation is fully distributed
	- easy generation with uuidgen
	- fixed size
	- very easy to pass to the kernel, there is not char * in a struct pass to kernel
	- not to worry about '\0'

We use UUID to idenitfy a format + a version number. The version number can be useful
to identify backward compatible versions of a format.

> > +/*
> > + * find a buffer format based on its uuid
> > + */
> > +struct pfm_smpl_fmt *pfm_smpl_fmt_get(pfm_uuid_t uuid)
> > +{
> > +	struct pfm_smpl_fmt * fmt;
> > +
> > +	spin_lock(&pfm_smpl_fmt_lock);
> > +
> > +	fmt = __pfm_find_smpl_fmt(uuid);
> > +
> > +	/*
> > +	 * increase module refcount
> > +	 */
> > +	if (fmt && fmt_is_mod(fmt) && !try_module_get(fmt->owner))
> > +		fmt = NULL;
> > +
> > +	spin_unlock(&pfm_smpl_fmt_lock);
> > +
> > +	return fmt;
> > +}
> 
> Is pfm_smpl_fmt_lock really needed?  The module API _should_ be unracy wrt
> lookup and removal.  If the name of the module was equal to the name of the
> format (sensible) then perhaps the module system's
> refcounting/atomicity/lookup mechanisms are sufficient?
> 

I believe the module API + refcount make it safe against removal. When
perfmon does a lookup in the linked list of registered formats, it is not
clear to me what would protect it. I believe it would be safe if I used 
the rcu list instead of basic list.

> > +	pfm_sysfs_add_fmt(fmt);
> 
> Please check for and handle all sysfs-related errors.  All errors, indeed.
> 
> Yes, a lot of the kernel blithely assumes that sysfs operations never fail.
> We need to fix that badness rather than copy it.
> 
I fixed that now. 

thanks.
-- 
-Stephane