2.6.18-rc5-mm1: {dis,en}able_irq_lockdep_irqrestore compile error

2.6.18-rc5-mm1: {dis,en}able_irq_lockdep_irqrestore compile error

[Adrian Bunk]

lockdep-core-add-enable-disable_irq_irqsave-irqrestore-apis.patch causes 
the following compile error on frv:

<--  snip  -->

...
  LD      .tmp_vmlinux1
drivers/built-in.o: In function `ei_start_xmit':
8390.c:(.text+0x241c8): undefined reference to `disable_irq_nosync_lockdep_irqsave'
8390.c:(.text+0x242a0): undefined reference to `enable_irq_lockdep_irqrestore'
8390.c:(.text+0x2440c): undefined reference to `enable_irq_lockdep_irqrestore'
make: *** [.tmp_vmlinux1] Error 1

<--  snip  -->

cu
Adrian

-- 

       "Is there not promise of rain?" Ling Tan asked suddenly out
        of the darkness. There had been need of rain for many days.
       "Only a promise," Lao Er said.
                                       Pearl S. Buck - Dragon Seed

[PATCH] FRV: Fix {dis,en}able_irq_lockdep_irqrestore compile error

[David Howells]

Fix the lack of certain non-LOCKDEP stub functions in linux/interrupt.h and
also provide FRV with LOCKDEP variants.

This is to be applied to -mm kernel since not all of the functions added exist
in the main kernel.

Signed-Off-By: David Howells <dhowells@redhat.com>
---
warthog>diffstat -p1 frv-irq-lockdep-2618rc5mm1.diff 
 include/asm-frv/irq.h     |   43 +++++++++++++++++++++++++++++++++++++++++++
 include/linux/interrupt.h |    2 ++
 2 files changed, 45 insertions(+)

