lkml.org 
[lkml]   [2006]   [Jan]   [20]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 2/6] 2.6.16-rc1 perfmon2 patch for review
From
Date
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/

\
 
 \ /
  Last update: 2006-01-20 17:26    [W:0.060 / U:1.132 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site