Messages in this thread | | | Date | Fri, 31 Oct 2008 10:38:04 +0100 | From | Ingo Molnar <> | Subject | Re: [PATCH -v2] ring-buffer: add paranoid checks for loops |
| |
* Steven Rostedt <rostedt@goodmis.org> wrote:
> + /* > + * This should normally only loop twice. But because the > + * start of the reader inserts an empty page, it causes > + * a case where we will loop three times. There should be no > + * reason to loop four times (that I know of). > + */ > + if (unlikely(paranoid > 2)) { > + RB_WARN_ON(cpu_buffer, 1); > + reader = NULL; > + goto out; > + } > + paranoid++;
ok, the explanations look nice now.
A small nit - the above comment suggests that looping 4 times is the anomaly - still the test is for paranoid > 2 ?
> + int paranoid = 0;
another small nit: i'd suggest to rename 'paranoid' to 'nr_loops' or 'nr_iterations' or so. It is the _condition_ that signals paranoia, not the variable in itself - making the current patch look a bit weird.
> again: > + /* > + * We repeat when a timestamp is encountered. It is possible > + * to get multiple timestamps from an interrupt entering just > + * as one timestamp is about to be written. The max times > + * that this can happen is the number of nested interrupts we > + * can have. 10 should be more than enough. > + */ > + if (unlikely(paranoid > 10)) { > + RB_WARN_ON(cpu_buffer, 1); > + return NULL;
s/10 should be more than enough/Nesting higher than 10 is clearly anomalous/
> + /* > + * We repeat when a timestamp is encountered. It is possible > + * to get multiple timestamps from an interrupt entering just > + * as one timestamp is about to be written. The max times > + * that this can happen is the number of nested interrupts we > + * can have. 10 should be more than enough. > + */ > + if (unlikely(paranoid > 10)) { > + RB_WARN_ON(cpu_buffer, 1); > + return NULL;
ditto.
Ingo
| |