lkml.org 
[lkml]   [2008]   [Nov]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [patch 02/24] perfmon: base code
On Wed, Nov 26, 2008 at 12:42:00AM -0800, eranian@googlemail.com wrote:
> + * __pfm_unload_context() cannot fail
> + * in the context states we are interested in
> + */
> + switch (ctx->state) {
> + case PFM_CTX_LOADED:
> + ret = __pfm_unload_context(ctx);
> + break;
> + case PFM_CTX_ZOMBIE:
> + ret = __pfm_unload_context(ctx);
> + free_ok = 1;
> + break;
> + default:
> + BUG_ON(ctx->state != PFM_CTX_LOADED);

That is just BUG() because the condition is always true here.

> + *
> + * we come here if:
> + *
> + * - we are zombie and we need to cleanup our state

...
> +void pfm_handle_work(struct pt_regs *regs)
> +{
> + struct pfm_context *ctx;
> + unsigned long flags;
> + int type;
> +
> + if (!user_mode(regs))
> + return;

zombies are never in user mode. Either the comment
is wrong or the test.

> + * enable interrupt for vfree()
> + */
> + local_irq_enable();
> +
> + /*
> + * actual context free
> + */
> + pfm_free_context(ctx);
> +
> + /*
> + * restore interrupts as they were upon entry
> + */
> + local_irq_restore(flags);

That seems redundant after the local_irq_enable() above.

> + */
> +#include <linux/kernel.h>
> +#include <linux/fs.h>
> +#include <linux/file.h>
> +#include <linux/vfs.h>
> +#include <linux/mount.h>
> +#include <linux/perfmon_kern.h>
> +#include "perfmon_priv.h"
> +
> +#define PFMFS_MAGIC 0xa0b4d889 /* perfmon filesystem magic number */

This should be probably somewhere else.

> + return 1;
> +}
> +__setup("perfmon_debug", enable_debug);

There's a new generic dynamic_printk.h now that might be an alternative.

> +{
> + PFM_LOG("version %u.%u", PFM_VERSION_MAJ, PFM_VERSION_MIN);
> +
> + if (pfm_init_ctx())
> + goto error_disable;
> +
> + if (pfm_init_fs())
> + goto error_disable;
> +
> + /*
> + * one time, arch-specific global initialization
> + */
> + if (pfm_arch_init())
> + goto error_disable;
> +
> + return 0;
> +
> +error_disable:
> + PFM_ERR("perfmon is disabled due to initialization error");
> + perfmon_disabled = 1;
> + return -1;

I presume failure is very unlikely, but this has leaks under various
error conditions, hasn't it?

If they are very unlikely a comment that you don't handle it
intentionally is enough, but at least the comment if not
proper code should be there.

> +{
> +#ifdef CONFIG_SMP
> + struct pfm_context *ctxp;
> +
> + ctxp = __get_cpu_var(pmu_ctx);
> + if (!ctxp)
> + return;
> + /*
> + * in UP per-thread, due to lazy save

But that's in CONFIG_SMP?

Also that assumption might be not true on !x86?

> + * there could be a context from another
> + * task. We need to push it first before
> + * installing our new state
> + */
> + pfm_save_pmds(ctxp);
> + /*
> + * do not clear ownership because we rewrite
> + * right away
> + */
> +#endif
> +}
> + * global information about all sessions
> + */
> +struct pfm_resources {
> + cpumask_t sys_cpumask; /* bitmask of used cpus */
> + u32 thread_sessions; /* #num loaded per-thread sessions */
> +};
> +
> +static struct pfm_resources pfm_res;
> +
> +static __cacheline_aligned_in_smp DEFINE_SPINLOCK(pfm_res_lock);

Would it make sense to share other global perfmon context
on the same cache line?

> + */
> + spin_lock_irqsave(&pfm_res_lock, flags);
> +
> + PFM_DBG("in thread=%u",
> + pfm_res.thread_sessions);
> +
> + pfm_res.thread_sessions++;
> +
> + PFM_DBG("out thread=%u ret=%d",
> + pfm_res.thread_sessions,
> + ret);
> +
> + spin_unlock_irqrestore(&pfm_res_lock, flags);

The usual pattern would be to just do this with an atomic
counter. I suppose it wouldn't buy scalability, but perhaps
shorter code.

-Andi

--
ak@linux.intel.com


\
 
 \ /
  Last update: 2008-11-26 12:09    [W:0.081 / U:0.264 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site