Messages in this thread Patch in this message | | | Date | Thu, 17 Jun 2004 13:00:20 +0100 | From | Christoph Hellwig <> | Subject | Re: [3/4] [PATCH]Diskdump - yet another crash dump function |
| |
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.
- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |