[PATCH 1/5] RFC genirq: Add IRQ timestamping

[PATCH 1/5] RFC genirq: Add IRQ timestamping

[Torben Hohn]

This patch adds the IRQF_TIMESTAMP flag to register_threaded_irq()

There are several drivers which record timestamps in their irq handlers.
With the appearance of force_threaded irq handlers these timestamps will see
higher jitter. So by moving the task of taking the timestamp into genirq,
we can mitigate this problem.

Possible users of this:

- PPS is certeinly interested in timestamps as accurate as possible.
  they even tried to implement similar functionality.
  http://kerneltrap.org/mailarchive/linux-kernel/2008/9/10/3285514

- network software timestamps could take advantage of this.

- alsa also takes timestamps.

I am assuming here, that the drivers are actually interested in the time,
when the irq line was risen (Some of them take the timestamps pretty late,
which might be because, before hrtimer, time didnt advance in irq context
anyways, or they dont really care for accuracy, or they are actually
timestamping some operation inside the irq handler)

I am currently storing the timestamp in struct irqdesc and provide
struct timespec irq_get_timestamp( int irq );

Thomas Gleixner proposed to mandate that IRQF_TIMESTAMP irqs need to make
their device_id pointer point to the place where they want to record their
timestamp, and use container_of() to get their device struct then.

I must say i am not a huge fan of container_of() and tacking "obscure"
semantics to void pointers. But if an irq can fire while the handler thread
is still running, this might be our only choice.

However... this would also need to be supported in the subsystems.
iE if snd_usbaudio wants nice timestamps on its irqs. USB subsys would
need to allow it to request timestamps, and pass them on somehow.
So we probaby need functions to dynamically switch timestamping on and off.

This will be addressed in following patches.

Signed-off-by: Torben Hohn <torbenh@gmx.de>
---
 arch/arm/mach-ns9xxx/irq.c              |    3 +++
 arch/m68knommu/platform/5272/intc.c     |    4 ++++
 arch/powerpc/platforms/cell/interrupt.c |    3 +++
 include/linux/interrupt.h               |    1 +
 include/linux/irq.h                     |    1 +
 include/linux/irqdesc.h                 |    1 +
 kernel/irq/chip.c                       |   15 +++++++++++++++
 kernel/irq/manage.c                     |   17 +++++++++++++++++
 kernel/irq/spurious.c                   |    4 ++++
 9 files changed, 49 insertions(+), 0 deletions(-)

diff --git a/arch/arm/mach-ns9xxx/irq.c b/arch/arm/mach-ns9xxx/irq.c
index 389fa5c..d3d8a98 100644
--- a/arch/arm/mach-ns9xxx/irq.c
+++ b/arch/arm/mach-ns9xxx/irq.c
@@ -77,6 +77,9 @@ static void handle_prio_irq(unsigned int irq, struct irq_desc *desc)
 	if (unlikely(!action || (desc->status & IRQ_DISABLED)))
 		goto out_mask;
 
+	if (desc->status & IRQ_TIMESTAMP)
+		getrawmonotonic(&desc->last_timestamp);
+
 	desc->status |= IRQ_INPROGRESS;
 	raw_spin_unlock(&desc->lock);
 
diff --git a/arch/m68knommu/platform/5272/intc.c b/arch/m68knommu/platform/5272/intc.c
index 3cf681c..69c28f4 100644
--- a/arch/m68knommu/platform/5272/intc.c
+++ b/arch/m68knommu/platform/5272/intc.c
@@ -138,6 +138,10 @@ static int intc_irq_set_type(unsigned int irq, unsigned int type)
 static void intc_external_irq(unsigned int irq, struct irq_desc *desc)
 {
 	kstat_incr_irqs_this_cpu(irq, desc);
+
+	if (desc->status & IRQ_TIMESTAMP)
+		getrawmonotonic(&desc->last_timestamp);
+
 	desc->status |= IRQ_INPROGRESS;
 	desc->chip->ack(irq);
 	handle_IRQ_event(irq, desc->action);
diff --git a/arch/powerpc/platforms/cell/interrupt.c b/arch/powerpc/platforms/cell/interrupt.c
index 10eb1a4..a1fc538 100644
--- a/arch/powerpc/platforms/cell/interrupt.c
+++ b/arch/powerpc/platforms/cell/interrupt.c
@@ -257,6 +257,9 @@ static void handle_iic_irq(unsigned int irq, struct irq_desc *desc)
 	/* Mark the IRQ currently in progress.*/
 	desc->status |= IRQ_INPROGRESS;
 
+	if (desc->status & IRQ_TIMESTAMP)
+		getrawmonotonic(&desc->last_timestamp);
+
 	do {
 		struct irqaction *action = desc->action;
 		irqreturn_t action_ret;
diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 55e0d42..906b3b6 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -67,6 +67,7 @@
 #define IRQF_IRQPOLL		0x00001000
 #define IRQF_ONESHOT		0x00002000
 #define IRQF_NO_SUSPEND		0x00004000
+#define IRQF_TIMESTAMP		0x00008000
 
 #define IRQF_TIMER		(__IRQF_TIMER | IRQF_NO_SUSPEND)
 
diff --git a/include/linux/irq.h b/include/linux/irq.h
index abde252..e6900fd 100644
--- a/include/linux/irq.h
+++ b/include/linux/irq.h
@@ -71,6 +71,7 @@ typedef	void (*irq_flow_handler_t)(unsigned int irq,
 #define IRQ_SUSPENDED		0x04000000	/* IRQ has gone through suspend sequence */
 #define IRQ_ONESHOT		0x08000000	/* IRQ is not unmasked after hardirq */
 #define IRQ_NESTED_THREAD	0x10000000	/* IRQ is nested into another, no own handler thread */
+#define IRQ_TIMESTAMP		0x20000000	/* IRQ wants a timestamp  */
 
 #define IRQF_MODIFY_MASK	\
 	(IRQ_TYPE_SENSE_MASK | IRQ_NOPROBE | IRQ_NOREQUEST | \
diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index c1a95b7..89b0be0 100644
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -80,6 +80,7 @@ struct irq_desc {
 	struct proc_dir_entry	*dir;
 #endif
 	const char		*name;
+	struct timespec		last_timestamp;
 } ____cacheline_internodealigned_in_smp;
 
 #ifndef CONFIG_SPARSE_IRQ
diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c
index baa5c4a..aa06ca0 100644
--- a/kernel/irq/chip.c
+++ b/kernel/irq/chip.c
@@ -475,6 +475,9 @@ handle_simple_irq(unsigned int irq, struct irq_desc *desc)
 	if (unlikely(!action || (desc->status & IRQ_DISABLED)))
 		goto out_unlock;
 
+	if (desc->status & IRQ_TIMESTAMP)
+		getrawmonotonic(&desc->last_timestamp);
+
 	desc->status |= IRQ_INPROGRESS;
 	raw_spin_unlock(&desc->lock);
 
@@ -520,6 +523,9 @@ handle_level_irq(unsigned int irq, struct irq_desc *desc)
 	if (unlikely(!action || (desc->status & IRQ_DISABLED)))
 		goto out_unlock;
 
+	if (desc->status & IRQ_TIMESTAMP)
+		getrawmonotonic(&desc->last_timestamp);
+
 	desc->status |= IRQ_INPROGRESS;
 	raw_spin_unlock(&desc->lock);
 
@@ -572,6 +578,9 @@ handle_fasteoi_irq(unsigned int irq, struct irq_desc *desc)
 		goto out;
 	}
 
+	if (desc->status & IRQ_TIMESTAMP)
+		getrawmonotonic(&desc->last_timestamp);
+
 	desc->status |= IRQ_INPROGRESS;
 	desc->status &= ~IRQ_PENDING;
 	raw_spin_unlock(&desc->lock);
@@ -624,6 +633,9 @@ handle_edge_irq(unsigned int irq, struct irq_desc *desc)
 	}
 	kstat_incr_irqs_this_cpu(irq, desc);
 
+	if (desc->status & IRQ_TIMESTAMP)
+		getrawmonotonic(&desc->last_timestamp);
+
 	/* Start handling the irq */
 	desc->irq_data.chip->irq_ack(&desc->irq_data);
 
@@ -676,6 +688,9 @@ handle_percpu_irq(unsigned int irq, struct irq_desc *desc)
 {
 	irqreturn_t action_ret;
 
+	if (desc->status & IRQ_TIMESTAMP)
+		getrawmonotonic(&desc->last_timestamp);
+
 	kstat_incr_irqs_this_cpu(irq, desc);
 
 	if (desc->irq_data.chip->irq_ack)
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 0caa59f..e9df1f0 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -807,6 +807,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
 				(int)(new->flags & IRQF_TRIGGER_MASK));
 	}
 
+	if (new->flags & IRQF_TIMESTAMP)
+		desc->status |= IRQ_TIMESTAMP;
+
 	new->irq = irq;
 	*old_ptr = new;
 
@@ -926,10 +929,24 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
 	/* If this was the last handler, shut down the IRQ line: */
 	if (!desc->action) {
 		desc->status |= IRQ_DISABLED;
+		desc->status &= ~(IRQ_TIMESTAMP);
 		if (desc->irq_data.chip->irq_shutdown)
 			desc->irq_data.chip->irq_shutdown(&desc->irq_data);
 		else
 			desc->irq_data.chip->irq_disable(&desc->irq_data);
+	} else {
+		/* check if the IRQ_TIMESTAMP flag needs to be reset */
+		if (action->flags & IRQF_TIMESTAMP) {
+			struct irqaction *act = desc->action;
+			unsigned long have_timestamp = 0;
+			while (act) {
+				have_timestamp |= (act->flags & IRQF_TIMESTAMP);
+				act = act->next;
+			}
+
+			if (!have_timestamp)
+				desc->status &= ~(IRQ_TIMESTAMP);
+		}
 	}
 
 #ifdef CONFIG_SMP
diff --git a/kernel/irq/spurious.c b/kernel/irq/spurious.c
index 3089d3b..7cbd485 100644
--- a/kernel/irq/spurious.c
+++ b/kernel/irq/spurious.c
@@ -44,6 +44,10 @@ static int try_one_irq(int irq, struct irq_desc *desc)
 	}
 	/* Honour the normal IRQ locking */
 	desc->status |= IRQ_INPROGRESS;