diff -urp ../kernels/linux-2.6.18-rc5-mm1/include/asm-frv/irq.h linux-2.6.18-rc5-mm1-frv/include/asm-frv/irq.h
--- ../kernels/linux-2.6.18-rc5-mm1/include/asm-frv/irq.h	2006-09-04 18:02:48.000000000 +0100
+++ linux-2.6.18-rc5-mm1-frv/include/asm-frv/irq.h	2006-09-05 15:59:08.000000000 +0100
@@ -39,5 +39,48 @@ extern void disable_irq_nosync(unsigned 
 extern void disable_irq(unsigned int irq);
 extern void enable_irq(unsigned int irq);
 
+#ifdef CONFIG_LOCKDEP
+/*
+ * Special lockdep variants of irq disabling/enabling.
+ * These should be used for locking constructs that
+ * know that a particular irq context which is disabled,
+ * and which is the only irq-context user of a lock,
+ * that it's safe to take the lock in the irq-disabled
+ * section without disabling hardirqs.
+ *
+ * On !CONFIG_LOCKDEP they are equivalent to the normal
+ * irq disable/enable methods.
+ */
+static inline void disable_irq_nosync_lockdep(unsigned int irq)
+{
+	disable_irq_nosync(irq);
+	local_irq_disable();
+}
+
+static inline void disable_irq_nosync_lockdep_irqsave(unsigned int irq, unsigned long *flags)
+{
+	disable_irq_nosync(irq);
+	local_irq_save(*flags);
+}
+
+static inline void disable_irq_lockdep(unsigned int irq)
+{
+	disable_irq(irq);
+	local_irq_disable();
+}
+
+static inline void enable_irq_lockdep(unsigned int irq)
+{
+	local_irq_enable();
+	enable_irq(irq);
+}
+
+static inline void enable_irq_lockdep_irqrestore(unsigned int irq, unsigned long *flags)
+{
+	local_irq_restore(*flags);
+	enable_irq(irq);
+}
+#endif /* CONFIG_LOCKDEP */
+
 
 #endif /* _ASM_IRQ_H_ */
diff -urp ../kernels/linux-2.6.18-rc5-mm1/include/linux/interrupt.h linux-2.6.18-rc5-mm1-frv/include/linux/interrupt.h
--- ../kernels/linux-2.6.18-rc5-mm1/include/linux/interrupt.h	2006-09-04 18:03:31.000000000 +0100
+++ linux-2.6.18-rc5-mm1-frv/include/linux/interrupt.h	2006-09-05 15:58:53.000000000 +0100
@@ -178,6 +178,8 @@ static inline int disable_irq_wake(unsig
 #  define disable_irq_nosync_lockdep(irq)	disable_irq_nosync(irq)
 #  define disable_irq_lockdep(irq)		disable_irq(irq)
 #  define enable_irq_lockdep(irq)		enable_irq(irq)
+#  define disable_irq_nosync_lockdep_irqsave(irq, flags) disable_irq_nosync(irq)
+#  define enable_irq_lockdep_irqrestore(irq, flags) enable_irq(irq)
 # endif
 
 #endif /* CONFIG_GENERIC_HARDIRQS */

Re: [PATCH] FRV: Fix {dis,en}able_irq_lockdep_irqrestore compile error

[Ingo Molnar]

* David Howells <dhowells@redhat.com> wrote:

> Fix the lack of certain non-LOCKDEP stub functions in 
> linux/interrupt.h and also provide FRV with LOCKDEP variants.
> 
> This is to be applied to -mm kernel since not all of the functions 
> added exist in the main kernel.
> 
> Signed-Off-By: David Howells <dhowells@redhat.com>

Acked-by: Ingo Molnar <mingo@elte.hu>

	Ingo

[PATCH] NOMMU: Move the fallback arch_vma_name() to a sensible place

[David Howells]

Move the fallback arch_vma_name() to a sensible place (kernel/signal.c).

Currently it's in fs/proc/task_mmu.c, a file that is dependent on both
CONFIG_PROC_FS and CONFIG_MMU being enabled, but it's used from kernel/signal.c
from where it is called unconditionally.

Signed-Off-By: David Howells <dhowells@redhat.com>
---
warthog>diffstat -p1 nommu-arch_vma_name-2618rc5mm1.diff 
 fs/proc/task_mmu.c |    5 -----
 kernel/signal.c    |    5 +++++
 2 files changed, 5 insertions(+), 5 deletions(-)

diff -urp ../kernels/linux-2.6.18-rc5-mm1/fs/proc/task_mmu.c linux-2.6.18-rc5-mm1-frv/fs/proc/task_mmu.c
--- ../kernels/linux-2.6.18-rc5-mm1/fs/proc/task_mmu.c	2006-09-04 18:02:43.000000000 +0100
+++ linux-2.6.18-rc5-mm1-frv/fs/proc/task_mmu.c	2006-09-05 15:49:18.000000000 +0100
@@ -122,11 +122,6 @@ struct mem_size_stats
 	unsigned long private_dirty;
 };
 
-__attribute__((weak)) const char *arch_vma_name(struct vm_area_struct *vma)
-{
-	return NULL;
-}
-
 static int show_map_internal(struct seq_file *m, void *v, struct mem_size_stats *mss)
 {
 	struct proc_maps_private *priv = m->private;
diff -urp ../kernels/linux-2.6.18-rc5-mm1/kernel/signal.c linux-2.6.18-rc5-mm1-frv/kernel/signal.c
--- ../kernels/linux-2.6.18-rc5-mm1/kernel/signal.c	2006-09-04 18:03:32.000000000 +0100
+++ linux-2.6.18-rc5-mm1-frv/kernel/signal.c	2006-09-05 15:49:19.000000000 +0100
@@ -773,6 +773,11 @@ static void pad_len_spaces(int len)
 	printk("%*c", len, ' ');
 }
 
+__attribute__((weak)) const char *arch_vma_name(struct vm_area_struct *vma)
+{
+	return NULL;
+}
+
 static int print_vma(struct vm_area_struct *vma)
 {
 	struct mm_struct *mm = vma->vm_mm;

[PATCH] NOMMU: Provide page_mkclean() for NOMMU

[David Howells]

Provide a page_mkclean() implementation for NOMMU.  This doesn't do anything
except return successfully as there are no PTEs for it to play with.

This is only relevant to the -mm kernels.

Signed-Off-By: David Howells <dhowells@redhat.com>
---
warthog>diffstat -p1 nommu-page_mkclean-2618rc5mm1.diff 
 include/linux/rmap.h |    6 ++++++
 1 file changed, 6 insertions(+)

diff -urp ../kernels/linux-2.6.18-rc5-mm1/include/linux/rmap.h linux-2.6.18-rc5-mm1-frv/include/linux/rmap.h
--- ../kernels/linux-2.6.18-rc5-mm1/include/linux/rmap.h	2006-09-04 18:03:32.000000000 +0100
+++ linux-2.6.18-rc5-mm1-frv/include/linux/rmap.h	2006-09-05 15:34:35.000000000 +0100
@@ -120,6 +120,12 @@ int page_mkclean(struct page *);
 #define page_referenced(page,l) TestClearPageReferenced(page)
 #define try_to_unmap(page, refs) SWAP_FAIL
 
+static inline int page_mkclean(struct page *page)
+{
+	return 0;
+}
+
+
 #endif	/* CONFIG_MMU */
 
 /*

[PATCH] NOMMU: Make lib/ioremap.c conditional

[David Howells]

Make lib/ioremap.c conditional on !CONFIG_MMU.  It plays with PTEs which don't
exist under NOMMU conditions.

Signed-Off-By: David Howells <dhowells@redhat.com>
---
warthog>diffstat -p1 nommu-ioremap-2618rc5mm1.diff 
 lib/Makefile |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff -urp ../kernels/linux-2.6.18-rc5-mm1/lib/Makefile linux-2.6.18-rc5-mm1-frv/lib/Makefile
--- ../kernels/linux-2.6.18-rc5-mm1/lib/Makefile	2006-09-04 18:03:32.000000000 +0100
+++ linux-2.6.18-rc5-mm1-frv/lib/Makefile	2006-09-05 16:01:38.000000000 +0100
@@ -5,8 +5,9 @@
 lib-y := ctype.o string.o vsprintf.o cmdline.o \
 	 bust_spinlocks.o rbtree.o radix-tree.o dump_stack.o \
 	 idr.o div64.o int_sqrt.o bitmap.o extable.o prio_tree.o \
-	 sha1.o ioremap.o
+	 sha1.o
 
+lib-$(CONFIG_MMU) += ioremap.o
 lib-$(CONFIG_SMP) += cpumask.o
 
 lib-y	+= kobject.o kref.o kobject_uevent.o klist.o

[PATCH] FRV: do_gettimeofday() should no longer use tickadj

[David Howells]

Stop do_gettimeofday() on FRV from using tickadj, and model it after ARM
instead.

This patch also provides a placeholder macro for getting hardware timer data to
be filled in when such is available.

Signed-Off-By: David Howells <dhowells@redhat.com>
---
warthog>diffstat -p1 frv-tickadj-2618rc5mm1.diff 
 arch/frv/kernel/time.c |   20 +++++---------------
 1 file changed, 5 insertions(+), 15 deletions(-)

diff -urp ../kernels/linux-2.6.18-rc5-mm1/arch/frv/kernel/time.c linux-2.6.18-rc5-mm1-frv/arch/frv/kernel/time.c
--- ../kernels/linux-2.6.18-rc5-mm1/arch/frv/kernel/time.c	2006-09-04 18:03:14.000000000 +0100
+++ linux-2.6.18-rc5-mm1-frv/arch/frv/kernel/time.c	2006-09-05 15:44:42.000000000 +0100
@@ -31,6 +31,9 @@
 
 #define TICK_SIZE (tick_nsec / 1000)
 
+/* H/W clock data if we can get it (in microseconds) */
+#define FRV_HW_CLOCK_DATA (0)
+
 unsigned long __nongprelbss __clkin_clock_speed_HZ;
 unsigned long __nongprelbss __ext_bus_clock_speed_HZ;
 unsigned long __nongprelbss __res_bus_clock_speed_HZ;
@@ -148,23 +151,10 @@ void do_gettimeofday(struct timeval *tv)
 {
 	unsigned long seq;
 	unsigned long usec, sec;
-	unsigned long max_ntp_tick;
 
 	do {
 		seq = read_seqbegin(&xtime_lock);
-
-		usec = 0;
-
-		/*
-		 * If time_adjust is negative then NTP is slowing the clock
-		 * so make sure not to go into next possible interval.
-		 * Better to lose some accuracy than have time go backwards..
-		 */
-		if (unlikely(time_adjust < 0)) {
-			max_ntp_tick = (USEC_PER_SEC / HZ) - tickadj;
-			usec = min(usec, max_ntp_tick);
-		}
-
+		usec = FRV_HW_CLOCK_DATA;
 		sec = xtime.tv_sec;
 		usec += (xtime.tv_nsec / 1000);
 	} while (read_seqretry(&xtime_lock, seq));
@@ -195,7 +185,7 @@ int do_settimeofday(struct timespec *tv)
 	 * wall time.  Discover what correction gettimeofday() would have
 	 * made, and then undo it!
 	 */
-	nsec -= 0 * NSEC_PER_USEC;
+	nsec -= FRV_HW_CLOCK_DATA * NSEC_PER_USEC;
 
 	wtm_sec  = wall_to_monotonic.tv_sec + (xtime.tv_sec - sec);
 	wtm_nsec = wall_to_monotonic.tv_nsec + (xtime.tv_nsec - nsec);

Re: [PATCH] FRV: do_gettimeofday() should no longer use tickadj

[john stultz]

On Tue, 2006-09-05 at 16:35 +0100, David Howells wrote:
> Stop do_gettimeofday() on FRV from using tickadj, and model it after ARM
> instead.
> 
> This patch also provides a placeholder macro for getting hardware timer data to
> be filled in when such is available.

>From this patch it looks like the FRV arch could be trivially converted
to GENERIC_TIME.

Would you consider the following, totally untested patch?

Signed-off-by: John Stultz <johnstul@us.ibm.com>

 Kconfig       |    4 ++
 kernel/time.c |   81 ----------------------------------------------------------
 2 files changed, 4 insertions(+), 81 deletions(-)

diff --git a/arch/frv/Kconfig b/arch/frv/Kconfig
index 95a3892..a601a17 100644
--- a/arch/frv/Kconfig
+++ b/arch/frv/Kconfig
@@ -29,6 +29,10 @@ config GENERIC_HARDIRQS
 	bool
 	default n
 
+config GENERIC_TIME
+	bool
+	default y
+
 config TIME_LOW_RES
 	bool
 	default y
diff --git a/arch/frv/kernel/time.c b/arch/frv/kernel/time.c
index d5b64e1..68a77fe 100644
--- a/arch/frv/kernel/time.c
+++ b/arch/frv/kernel/time.c
@@ -32,8 +32,6 @@
 
 #define TICK_SIZE (tick_nsec / 1000)
 
-extern unsigned long wall_jiffies;
-
 unsigned long __nongprelbss __clkin_clock_speed_HZ;
 unsigned long __nongprelbss __ext_bus_clock_speed_HZ;
 unsigned long __nongprelbss __res_bus_clock_speed_HZ;
@@ -145,85 +143,6 @@ void time_init(void)
 }
 
 /*
- * This version of gettimeofday has near microsecond resolution.
- */
-void do_gettimeofday(struct timeval *tv)
-{
-	unsigned long seq;
-	unsigned long usec, sec;
-	unsigned long max_ntp_tick;
-
-	do {
-		unsigned long lost;
-
-		seq = read_seqbegin(&xtime_lock);
-
-		usec = 0;
-		lost = jiffies - wall_jiffies;
-
-		/*
-		 * If time_adjust is negative then NTP is slowing the clock
-		 * so make sure not to go into next possible interval.
-		 * Better to lose some accuracy than have time go backwards..
-		 */
-		if (unlikely(time_adjust < 0)) {
-			max_ntp_tick = (USEC_PER_SEC / HZ) - tickadj;
-			usec = min(usec, max_ntp_tick);
-
-			if (lost)
-				usec += lost * max_ntp_tick;
-		}
-		else if (unlikely(lost))
-			usec += lost * (USEC_PER_SEC / HZ);
-
-		sec = xtime.tv_sec;
-		usec += (xtime.tv_nsec / 1000);
-	} while (read_seqretry(&xtime_lock, seq));
-
-	while (usec >= 1000000) {
-		usec -= 1000000;
-		sec++;
-	}
-
-	tv->tv_sec = sec;
-	tv->tv_usec = usec;
-}
-
-EXPORT_SYMBOL(do_gettimeofday);
-
-int do_settimeofday(struct timespec *tv)
-{
-	time_t wtm_sec, sec = tv->tv_sec;
-	long wtm_nsec, nsec = tv->tv_nsec;
-
-	if ((unsigned long)tv->tv_nsec >= NSEC_PER_SEC)
-		return -EINVAL;
-
-	write_seqlock_irq(&xtime_lock);
-	/*
-	 * This is revolting. We need to set "xtime" correctly. However, the
-	 * value in this location is the value at the most recent update of
-	 * wall time.  Discover what correction gettimeofday() would have
-	 * made, and then undo it!
-	 */
-	nsec -= 0 * NSEC_PER_USEC;
-	nsec -= (jiffies - wall_jiffies) * TICK_NSEC;
-
-	wtm_sec  = wall_to_monotonic.tv_sec + (xtime.tv_sec - sec);
-	wtm_nsec = wall_to_monotonic.tv_nsec + (xtime.tv_nsec - nsec);
-
-	set_normalized_timespec(&xtime, sec, nsec);
-	set_normalized_timespec(&wall_to_monotonic, wtm_sec, wtm_nsec);
-
-	ntp_clear();
-	write_sequnlock_irq(&xtime_lock);
-	clock_was_set();
-	return 0;
-}
-
-EXPORT_SYMBOL(do_settimeofday);
-
-/*
  * Scheduler clock - returns current time in nanosec units.
  */
 unsigned long long sched_clock(void)

Re: [PATCH] FRV: do_gettimeofday() should no longer use tickadj

[David Howells]

john stultz <johnstul@us.ibm.com> wrote:

> From this patch it looks like the FRV arch could be trivially converted
> to GENERIC_TIME.
> 
> Would you consider the following, totally untested patch?

It certainly looks interesting.  I'll have to study the clocksource stuff -
some FRV CPUs have an effective TSC.

David

Re: [PATCH] FRV: do_gettimeofday() should no longer use tickadj

[Ingo Molnar]

* David Howells <dhowells@redhat.com> wrote:

> john stultz <johnstul@us.ibm.com> wrote:
> 
> > From this patch it looks like the FRV arch could be trivially 
> > converted to GENERIC_TIME.
> > 
> > Would you consider the following, totally untested patch?
> 
> It certainly looks interesting.  I'll have to study the clocksource 
> stuff - some FRV CPUs have an effective TSC.

btw., would be nice to convert it to genirq (and irqchips) too =B-) That 
would solve the kind of disable_irq_lockdep() breakage that was reported 
recently.

	Ingo

Re: [PATCH] FRV: do_gettimeofday() should no longer use tickadj

[David Howells]

Ingo Molnar <mingo@elte.hu> wrote:

> btw., would be nice to convert it to genirq (and irqchips) too =B-) That 
> would solve the kind of disable_irq_lockdep() breakage that was reported 
> recently.

