Messages in this thread | | | Date | Wed, 23 Aug 2006 15:35:37 -0700 | From | Andrew Morton <> | Subject | Re: [PATCH 6/18] 2.6.17.9 perfmon2 patch for review: sampling format support |
| |
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?
> +/* > + * 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?
> + 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.
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |