Re: [patch 02/24] perfmon: base code

[Andi Kleen <andi@firstfloor.org>]

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