I can think of reasons for not using that stuff also.

 (1) Passing "struct pt_regs *regs" around is a complete waste of resources on
     FRV.  It's in GR28 at all times and can thus be accessed directly.

 (2) All the little operations functions cause unnecessary jumping, jumps that
     icache lookahead can't predict because they're register-indirect.

 (3) ACK'ing and controlling interrupts has to be done by groups.

 (4) No account is taken of interrupt priority.

 (5) The FRV CPU doesn't tell me which IRQ source fired.  Much of the code
     I've got is stuff to try and work it out.  I could just blindly poll all
     the sources attached to a particular interrupt level, but that seems
     somehow less efficient.

David

BTW, have you looked at my patch to fix lockdep yet?

Re: [PATCH] FRV: do_gettimeofday() should no longer use tickadj

[Ingo Molnar]

* David Howells <dhowells@redhat.com> wrote:

> Ingo Molnar <mingo@elte.hu> wrote:
> 
> > btw., would be nice to convert it to genirq (and irqchips) too =B-) That 
> > would solve the kind of disable_irq_lockdep() breakage that was reported 
> > recently.
> 
> I can think of reasons for not using that stuff also.
> 
>  (1) Passing "struct pt_regs *regs" around is a complete waste of 
>      resources on FRV.  It's in GR28 at all times and can thus be 
>      accessed directly.

we'll get rid of that pt_regs thing centrally, from all drivers at once 
- there's upstream buy-in for that already, and Thomas already generated 
a test-patch for that a few months ago. But it's not a big issue right 
now.

>  (2) All the little operations functions cause unnecessary jumping, 
>      jumps that icache lookahead can't predict because they're 
>      register-indirect.

this shouldnt be a big issue either - we use indirect jumps all around 
the kernel. CPUs are either smart enough to predict it, or they simply 
dont have that high penalties. (and if they have high penalties but dont 
optimize for it then genirq will be the last of their worries. The VFS 
and the MM is full of function pointers.)

>  (3) ACK'ing and controlling interrupts has to be done by groups.

please be more specific, how is this not possible via genirq?

>  (4) No account is taken of interrupt priority.

hm, i'm not sure what you mean - could you be more specific?

>  (5) The FRV CPU doesn't tell me which IRQ source fired.  Much of the 
>      code I've got is stuff to try and work it out.  I could just 
>      blindly poll all the sources attached to a particular interrupt 
>      level, but that seemssomehow less efficient.

but ... somehow the current FRV code does figure out which IRQ source 
fired, right? How is that a hindrance for a genirq/irqchips based 
design?

> BTW, have you looked at my patch to fix lockdep yet?

oops, i missed it - just acked it. (This problem too was a side-effect 
of FRV having its own IRQ layer.)

	Ingo

Re: [PATCH] FRV: do_gettimeofday() should no longer use tickadj

[David Howells]

Ingo Molnar <mingo@elte.hu> wrote:

> we'll get rid of that pt_regs thing centrally, from all drivers at once 
> - there's upstream buy-in for that already, and Thomas already generated 
> a test-patch for that a few months ago. But it's not a big issue right 
> now.

Yay!  Can you give me a pointer to the patch?

> this shouldnt be a big issue either - we use indirect jumps all around 
> the kernel.

Yes, I know.  I'm sometimes concerned at just how fast indirect jumps (and even
direct calls) are proliferating.  Look at the read syscall path for something
like ext3 these days: it's like a pile of spaghetti.  That seems particularly
true of direct-IO where it seems to weave in and out of core code and the
filesystem as it goes down.  I'm also concerned about stack usage.

> CPUs are either smart enough to predict it

I was told a while back (2002?) not to use indirect pointers for some stuff
because CPUs _couldn't_ predict it.  Maybe this has changed in modern CPUs.

> >  (3) ACK'ing and controlling interrupts has to be done by groups.
> 
> please be more specific,

Under some circumstances I can work out which sources have triggered which
interrupts (there are various off-CPU FPGAs which implement auxiliary PICs that
do announce their sources), but the aux-PIC channels are grouped together upon
delivery to the CPU PIC, so some of the ACK'ing has to be done at the group
level.

> how is this not possible via genirq?

How is it possible with genirq?

Unless I tie all the grouped sources together into one virtual IRQ line, this
doesn't appear to be possible.  But doing that I might then also have a mixed
set of "flow" types in any particular IRQ.

> >  (4) No account is taken of interrupt priority.
> 
> hm, i'm not sure what you mean - could you be more specific?

The FRV CPU, like many others, supports interrupt prioritisation.  A particular
interrupt level is set in the PSR, and any interrupt of a higher priority can
interrupt.  do_IRQ() can then do the interrupt processing in the interrupt
level of the interrupt that invoked it, thus permitting higher priority
interrupts to still happen.

> but ... somehow the current FRV code does figure out which IRQ source 
> fired, right?

Not always; sometimes it has to fall back to polling the drivers unfortunately.

Btw why are we using IRQ_INPROGRESS, IRQ_DISABLED, IRQ_PENDING and friends?
They would appear unnecessary.

David

Re: [PATCH] FRV: do_gettimeofday() should no longer use tickadj

[Benjamin Herrenschmidt]

> Under some circumstances I can work out which sources have triggered which
> interrupts (there are various off-CPU FPGAs which implement auxiliary PICs that
> do announce their sources), but the aux-PIC channels are grouped together upon
> delivery to the CPU PIC, so some of the ACK'ing has to be done at the group
> level.
> 
> > how is this not possible via genirq?
> 
> How is it possible with genirq?

Well, genirq gives you more flexibility than the current mecanism so ...

If I understand correctly, you need to do scray stuff to figure out your
toplevel irq, which shound't be a problem with either mecanisms... 

Now, if you have funky cascades, then you can always group them into a
virtual irq cascade line and have a special chained flow handler that
does all the "figuring out" off those... it's up to you. 

In general, I found genirq allowed me to do more fancy stuff, and end up
with actually less hooks and indirect function calls on the path to a
given irq than before as you can use tailored flow handlers that do just
the right thing.

Ben.

Re: [PATCH] FRV: do_gettimeofday() should no longer use tickadj

[David Howells]

Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> Well, genirq gives you more flexibility than the current mecanism so ...

No, it doesn't because the FRV arch contains its own mechanism and I can do
what ever I like in it.

genirq's flexibility comes at a price.  Count the number of hooks in struct
irq_chip and struct irq_desc together.

> If I understand correctly, you need to do scray stuff to figure out your
> toplevel irq, which shound't be a problem with either mecanisms... 

Yeah.  I can't actually find out what source caused top-level IRQs.  I have to
guess from looking at the IRQ priority and poking around in the hardware.  I
got bitten that way too: at one point, I was peeking at the interrupt flag in
the serial regs, only to realise this was causing the driver to go wrong
because it cleared the interrupt requested flag in UART.

Obviously I'd rather not use IRQ priorisation to help multiplex irqs, but
unless I want a large polling set...

> Now, if you have funky cascades, then you can always group them into a
> virtual irq cascade line and have a special chained flow handler that
> does all the "figuring out" off those... it's up to you. 

You make it sound so easy, but it's not obvious how to do this, apart from
installing interrupt handlers for the auxiliary PIC interrupts on the CPU and
having those call back into __do_IRQ().  Chaining isn't mentioned in
genericirq.tmpl.

> In general, I found genirq allowed me to do more fancy stuff, and end up
> with actually less hooks and indirect function calls on the path to a
> given irq than before as you can use tailored flow handlers that do just
> the right thing.

My code in the FRV arch has fewer indirection calls than the genirq code
simply because it doesn't require tables of operations.  I can trace through
it with gdb and see them.

I built all the stuff that genirq does in indirections directly into the
handlers.  It certainly has fewer hooks.

I attempted to convert it over to use genirq, and I came up with some numbers:

The difference in kernel sizes:

	   text    data     bss     dec     hex filename
	1993023   77912  166964 2237899  2225cb vmlinux  [with genirq]
	1986511   76016  167908 2230435  2208a3 vmlinux  [without genirq]

The genirq subdir all wraps up into this:

	  10908    3272      12   14192    3770 kernel/irq/built-in.o
	   1548      64       4    1616     650 arch/frv/kernel/irq.o
	---------------------------------------------------------------------
	  12456    3336      16   15808    3dc0 total

My FRV-specific IRQ handling wraps up into these:

	    480     488       0     968     3c8 arch/frv/kernel/irq-mb93091.o
	   4688      16     520    5224    1468 arch/frv/kernel/irq.o
	   1576    1152      16    2744     ab8 arch/frv/kernel/irq-routing.o
	---------------------------------------------------------------------
	   6744    1656     536    8936    22e8 total

There's a difference in BSS size in the main kernel that I can't account for,
but basically genirq uses 6.3KB more code and 1.8KB more initialised data, but
0.9KB less BSS.  Overall, about 7.2KB more memory.  I can shrink the BSS usage
in the FRV specific version by reducing the amount of space in the IRQ sources
table.

So, again, why _should_ I use the generic IRQ stuff?  It's bigger and very
probably slower than what I already have.

David

Re: [PATCH] FRV: do_gettimeofday() should no longer use tickadj

[Ingo Molnar]

* David Howells <dhowells@redhat.com> wrote:

> The genirq subdir all wraps up into this:
> 
> 	  10908    3272      12   14192    3770 kernel/irq/built-in.o
> 	   1548      64       4    1616     650 arch/frv/kernel/irq.o
> 	---------------------------------------------------------------------
> 	  12456    3336      16   15808    3dc0 total

hm, could you take a look at why that difference happens? Do you make 
use of __do_IRQ()? Do you make use of all the various flow handlers that 
are offered in handle.c? Could you #ifdef out all the functions that are 
unused? The kernel build process doesnt remove them and i havent (yet) 
put them into a library.

Could you please send us the patch that genirq-ifies FRV?

> So, again, why _should_ I use the generic IRQ stuff? [...]

To have shared code between architectures? To make generic API updates 
easier for all of us? To have less cruft in interrupt.h? To not having 
to add last-minute patches to v2.6.18 because some arch defines its own 
IRQ prototypes and a difficult generic feature like irqtrace breaks? To 
get new IRQ subsystem features for free like preemptible irqs, irqpoll 
or SHIRQ debugging?

the same "why should we share code" argument could be made for the VFS 
too. Sharing code has a (small) price most of the time, but it's also 
very much worth it. I think the size increases you are seeing are 
artificial and most of it is not caused by the indirections. If they 
were caused by the indirections i'd probably agree with you.

if your argument were true every arch should run its whole Linux kernel 
in arch/frv, with zero sharing with anyone else. There's always a lot of 
'unnecessary' stuff all around the kernel that is just a hindrance for 
FRV. In reality what makes us stronger is to work together. I dont for a 
minute say that we should overdo code sharing - if it's not possible 
then it must not be forced, but just the pure fact of "more 
indirections" or "what does this bring me _now_" isnt a good enough 
reason i believe - it simply makes _future_ changes easier.

	Ingo

Re: [PATCH] FRV: do_gettimeofday() should no longer use tickadj

[David Howells]

Ingo Molnar <mingo@elte.hu> wrote:

> > So, again, why _should_ I use the generic IRQ stuff? [...]
> 
> To have shared code between architectures?

That's reasonable as far as it goes, the algorithms are similar per-arch, but
the PICs are quite ofter quite different.  My FRV board here has three very
different ones, none of them compatible with anything else as far as I know.

> To make generic API updates easier for all of us?

That's reasonable.

> To have less cruft in interrupt.h?

That's specious.  The whole point of having arch-specific code is to support
arch-specific stuff.

> To not having to add last-minute patches to v2.6.18 because some arch
> defines its own IRQ prototypes and a difficult generic feature like irqtrace
> breaks?

Specious again.  If whoever it was made the changes got them right in the
first place, then it wouldn't have required a last minute patch for the
LOCKDEP=n case now would it?

If you're going to insist on the genirq stuff being used, than you should take
CONFIG_GENERIC_HARDIRQS away and force everyone else to move to what you've
decided they should use, right?

> To get new IRQ subsystem features for free like preemptible irqs, irqpoll or
> SHIRQ debugging?

That's reasonable, but you don't get necessarily get features for "free" when
you add up the cost of having support there for them.  The features appear for
the subscribed arches automatically, and so do the costs.

> hm, could you take a look at why that difference happens? Do you make 
> use of __do_IRQ()?

I did say I used it.  In fact, as far as I can tell, I have to use it
recursively.  There doesn't seem to be any other way in that's correct.

> Do you make use of all the various flow handlers that are offered in
> handle.c?

Some of them.

> Could you #ifdef out all the functions that are unused? The kernel build
> process doesnt remove them and i havent (yet) put them into a library.

I could get away with commenting out:

	no_action()
	set_irq_wake()
	can_request_irq()
	set_irq_type()
	set_irq_data()
	set_irq_chip_data()
	handle_simple_irq()
	handle_fasteio_irq()
	bits of handle_irq_name() corresponding to the previous two

This results in a small shrinkage of text and a slight increase in the amount
of data used:

	   text    data     bss     dec     hex filename
	1993023   77908  166964 2237895  2225c7 vmlinux [before]
	1991407   77912  166964 2236283  221f7b vmlinux [after]
	---------------------------------------
	   1616      -4       0    1612

The increase in data size is slightly puzzling, but may have something to do
with there being fewer strings in handle_irq_name().  The text decrease is
about 12% of the unmodified total:

	   text    data     bss     dec     hex filename
	  10908    3272      12   14192    3770 kernel/irq/built-in.o
	   1548      64       4    1616     650 arch/frv/kernel/irq.o
	    744     192       0     936     3a8 arch/frv/kernel/irq-mb93091.o
	---------------------------------------
	  13200    3528      16   16744         total

> the same "why should we share code" argument could be made for the VFS too.

That argument doesn't really follow.  We only have one interrupt system in the
kernel, but we have lots of different filesystems.

> Sharing code has a (small) price most of the time, but it's also very much
> worth it. I think the size increases you are seeing are artificial

Artificial in what manner?  I haven't added extra code to genirq to make it
look bad or anything like that.

> and most of it is not caused by the indirections. If they were caused by the
> indirections i'd probably agree with you.

I think most of the size increase is due to the core genirq function set being
large, not the indirections themselves.  There aren't many indirections
implemented in the core set.

The indirected functions exist in the arch code for the most part, and where
they are implemented they are generally very small.  In FRV's case, one lot in
arch/frv/kernel/irq.c for the CPU and one lot in arch/frv/kernel/irq-mb93091.c
or similar for the on-motherboard FPGA.

> if your argument were true every arch should run its whole Linux kernel 
> in arch/frv, with zero sharing with anyone else.

Not really.  eCos manages this more efficiently than Linux, with generally
fewer indirections through the use of macros and inline functions.

At some point you do have to draw a line and do common stuff.  The VFS is
definitely in the common region.  It has little need of arch-specific stuff in
there, and that that it does is quite readily encapsulated in inline functions
where it has little effect on the space.  I'm not entirely convinced that this
applies to interrupt handling though.  That is at the basic level very
arch-dependent.

> There's always a lot of 'unnecessary' stuff all around the kernel that is
> just a hindrance for FRV.

Or any other platform, embedded or otherwise, that doesn't want it.  A lot of
it I can disable - the block layer now, for instance - but some of it I can't
(like interrupt handling).

> In reality what makes us stronger is to work together.  I dont for a minute
> say that we should overdo code sharing

So who defines what is "overdone"?  You seem to have decided that you do.

> - if it's not possible then it must not be forced, but just the pure fact of
> "more indirections" or "what does this bring me _now_" isnt a good enough
> reason i believe - it simply makes _future_ changes easier.

And makes the kernel larger and slower and makes it consume more stack space
and less easy for the compiler to optimise.

David

Re: [PATCH] FRV: do_gettimeofday() should no longer use tickadj

[Benjamin Herrenschmidt]

On Thu, 2006-09-07 at 10:55 +0100, David Howells wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 
> > Well, genirq gives you more flexibility than the current mecanism so ...
> 
> No, it doesn't because the FRV arch contains its own mechanism and I can do
> what ever I like in it.

But genirq allows you to have your own flow handlers... Which does mean
do whatever you like as well

> genirq's flexibility comes at a price.  Count the number of hooks in struct
> irq_chip and struct irq_desc together.

You do not have to implement them all. Some of them are specific to a
given flow. For example, on XICS or MPIC, I only call eoi(), that is a
single hook, using the fasteoi handler. That's actually less than with
the previous generic code.
 
> > If I understand correctly, you need to do scray stuff to figure out your
> > toplevel irq, which shound't be a problem with either mecanisms... 
> 
> Yeah.  I can't actually find out what source caused top-level IRQs.  I have to
> guess from looking at the IRQ priority and poking around in the hardware.  I
> got bitten that way too: at one point, I was peeking at the interrupt flag in
> the serial regs, only to realise this was causing the driver to go wrong
> because it cleared the interrupt requested flag in UART.
> 
> Obviously I'd rather not use IRQ priorisation to help multiplex irqs, but
> unless I want a large polling set...

But neither the old mecanism nor genirq changes nor does anything in the
way of discovering what the toplevel irq is ... They only intervene
after you have found it...

> > Now, if you have funky cascades, then you can always group them into a
> > virtual irq cascade line and have a special chained flow handler that
> > does all the "figuring out" off those... it's up to you. 
> 
> You make it sound so easy, but it's not obvious how to do this, apart from
> installing interrupt handlers for the auxiliary PIC interrupts on the CPU and
> having those call back into __do_IRQ().  Chaining isn't mentioned in
> genericirq.tmpl.

No, you do a chain handler. Look at how I do it in
arch/powerpc/platform/pseries/setup.c for example. It's actually
trivial. You install a special flow handler (which means that there is
very little overhead, almost none, from the toplevel irq to the chained
irq). You can _also_ if you want just install an IRQ handler for the
cascaded controller and call generic_handle_irq (rather than __do_IRQ)
from it, but that has more overhead. A chained handler completely
relaces the flow handler for the cascade, and thus, if you don't need
all of the nits and bits of the other flow handlers for your cascade,
you can speed things up by hooking at that level.

> My code in the FRV arch has fewer indirection calls than the genirq code
> simply because it doesn't require tables of operations.  I can trace through
> it with gdb and see them.
> 
> I built all the stuff that genirq does in indirections directly into the
> handlers.  It certainly has fewer hooks.

genirq allows you to do just that by using custom handlers.

> I attempted to convert it over to use genirq, and I came up with some numbers:
> 
> The difference in kernel sizes:
> 
> 	   text    data     bss     dec     hex filename
> 	1993023   77912  166964 2237899  2225cb vmlinux  [with genirq]
> 	1986511   76016  167908 2230435  2208a3 vmlinux  [without genirq]
> 
> The genirq subdir all wraps up into this:
> 
> 	  10908    3272      12   14192    3770 kernel/irq/built-in.o
> 	   1548      64       4    1616     650 arch/frv/kernel/irq.o
> 	---------------------------------------------------------------------
> 	  12456    3336      16   15808    3dc0 total
> 
> My FRV-specific IRQ handling wraps up into these:
> 
> 	    480     488       0     968     3c8 arch/frv/kernel/irq-mb93091.o
> 	   4688      16     520    5224    1468 arch/frv/kernel/irq.o
> 	   1576    1152      16    2744     ab8 arch/frv/kernel/irq-routing.o
> 	---------------------------------------------------------------------
> 	   6744    1656     536    8936    22e8 total
> 
> There's a difference in BSS size in the main kernel that I can't account for,
> but basically genirq uses 6.3KB more code and 1.8KB more initialised data, but
> 0.9KB less BSS.  Overall, about 7.2KB more memory.  I can shrink the BSS usage
> in the FRV specific version by reducing the amount of space in the IRQ sources
> table.
> 
> So, again, why _should_ I use the generic IRQ stuff?  It's bigger and very
> probably slower than what I already have.

Well, I would have to look precisely at how you did the port to
genirq... But in my case, it's definitely not slower, especially when
cascaded controllers are involved.

Ben.

Re: [PATCH] FRV: do_gettimeofday() should no longer use tickadj

[David Howells]

Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> No, you do a chain handler. Look at how I do it in
> arch/powerpc/platform/pseries/setup.c for example. It's actually
> trivial. You install a special flow handler (which means that there is
> very little overhead, almost none, from the toplevel irq to the chained
> irq). You can _also_ if you want just install an IRQ handler for the
> cascaded controller and call generic_handle_irq (rather than __do_IRQ)
> from it, but that has more overhead. A chained handler completely
> relaces the flow handler for the cascade, and thus, if you don't need
> all of the nits and bits of the other flow handlers for your cascade,
> you can speed things up by hooking at that level.

Please update Documentation/DocBook/genericirq.tmpl.  That doesn't mention it.

David

Re: [PATCH] FRV: do_gettimeofday() should no longer use tickadj

[Benjamin Herrenschmidt]

On Fri, 2006-09-08 at 11:25 +0100, David Howells wrote:
> Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
> 
> > No, you do a chain handler. Look at how I do it in
> > arch/powerpc/platform/pseries/setup.c for example. It's actually
> > trivial. You install a special flow handler (which means that there is
> > very little overhead, almost none, from the toplevel irq to the chained
> > irq). You can _also_ if you want just install an IRQ handler for the
> > cascaded controller and call generic_handle_irq (rather than __do_IRQ)
> > from it, but that has more overhead. A chained handler completely
> > relaces the flow handler for the cascade, and thus, if you don't need
> > all of the nits and bits of the other flow handlers for your cascade,
> > you can speed things up by hooking at that level.
> 
> Please update Documentation/DocBook/genericirq.tmpl.  That doesn't mention it.

I must admit I haven't read the documentation :) I looked at the
code/patches when genirq was posted and did my powerpc implementation
based on my understanding of the code and discussions with Thomas and
Ingo. I'll have a look at the doc next week and see if I can improve it.

Cheers,
Ben.

Re: [PATCH] FRV: do_gettimeofday() should no longer use tickadj

[David Howells]

Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> > Please update Documentation/DocBook/genericirq.tmpl.  That doesn't mention it.
> 
> I must admit I haven't read the documentation :) I looked at the
> code/patches when genirq was posted and did my powerpc implementation
> based on my understanding of the code and discussions with Thomas and
> Ingo. I'll have a look at the doc next week and see if I can improve it.

While you're at it, you should also encomment pseries_8259_cascade() which is
what I suspect you're referring to in the powerpc sources.

David

Re: [PATCH] FRV: do_gettimeofday() should no longer use tickadj

[David Howells]

Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:

> > > Now, if you have funky cascades, then you can always group them into a
> > > virtual irq cascade line and have a special chained flow handler that
> > > does all the "figuring out" off those... it's up to you. 
> > 
> > You make it sound so easy, but it's not obvious how to do this, apart from
> > installing interrupt handlers for the auxiliary PIC interrupts on the CPU and
> > having those call back into __do_IRQ().  Chaining isn't mentioned in
> > genericirq.tmpl.
> 
> No, you do a chain handler. Look at how I do it in
> arch/powerpc/platform/pseries/setup.c for example. It's actually
> trivial. You install a special flow handler (which means that there is
> very little overhead, almost none, from the toplevel irq to the chained
> irq). You can _also_ if you want just install an IRQ handler for the
> cascaded controller and call generic_handle_irq (rather than __do_IRQ)
> from it, but that has more overhead. A chained handler completely
> relaces the flow handler for the cascade, and thus, if you don't need
> all of the nits and bits of the other flow handlers for your cascade,
> you can speed things up by hooking at that level.

