[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <alpine.LFD.2.00.0811271638350.3325@localhost.localdomain>
Date: Thu, 27 Nov 2008 18:24:58 +0100 (CET)
From: Thomas Gleixner <tglx@...utronix.de>
To: eranian@...glemail.com
cc: linux-kernel@...r.kernel.org, akpm@...ux-foundation.org,
mingo@...e.hu, x86@...nel.org, andi@...stfloor.org,
eranian@...il.com, sfr@...b.auug.org.au
Subject: Re: [patch 02/24] perfmon: base code
On Wed, 26 Nov 2008, eranian@...glemail.com wrote:
> Index: o3/perfmon/perfmon_res.c
> +/*
> + * global information about all sessions
> + */
> +struct pfm_resources {
> + cpumask_t sys_cpumask; /* bitmask of used cpus */
> + u32 thread_sessions; /* #num loaded per-thread sessions */
> +};
What's the purpose of this being a structure if it's just a single
instance ?
> +static struct pfm_resources pfm_res;
> +
> +static __cacheline_aligned_in_smp DEFINE_SPINLOCK(pfm_res_lock);
> +/**
> + * pfm_session_acquire - reserve a per-thread session
> + *
> + * return:
> + * 0 : success
> + * -EBUSY: if conflicting session exist
Where ?
> + */
> +int pfm_session_acquire(void)
> +{
> + unsigned long flags;
> + int ret = 0;
> +
> + /*
> + * validy checks on cpu_mask have been done upstream
> + */
How please ? pfm_res.sys_cpumask is local to this file and you want
to check it under the lock and _before_ you increment
thread_sessions blindly.
> + 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);
> +
> + return ret;
> +}
> +
> +/**
> + * pfm_session_release - release a per-thread session
> + *
> + * called from __pfm_unload_context()
> + */
> +void pfm_session_release(void)
> +{
> + unsigned long flags;
> +
> + 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",
> + pfm_res.thread_sessions);
What's the value of these debugs ? Prove that the compiler managed to
compile "pfm_res.thread_sessions--;" correctly ?
A WARN_ON(!pfm_res.thread_sessions) instead of blindly decrementing
would be way more useful.
> + spin_unlock_irqrestore(&pfm_res_lock, flags);
> +}
<adding the bits from the oprofile patch, which belong here as they
are not x86 specific>
+
+/**
+ * pfm_session_allcpus_acquire - acquire per-cpu sessions on all available cpus
+ *
+ * currently used by Oprofile on X86
+ */
+int pfm_session_allcpus_acquire(void)
+ for_each_online_cpu(cpu) {
+ cpu_set(cpu, pfm_res.sys_cpumask);
+ nsys_cpus++;
+ }
Sigh, why do we need a loop to copy a bitfield ?
+/**
+ * pfm_session_allcpus_release - relase per-cpu sessions on all cpus
+ *
+ * currently used by Oprofile code
+ */
+void pfm_session_allcpus_release(void)
+{
+ unsigned long flags;
+ u32 nsys_cpus, cpu;
+
+ spin_lock_irqsave(&pfm_res_lock, flags);
+
+ nsys_cpus = cpus_weight(pfm_res.sys_cpumask);
+
+ PFM_DBG("in sys=%u task=%u",
+ nsys_cpus,
+ pfm_res.thread_sessions);
+
+ /*
+ * XXX: could use __cpus_clear() with nbits
+ */
__cpus_clear(pfm_res.sys_cpumask, nsys_cpus); ????
That'd be real fun with a sparse mask.
+ for_each_online_cpu(cpu) {
+ cpu_clear(cpu, pfm_res.sys_cpumask);
+ nsys_cpus--;
+ }
Yuck. cpus_clear() perhaps ?
+EXPORT_SYMBOL(pfm_session_allcpus_release);
All what that code should do (in fact it does not) is preventing the
mix of thread and system wide sessions.
It does neither need a cpumask nor tons of useless loops and debug
outputs with zero value.
static int global_session;
static int thread_sessions;
static DEFINE_SPINLOCK(session_lock);
int pfm_session_request(int global)
{
unsigned long flags;
int res = -EBUSY;
spin_lock_irqsave(&session_lock, flags);
if (!global && !global_session) {
thread_sessions++;
res = 0;
}
if (global && !thread_sessions && !global_session) {
global_session = 1;
res = 0;
}
spin_unlock_irqrestore(&session_lock, flags);
return res;
}
void pfm_session_release(int global)
{
unsigned long flags;
spin_lock_irqsave(&session_lock, flags);
if (global) {
WARN_ON(!global_session);
global_session = 0;
} else {
if (!global_session && thread_sessions)
thread_session--;
else
WARN();
}
spin_unlock_irqrestore(&session_lock, flags);
}
Would do it nicely including useful sanity checks and 70% less code.
Thanks,
tglx
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists