Re: [PATCH] kmemcheck: use tasklets instead of timers

From: Vegard Nossum
Date: Fri May 23 2008 - 16:17:20 EST


Oops. Big mistake on my part. This patch was simply wrong :-(

On Wed, May 21, 2008 at 11:37 PM, Vegard Nossum <vegard.nossum@xxxxxxxxx> wrote:
> From: Vegard Nossum <vegard.nossum@xxxxxxxxx>
> Date: Wed, 21 May 2008 22:58:37 +0200
> Subject: [PATCH] kmemcheck: use tasklets instead of timers
>
> Instead of triggering a timer every HZ, we use the new tasklet function
> which guarantuees not to touch any other tasklets on the tasklet list,
> and is thus safe to use from the page fault handler.
>
> Signed-off-by: Vegard Nossum <vegard.nossum@xxxxxxxxx>
> ---
> arch/x86/kernel/kmemcheck.c | 16 ++++++++--------
> 1 files changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/arch/x86/kernel/kmemcheck.c b/arch/x86/kernel/kmemcheck.c
> index def1a08..c0045e8 100644
> --- a/arch/x86/kernel/kmemcheck.c
> +++ b/arch/x86/kernel/kmemcheck.c
> @@ -9,6 +9,7 @@
> */
>
> #include <linux/init.h>
> +#include <linux/interrupt.h>
> #include <linux/kallsyms.h>
> #include <linux/kdebug.h>
> #include <linux/kernel.h>
> @@ -18,7 +19,6 @@
> #include <linux/page-flags.h>
> #include <linux/percpu.h>
> #include <linux/stacktrace.h>
> -#include <linux/timer.h>
>
> #include <asm/cacheflush.h>
> #include <asm/kmemcheck.h>
> @@ -73,8 +73,6 @@ static unsigned int error_rd;
> static unsigned int error_wr;
> static unsigned int error_missed_count;
>
> -static struct timer_list kmemcheck_timer;
> -
> static struct kmemcheck_error *error_next_wr(void)
> {
> struct kmemcheck_error *e;
> @@ -105,6 +103,9 @@ static struct kmemcheck_error *error_next_rd(void)
> return e;
> }
>
> +static void do_wakeup(unsigned long);
> +static DECLARE_TASKLET(kmemcheck_tasklet, &do_wakeup, 0);
> +
> static void *address_get_shadow(unsigned long address);
>
> /*
> @@ -148,6 +149,8 @@ static void error_save(enum shadow state,
> BUG_ON(!shadow_copy);
>
> memcpy(e->shadow_copy, shadow_copy, SHADOW_COPY_SIZE);
> +
> + tasklet_hi_schedule(&kmemcheck_tasklet);

This should of course say tasklet_hi_schedule_first(), which was the
new function I introduced in the previous patch. Duh.

> }
>
> /*
> @@ -170,6 +173,8 @@ static void error_save_bug(struct pt_regs *regs)
> e->trace.max_entries = ARRAY_SIZE(e->trace_entries);
> e->trace.skip = 1;
> save_stack_trace(&e->trace);
> +
> + tasklet_hi_schedule(&kmemcheck_tasklet);

The same here...

> }
>
> static void error_recall(void)
> @@ -233,8 +238,6 @@ static void do_wakeup(unsigned long data)
> "the queue was too small\n", error_missed_count);
> error_missed_count = 0;
> }
> -
> - mod_timer(&kmemcheck_timer, kmemcheck_timer.expires + HZ);
> }
>
> void __init kmemcheck_init(void)
> @@ -250,9 +253,6 @@ void __init kmemcheck_init(void)
> setup_max_cpus = 1;
> }
> #endif
> -
> - setup_timer(&kmemcheck_timer, &do_wakeup, 0);
> - mod_timer(&kmemcheck_timer, jiffies + HZ);
> }
>
> #ifdef CONFIG_KMEMCHECK_DISABLED_BY_DEFAULT
> --
> 1.5.4.1
>
>

But even so, tasklet_hi_schedule_first() will call
raise_softirq_irqoff() which will call wakeup_softirqd() which will
call wake_up_process() which will do all sorts of badness like
accessing runqueues, etc., etc.

So forget all about the tasklet patches :-(

But thanks for the reviews... ;-)


Vegard

--
"The animistic metaphor of the bug that maliciously sneaked in while
the programmer was not looking is intellectually dishonest as it
disguises that the error is the programmer's own creation."
-- E. W. Dijkstra, EWD1036
--
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/