Re: [3/4] [PATCH]Diskdump - yet another crash dump function

[Christoph Hellwig <hch@infradead.org>]

On Thu, Jun 17, 2004 at 08:34:37PM +0900, Takao Indoh wrote:
> Instead of redefinition, I add hooks into the timer/tasklet routines.
> 
> ex.
> 
>  int mod_timer(struct timer_list *timer, unsigned long expires)
>  {
>  	BUG_ON(!timer->function);
> 
> +	if (crashdump_mode()) {
> +		return diskdump_mod_timer(timer, expires);
> +	}
>  
>  	 check_timer(timer);
> 
> Please see the following patches for details.
> How do you think? 

Looks good in principle, some more comments on the actual patch below:

> diff -Nur linux-2.6.6.org/include/linux/diskdumplib.h linux-2.6.6/include/linux/diskdumplib.h
> --- linux-2.6.6.org/include/linux/diskdumplib.h	1970-01-01 09:00:00.000000000 +0900
> +++ linux-2.6.6/include/linux/diskdumplib.h	2004-06-17 18:28:01.000000000 +0900

Should probably just go to linux/dump.h or dump_priv.h

> diff -Nur linux-2.6.6.org/include/linux/interrupt.h linux-2.6.6/include/linux/interrupt.h
> --- linux-2.6.6.org/include/linux/interrupt.h	2004-06-04 21:22:09.000000000 +0900
> +++ linux-2.6.6/include/linux/interrupt.h	2004-06-17 18:28:01.000000000 +0900
> @@ -7,6 +7,7 @@
>  #include <linux/linkage.h>
>  #include <linux/bitops.h>
>  #include <linux/preempt.h>
> +#include <linux/diskdumplib.h>
>  #include <asm/atomic.h>
>  #include <asm/hardirq.h>
>  #include <asm/ptrace.h>
> @@ -172,6 +173,11 @@
>  
>  static inline void tasklet_schedule(struct tasklet_struct *t)
>  {
> +	if (crashdump_mode()) {
> +		diskdump_tasklet_schedule(t);
> +		return;
> +	}
> +
>  	if (!test_and_set_bit(TASKLET_STATE_SCHED, &t->state))
>  		__tasklet_schedule(t);
>  }

Please sprinclde unlikely's all over here.  Also the above could be
shortened to

static inline void tasklet_schedule(struct tasklet_struct *t)
{
	if (unlikely(crashdump_mode()))
		diskdump_tasklet_schedule(t);
	else if (!test_and_set_bit(TASKLET_STATE_SCHED, &t->state))
		__tasklet_schedule(t);
}

> +int diskdump_schedule_work(struct work_struct *work)
> +{
> +	list_add_tail(&work->entry, &diskdump_workq);
> +	return 1;
> +}

Should probably just inlined, I guess the function call is bigger
than all of list_add_tail.

> +void diskdump_add_timer(struct timer_list *timer)
> +{
> +	timer->base = (void *)1;
> +	list_add(&timer->entry, &diskdump_timers);
> +}

dito.

> +	/* run timers */
> +	list_for_each_safe(t, n, &diskdump_timers) {
> +		timer = list_entry(t, struct timer_list, entry);

list_for_each_entry_safe here please.

> @@ -255,6 +255,10 @@
>  {
>  	BUG_ON(!timer->function);
>  
> +	if (crashdump_mode()) {
> +		return diskdump_mod_timer(timer, expires);
> +	}

please remove the superflous braces here.