Re: [PATCH 2/6] 2.6.16-rc1 perfmon2 patch for review
- From: Bryan O'Sullivan <bos@xxxxxxxxxxxxxx>
- 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
- Follow-Ups:
- Re: [PATCH 2/6] 2.6.16-rc1 perfmon2 patch for review
- From: Stephane Eranian
- Re: [PATCH 2/6] 2.6.16-rc1 perfmon2 patch for review
- References:
- [PATCH 2/6] 2.6.16-rc1 perfmon2 patch for review
- From: Stephane Eranian
- [PATCH 2/6] 2.6.16-rc1 perfmon2 patch for review
- Prev by Date: Re: [PATCH 000 of 5] md: Introduction
- Next by Date: Iptables error [Was: 2.6.16-rc1-mm2]
- Previous by thread: [PATCH 2/6] 2.6.16-rc1 perfmon2 patch for review
- Next by thread: Re: [PATCH 2/6] 2.6.16-rc1 perfmon2 patch for review
- Index(es):
Relevant Pages
|
|
