Messages in this thread | | | Subject | Re: [PATCH 2/6] 2.6.16-rc1 perfmon2 patch for review | From | Bryan O'Sullivan <> | Date | Fri, 20 Jan 2006 08:23:09 -0800 |
| |
On Fri, 2006-01-20 at 07:20 -0800, Stephane Eranian wrote:
> This a split version of the perfmon. Each chunk was split to fit > the constraints of lkml on message size. the patch is relative > to 2.6.16-rc1.
Please keep the generic boilerplate in a [PATCH 0/6] message, and have a descriptive body before each individual message that describes it, not the entire patch series.
Also, perfmon.c is a big source file. It might help reviewers if it got split into several source files that had different logical functions.
> +#if 0 > +static void pfm_map_show(struct pfm_context *ctx) > +{
> +} > +#endif
Why submit dead code?
> + if (ctx->ctx_cpu != smp_processor_id()) { > +#ifdef __i386__ > + /* On IA64 we use smp_call_function_single(), so we > + * should never be called on the wrong CPU. On other > + * archs, that doesn't exist and we use > + * smp_call_function instead, so silently ignore all > + * CPUs except the one we care about. > + */
This looks grotty. Can't you add the necessary arch support, instead of an i386-specific hack with a misleading comment? The block should at least be "#ifndef __ia64__" to match the comment.
> +#ifndef __i386__ > + ret = smp_call_function_single(ctx->ctx_cpu, pfm_syswide_force_stop, > + ctx, 0, 1); > +#else > + ret = smp_call_function(pfm_syswide_force_stop, ctx, 0, 1); > +#endif > + DPRINT(("called CPU%u for cleanup ret=%d\n", ctx->ctx_cpu, ret)); > +} > +#endif /* CONFIG_SMP */
Same "yeugh" response.
> +#ifdef CONFIG_IA64_PERFMON_COMPAT > +/* > + * function providing some help for backward compatiblity with old IA-64 > + * applications.
This should be in a separate source file, whose compilation is conditional on CONFIG_IA64_PERFMON_COMPAT.
> + BUG_ON(ctx->ctx_fl_system == 0 && ctx->ctx_task != current);
What's this intended to catch?
> + DPRINT(("set_id=%u not found\n", set_id)); > +error: > + pfm_retflag_set(req->set_flags, PFM_REG_RETFL_EINVAL); > + return -EINVAL; > +found: > + if (is_loaded && set == ctx->ctx_active_set) > + goto error;
I've seen this style of goto usage in the code a few times, and it's bizarre. Why are you jumping backwards to the error exit? There's nothing wrong with using a goto to exit, it's just more usual to have a single section at the end of the function that has both the error and normal exit paths.
In fact, I see that sometimes you use a backwards goto in the middle to exit, sometimes there's a single hunk at the end with forwards gotos, and sometimes routines just return wherever they feel like it. Please pick one style and stick with it throughout.
<b
- 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/
| |