But funky cascading using chained flow handlers doesn't work if the cascade
must share an IRQ with some other device, right?

David

Re: [PATCH] FRV: do_gettimeofday() should no longer use tickadj

[Benjamin Herrenschmidt]

> But funky cascading using chained flow handlers doesn't work if the cascade
> must share an IRQ with some other device, right?

Indeed. Best way there is then to have a normal action handler like you
do and have it call generic_handle_irq() on the cascaded interrupts.

Ben.

Re: [PATCH] FRV: do_gettimeofday() should no longer use tickadj

[Ingo Molnar]

* David Howells <dhowells@redhat.com> wrote:

> Ingo Molnar <mingo@elte.hu> wrote:
> 
> > we'll get rid of that pt_regs thing centrally, from all drivers at once 
> > - there's upstream buy-in for that already, and Thomas already generated 
> > a test-patch for that a few months ago. But it's not a big issue right 
> > now.
> 
> Yay!  Can you give me a pointer to the patch?

i cannot find Thomas' recent 2.6 one (Thomas, do you have a link to 
it?), but i did one 5 years ago:

 http://people.redhat.com/mingo/irq-rewrite-patches/irq-cleanup-2.4.15-B1.bz2

in general it's a large but otherwise pretty dumb patch.

> > this shouldnt be a big issue either - we use indirect jumps all around 
> > the kernel.
> 
> Yes, I know.  I'm sometimes concerned at just how fast indirect jumps 
> (and even direct calls) are proliferating.  Look at the read syscall 
> path for something like ext3 these days: it's like a pile of 
> spaghetti.  That seems particularly true of direct-IO where it seems 
> to weave in and out of core code and the filesystem as it goes down.  
> I'm also concerned about stack usage.

yeah - but unless you can suggest some low-maintainance-overhead 
solution, not much can be done i suspect: being a few cycles slower is a 
lot less of a problem than being less flexible in the design. In general 
CPUs do optimize this quite well, but it is true that not every CPU 
does.

> > CPUs are either smart enough to predict it
> 
> I was told a while back (2002?) not to use indirect pointers for some 
> stuff because CPUs _couldn't_ predict it.  Maybe this has changed in 
> modern CPUs.

indirect pointers are very common both in OSs and in applications, 
especially in C++ based ones, where lots of execution goes off dynamic 
objects which have function pointers associated to them. So _lots_ of 
effort goes into branch prediction on the hardware side - and yes, 
modern CPUs do quite well with indirect pointers too.

The worst-case scenario is when the indirect branch flip-flops between 
multiple destination addresses - but that shouldnt be an issue for 
genirq because most systems have _one_ preferred way of handling 
interrupts that the majority of interrupt traffic uses. (for example on 
i686 it's level-triggered PCI irqs)

But even if there's multiple destinations from the indirect jump, newest 
CPUs (such as Core 2) can actually store _multiple_ branch history 
targets and can prefetch all of them at once (if there's idle capacity 
left).

(And i wouldnt be surprised if some modern CPUs already stored the 
indirection register's index in the BHT, and used that for the 
prediction. Most indirect calls happen off registers, and if the 
compiler loads the register early enough (which it typically does) then 
the branch target value is available to the CPU. Other context 
information can be included in a BHT too.)

Also, in general, if something is arguably a smart thing to do in an OS 
(and more design flexibility via function pointers is a smart thing for 
which there is no viable alternative), we can expect CPUs to get 
gradually better at handling them.

> > >  (4) No account is taken of interrupt priority.
> > 
> > hm, i'm not sure what you mean - could you be more specific?
> 
> The FRV CPU, like many others, supports interrupt prioritisation.  A 
> particular interrupt level is set in the PSR, and any interrupt of a 
> higher priority can interrupt.  do_IRQ() can then do the interrupt 
> processing in the interrupt level of the interrupt that invoked it, 
> thus permitting higher priority interrupts to still happen.

ah, ok. For PREEMPT_HARDIRQS we thought about possibly utilizing 
hw-level IRQ prioritization too - but it's quite inflexible in most IRQ 
controller designs: the prioritization is rarely integrated with the CPU 
and is often attached to the ACK/EOI-ing of the IRQ line (and an 
unACK-ed IRQ can have side-effects).

So the thing we chose for PREEMPT_HARDIRQS was to do the prioritization 
at the OS/scheduler level. And OS level handling of this is what we need 
anyway: IRQ handlers are just the first, often tiny portion in a 
critical workload that a system must perform. (we have softirqs, 
signals, tasks, etc.) Nevertheless the door is open to utilize hw 
capabilities of IRQ prioritization - we 'only' need standard driver and 
/sys APIs to make use of them.

	Ingo

Re: [PATCH] FRV: do_gettimeofday() should no longer use tickadj

[David Howells]

Ingo Molnar <mingo@elte.hu> wrote:

> i cannot find Thomas' recent 2.6 one (Thomas, do you have a link to 
> it?), but i did one 5 years ago:
> 
>  http://people.redhat.com/mingo/irq-rewrite-patches/irq-cleanup-2.4.15-B1.bz2
> 
> in general it's a large but otherwise pretty dumb patch.

I wrote my own patch to test this last Friday.  I found that removing all the
regs pointer passing from the interrupt code reduced interrupt entry with a
warm cache by 1 cpu cycle out of 87, and interrupt exit by 19 cycles out of 99.

I can't tell from that exactly how many instructions/memory accesses have been
removed since the FRV permits two instructions to be executed in one cycle
under some circumstances, and two registers to be stored/loaded in one
instruction.

But the main gain in the exit path has to be due to recovery of the clobbered
regs parameter due to a call inside a loop, possibly in handle_IRQ_event().

I'd expect i386 to do better in cycle reduction because it has fewer registers
and so getting one back should gain more.

David