+
+	if (desc->status & IRQ_TIMESTAMP)
+		getrawmonotonic(&desc->last_timestamp);
+
 	action = desc->action;
 	raw_spin_unlock(&desc->lock);
 
-- 
1.7.2.3

[PATCH 2/5] RFC genirq: add function to retrieve timestamp from irqdesc

[Torben Hohn]

this is probably not useful.
and will be superceded by other functions.
its just here to ilustrate my initial idea.

Signed-off-by: Torben Hohn <torbenh@gmx.de>
---
 kernel/irq/manage.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index e9df1f0..adbf6c9 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1176,3 +1176,12 @@ int request_any_context_irq(unsigned int irq, irq_handler_t handler,
 	return !ret ? IRQC_IS_HARDIRQ : ret;
 }
 EXPORT_SYMBOL_GPL(request_any_context_irq);
+
+void retreive_irq_timestamp(unsigned int irq, struct timespec *ts)
+{
+	struct irq_desc *desc = irq_to_desc(irq);
+
+	*ts = desc->last_timestamp;
+}
+EXPORT_SYMBOL_GPL(retreive_irq_timestamp);
+
-- 
1.7.2.3

[PATCH 3/5] RFC genirq: save irq timestamps in void *dev_id

[Torben Hohn]

To make the timestamps accessible to the irq handler,
we mandate that IRQF_TIMESTAMP irqs have their dev_id
point to a struct timespec, where the timestamp is stored.

Genirq will fill in the timestamp before calling the irq handler.

