Messages in this thread | | | Date | Tue, 24 Jan 2006 07:13:25 -0800 | From | Stephane Eranian <> | Subject | Re: [PATCH 2/6] 2.6.16-rc1 perfmon2 patch for review |
| |
Bryan,
On Fri, Jan 20, 2006 at 08:23:09AM -0800, Bryan O'Sullivan wrote: > > > + 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. >
Well, I am not sure why the smp_call_function_single() is not already implemented for i386. I can see that the underlying function send_IPI_mask() is there. It also looks like flush_tlb_others() is also selecting CPUs a subset of CPUs. I am not a big enough expert on x86 to understand if there are gotchas to watch for. Yet it would surprise me if this is radically different than on x86_64 (em64t) which already has the call. Maybe someone can clarify this?
> > + 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.
This is not so pretty, I agree. I rewrote the loop differently now.
--
-Stephane - 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/
| |