Messages in this thread | | | Date | Fri, 6 Mar 2009 15:32:37 +0100 | From | Ingo Molnar <> | Subject | Re: [patch 1/4] x86, bts: add selftest for BTS |
| |
* Markus Metzger <markus.t.metzger@intel.com> wrote:
> +#ifdef CONFIG_X86_DS_SELFTEST > +static int ds_selftest_bts_consistency(const struct bts_trace *trace)
Consider putting this into ds_selftest.c instead of adding a large #ifdef block to ds.c.
> + if (trace->ds.end != > + (char *) trace->ds.begin + (trace->ds.n * trace->ds.size)) {
That extra space character after the type cast is not needed.
> + /* Check a few things which do not belong to this test. > + * They should be covered by other tests. */
Please use the customary comment style of:
/* * Comment ..... * ...... goes here: */
> + index = ((void *) at - trace->ds.begin) / trace->ds.size;
cast.
> +#define DS_SELFTEST_SIZEOF_BUFFER 1021 /* intentionally chose an odd size */ > +static int ds_selftest_bts(void)
We put newlines after definitions.
Also, DS_SELFTEST_SIZEOF_BUFFER is an odd name - why not DS_SELFTEST_BUFFER_SIZE ?
> + tracer = > + ds_request_bts(/* task = */ NULL, buffer, > + DS_SELFTEST_SIZEOF_BUFFER, > + /* ovfl = */ NULL, /* th = */ (size_t)-1, > + BTS_KERNEL);
that looks weird. Why not:
tracer = ds_request_bts(NULL, buffer, DS_SELFTEST_BUFFER_SIZE, NULL, (size_t)-1, BTS_KERNEL);
> + /* The return from the initialization call should already give > + * us enough trace. */
Comment style.
> + /* It is possible but highly unlikely that we got a > + * buffer overflow and end up at exactly the same > + * position we started from. > + * Let's issue a warning, but continue. */
ditto.
> + /* Let's read the trace again. Since we suspended tracing, we should > + * get the same result. */
ditto.
> + /* It is possible but highly unlikely that we got a > + * buffer overflow and end up at exactly the same > + * position we started from. > + * Let's issue a warning and check the full trace. */
ditto.
> + /* We had a buffer overflow - the entire buffer should > + * contain trace records. */
ditto.
> + /* It is quite likely that the buffer did not > + * overflow. Let's just check the delta trace. */
ditto.
> +#ifdef CONFIG_X86_DS_SELFTEST > + if (ds_cfg.ctl[dsf_bts]) { > + int error; > + > + printk(KERN_INFO "[ds] bts selftest..."); > + error = ds_selftest_bts(); > + printk(KERN_CONT "%s.\n", (error ? "failed" : "passed")); > + > + if (error) { > + WARN(1, "[ds] selftest failed. disabling bts.\n"); > + ds_cfg.ctl[dsf_bts] = 0; > + } > + } > +#endif /* CONFIG_X86_DS_SELFTEST */
This bit should be in a helper function i guess - so the ugly #ifdef goes out of a function.
Thanks,
Ingo
| |