Signed-off-by: Torben Hohn <torbenh@gmx.de>
---
 kernel/irq/handle.c |    6 ++++++
 kernel/irq/manage.c |    9 +++++++++
 2 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index 3540a71..4edcd03 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -64,6 +64,12 @@ irqreturn_t handle_IRQ_event(unsigned int irq, struct irqaction *action)
 	unsigned int status = 0;
 
 	do {
+		if (action->flags & IRQF_TIMESTAMP) {
+			struct timespec *ts = action->dev_id;
+			struct irq_desc *desc = irq_to_desc(irq);
+			*ts = desc->last_timestamp;
+		}
+
 		trace_irq_handler_entry(irq, action);
 		ret = action->handler(irq, action->dev_id);
 		trace_irq_handler_exit(irq, action, ret);
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index adbf6c9..b60350f 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -972,6 +972,8 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
 	 *   'real' IRQ doesn't run in * parallel with our fake. )
 	 */
 	if (action->flags & IRQF_SHARED) {
+		if (action->flags & IRQF_TIMESTAMP)
+			getrawmonotonic((struct timespec *)dev_id);
 		local_irq_save(flags);
 		action->handler(irq, dev_id);
 		local_irq_restore(flags);
@@ -1068,6 +1070,10 @@ EXPORT_SYMBOL(free_irq);
  *	IRQF_SHARED		Interrupt is shared
  *	IRQF_SAMPLE_RANDOM	The interrupt can be used for entropy
  *	IRQF_TRIGGER_*		Specify active edge(s) or level
+ *	IRQF_TIMESTAMP		Interrupt gets timestamped. (in this
+ *				case dev_id MUST point to a 
+ *				struct timespec which will be filled
+ *				with the timestamp)
  *
  */
 int request_threaded_irq(unsigned int irq, irq_handler_t handler,
@@ -1127,6 +1133,9 @@ int request_threaded_irq(unsigned int irq, irq_handler_t handler,
 		 */
 		unsigned long flags;
 
+		if (irqflags & IRQF_TIMESTAMP)
+			getrawmonotonic((struct timespec *)dev_id);
+
 		disable_irq(irq);
 		local_irq_save(flags);
 
-- 
1.7.2.3

[PATCH 4/5] RFC genirq: allow dynamic switching of timestamping irqs

[Torben Hohn]

Subsystems might want to dynamically switch irq timestamping,
if some subsystem driver requests timestamps.

This patch adds:
void irq_turn_on_timestamping(unsigned int irq, void *dev_id)
void irq_turn_off_timestamping(unsigned int irq, void *dev_id)

Signed-off-by: Torben Hohn <torbenh@gmx.de>
---
 include/linux/interrupt.h |    3 ++
 kernel/irq/manage.c       |   66 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 69 insertions(+), 0 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 906b3b6..cc9ed9c 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -175,6 +175,9 @@ request_any_context_irq(unsigned int irq, irq_handler_t handler,
 static inline void exit_irq_thread(void) { }
 #endif
 
+extern void irq_turn_on_timestamping(unsigned int irq, void *dev_id);
+extern void irq_turn_off_timestamping(unsigned int irq, void *dev_id);
+
 extern void free_irq(unsigned int, void *);
 
 struct device;
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index b60350f..66e9501 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -649,6 +649,72 @@ void exit_irq_thread(void)
 }
 
 /*
+ *	irq_turn_on_timestamping - Turn on timestamping for irq line.
+ *	@irq: Interrupt line
+ *	@dev_id: Device Id for which to turn on timestamping.
+ *	         (must point to a struct timespec)
+ *
+ *	Turning on timestamping nests.
+ */
+void irq_turn_on_timestamping(unsigned int irq, void *dev_id)
+{
+	struct irq_desc *desc = irq_to_desc(irq);
+	unsigned long flags;
+	struct irqaction *act;
+
+	raw_spin_lock_irqsave(&desc->lock, flags);
+	desc->status |= IRQ_TIMESTAMP;
+
+	act = desc->action;
+	while (act) {
+		if (act->dev_id == dev_id) {
+			act->flags |= IRQF_TIMESTAMP;
+			break;
+		}
+		act = act->next;
+	}
+
+	BUG_ON( act==NULL );
+
+	raw_spin_unlock_irqrestore(&desc->lock, flags);
+
+	/* maybe we need to synchronise_irq() here ??? */
+}
+EXPORT_SYMBOL_GPL(irq_turn_on_timestamping);
+
+/*
+ *	irq_turn_off_timestamping - Turn off timestamping for irq line.
+ *	@irq: Interrupt line
+ *	@dev_id: Device Id for which to turn off timestamping.
+ *
+ *	Turning on timestamping nests.
+ */
+void irq_turn_off_timestamping(unsigned int irq, void *dev_id)
+{
+	struct irq_desc *desc = irq_to_desc(irq);
+	unsigned long flags;
+	struct irqaction *act;
+	unsigned long irqf_accu = 0;
+
+	raw_spin_lock_irqsave(&desc->lock, flags);
+
+	act = desc->action;
+	while (act) {
+		if (act->dev_id == dev_id) {
+			act->flags &= ~(IRQF_TIMESTAMP);
+		}
+		irqf_accu |= act->flags;
+		act = act->next;
+	}
+
+	if ((irqf_accu & IRQF_TIMESTAMP) == 0)
+		desc->status &= ~(IRQ_TIMESTAMP);
+
+	raw_spin_unlock_irqrestore(&desc->lock, flags);
+}
+EXPORT_SYMBOL_GPL(irq_turn_off_timestamping);
+
+/*
  * Internal function to register an irqaction - typically used to
  * allocate special interrupts that are part of the architecture.
  */
-- 
1.7.2.3

[PATCH 5/5] RFC genirq: enforce dev_id to be struct timespec * with IRQF_TIMESTAMP

[Torben Hohn]

we can enforce the dev_id to be a struct timespec * in
irq_turn_on_timestamping() and irq_turn_off_timestamping()

Signed-off-by: Torben Hohn <torbenh@gmx.de>
---
 include/linux/interrupt.h |    4 ++--
 kernel/irq/manage.c       |    8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index cc9ed9c..759fead 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -175,8 +175,8 @@ request_any_context_irq(unsigned int irq, irq_handler_t handler,
 static inline void exit_irq_thread(void) { }
 #endif
 
-extern void irq_turn_on_timestamping(unsigned int irq, void *dev_id);
-extern void irq_turn_off_timestamping(unsigned int irq, void *dev_id);
+extern void irq_turn_on_timestamping(unsigned int irq, struct timespec *dev_id);
+extern void irq_turn_off_timestamping(unsigned int irq, struct timespec *dev_id);
 
 extern void free_irq(unsigned int, void *);
 
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index 66e9501..3ac9acc 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -656,7 +656,7 @@ void exit_irq_thread(void)
  *
  *	Turning on timestamping nests.
  */
-void irq_turn_on_timestamping(unsigned int irq, void *dev_id)
+void irq_turn_on_timestamping(unsigned int irq, struct timespec *dev_id)
 {
 	struct irq_desc *desc = irq_to_desc(irq);
 	unsigned long flags;
@@ -667,7 +667,7 @@ void irq_turn_on_timestamping(unsigned int irq, void *dev_id)
 
 	act = desc->action;
 	while (act) {
-		if (act->dev_id == dev_id) {
+		if (act->dev_id == (void *)dev_id) {
 			act->flags |= IRQF_TIMESTAMP;
 			break;
 		}
@@ -689,7 +689,7 @@ EXPORT_SYMBOL_GPL(irq_turn_on_timestamping);
  *
  *	Turning on timestamping nests.
  */
-void irq_turn_off_timestamping(unsigned int irq, void *dev_id)
+void irq_turn_off_timestamping(unsigned int irq, struct timespec *dev_id)
 {
 	struct irq_desc *desc = irq_to_desc(irq);
 	unsigned long flags;
@@ -700,7 +700,7 @@ void irq_turn_off_timestamping(unsigned int irq, void *dev_id)
 
 	act = desc->action;
 	while (act) {
-		if (act->dev_id == dev_id) {
+		if (act->dev_id == (void *)dev_id) {
 			act->flags &= ~(IRQF_TIMESTAMP);
 		}
 		irqf_accu |= act->flags;
-- 
1.7.2.3

Re: [PATCH 1/5] RFC genirq: Add IRQ timestamping

[torbenh]

On Mon, Mar 21, 2011 at 12:59:16PM +0100, Torben Hohn wrote:
> This patch adds the IRQF_TIMESTAMP flag to register_threaded_irq()

bah... i thought i could CC the linuxpps list. but its closed.
sorry for the inconvenience. be careful with group replies.


-- 
torben Hohn

Re: [PATCH 1/5] RFC genirq: Add IRQ timestamping

[john stultz]

On Mon, Mar 21, 2011 at 4:59 AM, Torben Hohn <torbenh@gmx.de> wrote:
> +       if (desc->status & IRQ_TIMESTAMP)
> +               getrawmonotonic(&desc->last_timestamp);
> +

I suspect you probably want some very clear comments around why you
chose timestamps based on CLOCK_MONOTONIC_RAW instead of
CLOCK_MONOTONIC. Likely in the code as well as in the commit log.

thanks
-john

Re: [PATCH 1/5] RFC genirq: Add IRQ timestamping

[torbenh]

On Mon, Mar 21, 2011 at 07:33:26PM -0700, john stultz wrote:
> On Mon, Mar 21, 2011 at 4:59 AM, Torben Hohn <torbenh@gmx.de> wrote:
> > +       if (desc->status & IRQ_TIMESTAMP)
> > +               getrawmonotonic(&desc->last_timestamp);
> > +
> 
> I suspect you probably want some very clear comments around why you
> chose timestamps based on CLOCK_MONOTONIC_RAW instead of
> CLOCK_MONOTONIC. Likely in the code as well as in the commit log.

I really dont know which clock would be the most useful one.
CLOCK_MONOTONIC_RAW seems to be more suitable to drive dlls.
But the network layer probably wants CLOCK_MONOTONIC or
even CLOCK_REALTIME.

I didnt put much thought in this point, because it requires a broader
discussion, I think.

> 
> thanks
> -john

-- 
torben Hohn