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

[Vegard Nossum]

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/