lkml.org 
[lkml]   [2006]   [Sep]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH 6/18] 2.6.17.9 perfmon2 patch for review: sampling format support
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
-
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/

\
 
 \ /
  Last update: 2006-09-01 18:13    [W:0.070 / U:0.472 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site