Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1302843 > unrolled thread
| Started by | Daniel Lezcano <daniel.lezcano@linaro.org> |
|---|---|
| First post | 2016-01-06 16:30 +0100 |
| Last post | 2016-01-12 12:50 +0100 |
| Articles | 4 — 2 participants |
Back to article view | Back to linux.kernel
[RFC PATCH 0/2] IRQ based next prediction Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-01-06 16:30 +0100
[RFC PATCH 1/2] irq: Add a framework to measure interrupt timings Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-01-06 16:30 +0100
Re: [RFC PATCH 1/2] irq: Add a framework to measure interrupt timings Thomas Gleixner <tglx@linutronix.de> - 2016-01-08 16:40 +0100
Re: [RFC PATCH 1/2] irq: Add a framework to measure interrupt timings Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-01-12 12:50 +0100
| From | Daniel Lezcano <daniel.lezcano@linaro.org> |
|---|---|
| Date | 2016-01-06 16:30 +0100 |
| Subject | [RFC PATCH 0/2] IRQ based next prediction |
| Message-ID | <qNYc9-2Y2-7@gated-at.bofh.it> |
The current approach to select an idle state is based on the idle period statistics computation. Useless to say this approach satisfied everyone as a solution to find the best trade-off between the performances and the energy saving via the menu governor. However, the kernel is evolving to act pro-actively regarding the energy constraints with the scheduler and the different power management subsystems are not collaborating with the scheduler as the conductor of the decisions, they all act independently. The cpuidle governors are based on idle period statistics, without knowledge of what woke up the cpu. In these sources of wakes up, the IPI are of course accounted (as well as the timers irq) which results in doing statistics on the scheduler behavior too. It is no sense to let the scheduler to take a decision based on a next prediction of its own decisions. In order to integrate the cpuidle framework into the scheduler, we have to radically change the approach by clearly identifying what is causing a wake up and how it behaves. This serie inverts the logic. Instead of tracking the idle durations and do statistics on them, these patches track the interrupt individually and try to predict the next interrupt. By combining the interrupts' next event on a single CPU, we can predict the next event for the CPU, hence predict how long we will be sleeping when entering idle. The IPI and timer interrupts are not taken into account. The first patch provides a callback to be registered in the irq subsystem and to be called when an interrupt is handled with a timestamp. The second patch uses the callback provided by the patch above to compute the delta and store it in a circular buffer. It is per cpu, the callback implements minimal operations as it is in an interrupt context. When we the cpu enters idle, it asks for the expected sleep time. Then the expected minimum sleep length for all interrupts is used and compared to the timer sleep length, again the minimum is taken and gives the expected sleep time. The statistics are very trivial and could be improved later but this first step shows we have a nice overall improvement in SMP. In UP the menu governor is a bit better which may indicate the next prediction computation could be improved but confirms removing the IPI from the equation increase the accuracy. These are the results with a workload emulator (mp3, video, browser, ...) on a Dual Xeon 6 cores. Each test has been run 10 times. The results are based on a debugfs statistics where, when we exit idle, we check if the idle state was the right one which gives a successful prediction even if the exact wakeup time is not correct. The error margin is absorbed by the idle state's target residency, eg. idle1 is 10 us, idle2 is 50us, idle3 is 150us, if we predict to sleep 110us but at the end we sleep 80us, the prediction is still successful as the idle2 is the correct one for 80us and 110us. (mean colum - 5th -, greater is better). SMP (12 cores): -------------------------- Successful predictions (%) -------------------------- scripts/iolatsimu.sh.menu.dat: N min max sum mean stddev 10 30.96 39.3 346.15 34.615 2.37729 scripts/iolatsimu.sh.irq.dat: N min max sum mean stddev 10 22.21 54.39 377.1 37.71 11.3374 -------------------------- Successful predictions (%) -------------------------- scripts/fio-aio.sh.menu.dat: N min max sum mean stddev 10 44.32 48.81 459.5 45.95 1.59114 scripts/fio-aio.sh.irq.dat: N min max sum mean stddev 10 54.18 64.75 592.86 59.286 2.83509 -------------------------- Successful predictions (%) -------------------------- scripts/fio-cpuio.sh.menu.dat: N min max sum mean stddev 10 86.86 91 890.21 89.021 1.34882 scripts/fio-cpuio.sh.irq.dat: N min max sum mean stddev 10 97.97 98.57 981.85 98.185 0.171804 -------------------------- Successful predictions (%) -------------------------- scripts/fio-zipf.sh.menu.dat: N min max sum mean stddev 10 87.58 94.53 917.07 91.707 1.81287 scripts/fio-zipf.sh.irq.dat: N min max sum mean stddev 10 95.51 98.88 980.25 98.025 1.06026 -------------------------- Successful predictions (%) -------------------------- scripts/fio-netio.sh.menu.dat: N min max sum mean stddev 10 65.51 81.82 729.41 72.941 5.34134 scripts/fio-netio.sh.irq.dat: N min max sum mean stddev 10 97.38 97.88 976.19 97.619 0.162238 -------------------------- Successful predictions (%) -------------------------- scripts/fio-falloc.sh.menu.dat: N min max sum mean stddev 10 79.4 91.58 858.71 85.871 3.51946 scripts/fio-falloc.sh.irq.dat: N min max sum mean stddev 10 65.06 87.54 728.72 72.872 6.29703 -------------------------- Successful predictions (%) -------------------------- scripts/fio-null.sh.menu.dat: N min max sum mean stddev 10 89.44 92.83 908.96 90.896 1.12064 scripts/fio-null.sh.irq.dat: N min max sum mean stddev 10 95.68 98.74 978.19 97.819 0.954468 -------------------------- Successful predictions (%) -------------------------- scripts/rt-app-browser.sh.menu.dat: N min max sum mean stddev 10 59 67.66 640.21 64.021 3.11123 scripts/rt-app-browser.sh.irq.dat: N min max sum mean stddev 10 81.45 91.48 865.2 86.52 3.39756 -------------------------- Successful predictions (%) -------------------------- scripts/rt-app-mp3.sh.menu.dat: N min max sum mean stddev 10 65.85 70.67 674.85 67.485 1.34196 scripts/rt-app-mp3.sh.irq.dat: N min max sum mean stddev 10 94.97 97.24 963.96 96.396 0.743897 -------------------------- Successful predictions (%) -------------------------- scripts/rt-app-video.sh.menu.dat: N min max sum mean stddev 10 59.41 63.14 615.57 61.557 1.24713 scripts/rt-app-video.sh.irq.dat: N min max sum mean stddev 10 70.92 76.8 747.7 74.77 1.84455 -------------------------- Successful predictions (%) -------------------------- scripts/video.sh.menu.dat: N min max sum mean stddev 10 27.99 35.54 302.23 30.223 2.27853 scripts/video.sh.irq.dat: N min max sum mean stddev 10 54.42 72.42 635.55 63.555 6.48589 -------------------------- Successful predictions (%) -------------------------- scripts/netperf.sh.menu.dat: N min max sum mean stddev 10 67.49 82.46 748.43 74.843 4.64976 scripts/netperf.sh.irq.dat: N min max sum mean stddev 10 97.51 99.43 990.12 99.012 0.545768 In UP: -------------------------- Successful predictions (%) -------------------------- scripts/iolatsimu.sh.menu.dat: N min max sum mean stddev 10 49.5 55.66 514.02 51.402 2.31278 scripts/iolatsimu.sh.irq.dat: N min max sum mean stddev 10 39.49 63.51 552.35 55.235 6.54344 -------------------------- Successful predictions (%) -------------------------- scripts/fio-aio.sh.menu.dat: N min max sum mean stddev 10 49.44 51.9 502.44 50.244 0.849552 scripts/fio-aio.sh.irq.dat: N min max sum mean stddev 10 35.58 42.06 381.57 38.157 2.05544 -------------------------- Successful predictions (%) -------------------------- scripts/fio-cpuio.sh.menu.dat: N min max sum mean stddev 10 93.07 97.26 954.71 95.471 1.52628 scripts/fio-cpuio.sh.irq.dat: N min max sum mean stddev 10 91.02 97.48 943.92 94.392 2.21478 -------------------------- Successful predictions (%) -------------------------- scripts/fio-zipf.sh.menu.dat: N min max sum mean stddev 10 76.92 88 831.94 83.194 3.2838 scripts/fio-zipf.sh.irq.dat: (2 runs without idle transitions) N min max sum mean stddev 8 50 100 657.5 82.1875 22.418 -------------------------- Successful predictions (%) -------------------------- scripts/fio-netio.sh.menu.dat: N min max sum mean stddev 10 78.95 85.71 818.7 81.87 2.01494 scripts/fio-netio.sh.irq.dat: N min max sum mean stddev 10 62.5 90 733.51 73.351 7.77129 -------------------------- Successful predictions (%) -------------------------- scripts/fio-falloc.sh.menu.dat: N min max sum mean stddev 10 29.42 38.46 357.83 35.783 2.57226 scripts/fio-falloc.sh.irq.dat: N min max sum mean stddev 10 28.85 37.48 333.22 33.322 3.121 -------------------------- Successful predictions (%) -------------------------- scripts/fio-null.sh.menu.dat: N min max sum mean stddev 10 73.53 84.62 806.44 80.644 3.57214 scripts/fio-null.sh.irq.dat: N min max sum mean stddev 10 57.14 85.71 761.68 76.168 8.73407 -------------------------- Successful predictions (%) -------------------------- scripts/rt-app-browser.sh.menu.dat: N min max sum mean stddev 10 96.33 98.8 976.84 97.684 0.834921 scripts/rt-app-browser.sh.irq.dat: N min max sum mean stddev 10 93.02 100 984.58 98.458 2.08916 -------------------------- Successful predictions (%) -------------------------- scripts/rt-app-mp3.sh.menu.dat: N min max sum mean stddev 10 99.06 99.59 993.8 99.38 0.178948 scripts/rt-app-mp3.sh.irq.dat: N min max sum mean stddev 10 95.81 99.54 978.95 97.895 1.36267 -------------------------- Successful predictions (%) -------------------------- scripts/rt-app-video.sh.menu.dat: N min max sum mean stddev 10 96.96 99.08 983.25 98.325 0.632899 scripts/rt-app-video.sh.irq.dat: N min max sum mean stddev 10 74.77 97.5 829.45 82.945 7.10247 -------------------------- Successful predictions (%) -------------------------- scripts/video.sh.menu.dat: N min max sum mean stddev 10 48.52 59.73 541.3 54.13 3.11611 scripts/video.sh.irq.dat: N min max sum mean stddev 10 43.71 58.5 533.62 53.362 4.84646 Daniel Lezcano (2): irq: Add a framework to measure interrupt timings sched: idle: IRQ based next prediction for idle period drivers/cpuidle/Kconfig | 5 + include/linux/interrupt.h | 45 ++++ include/linux/irqdesc.h | 3 + kernel/irq/Kconfig | 3 + kernel/irq/handle.c | 12 + kernel/irq/manage.c | 65 +++++- kernel/sched/Makefile | 1 + kernel/sched/idle-sched.c | 553 ++++++++++++++++++++++++++++++++++++++++++++++ 8 files changed, 686 insertions(+), 1 deletion(-) create mode 100644 kernel/sched/idle-sched.c -- 1.9.1 -- 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/
[toc] | [next] | [standalone]
| From | Daniel Lezcano <daniel.lezcano@linaro.org> |
|---|---|
| Date | 2016-01-06 16:30 +0100 |
| Subject | [RFC PATCH 1/2] irq: Add a framework to measure interrupt timings |
| Message-ID | <qNYca-2Y2-15@gated-at.bofh.it> |
| In reply to | #1302843 |
The interrupt framework gives a lot of information and statistics about
each interrupt.
Unfortunately there is no way to measure when interrupts occur and provide
a mathematical model for their behavior which could help in predicting
their next occurence.
This framework allows for registering a callback function that is invoked
when an interrupt occurs.
Each time, the callback will be called with the timestamp corresponding to
when happened the interrupt.
This framework allows a subsystem to register a handler in order to receive
the timing information for the registered interrupt. That gives other
subsystems the ability to compute predictions for the next interrupt occurence.
The main objective is to track and detect the periodic interrupts in order
to predict the next event on a cpu and anticipate the sleeping time when
entering idle. This fine grain approach allows to simplify and rationalize
a wake up event prediction without IPIs interference, thus letting the
scheduler to be smarter with the wakeup IPIs regarding the idle period.
The irq timings tracking showed, in the proof-of-concept, an improvement with
the predictions, the approach is correct but my knowledge to the irq
subsystem is limited. I am not sure this patch measuring irq time interval
is correct or acceptable, so it is at the RFC state (minus some polishing).
Signed-off-by: Daniel Lezcano <daniel.lezcano@linaro.org>
---
include/linux/interrupt.h | 45 ++++++++++++++++++++++++++++++++
include/linux/irqdesc.h | 3 +++
kernel/irq/Kconfig | 4 +++
kernel/irq/handle.c | 12 +++++++++
kernel/irq/manage.c | 65 ++++++++++++++++++++++++++++++++++++++++++++++-
5 files changed, 128 insertions(+), 1 deletion(-)
diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index be7e75c..f48e8ff 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -123,6 +123,51 @@ struct irqaction {
extern irqreturn_t no_action(int cpl, void *dev_id);
+#ifdef CONFIG_IRQ_TIMINGS
+/**
+ * timing handler to be called when an interrupt happens
+ */
+typedef void (*irqt_handler_t)(unsigned int, ktime_t, void *, void *);
+
+/**
+ * struct irqtimings - per interrupt irq timings descriptor
+ * @handler: interrupt handler timings function
+ * @data: pointer to the private data to be passed to the handler
+ * @timestamp: latest interruption occurence
+ */
+struct irqtimings {
+ irqt_handler_t handler;
+ void *data;
+} ____cacheline_internodealigned_in_smp;
+
+/**
+ * struct irqt_ops - structure to be used by the subsystem to call the
+ * register and unregister ops when an irq is setup or freed.
+ * @setup: registering callback
+ * @free: unregistering callback
+ *
+ * The callbacks assumes the lock is held on the irq desc
+ */
+struct irqtimings_ops {
+ int (*setup)(unsigned int, struct irqaction *);
+ void (*free)(unsigned int, void *);
+};
+
+extern int register_irq_timings(struct irqtimings_ops *ops);
+extern int setup_irq_timings(unsigned int irq, struct irqaction *act);
+extern void free_irq_timings(unsigned int irq, void *dev_id);
+#else
+static inline int setup_irq_timings(unsigned int irq, struct irqaction *act)
+{
+ return 0;
+}
+
+static inline void free_irq_timings(unsigned int irq, void *dev_id)
+{
+ ;
+}
+#endif
+
extern int __must_check
request_threaded_irq(unsigned int irq, irq_handler_t handler,
irq_handler_t thread_fn,
diff --git a/include/linux/irqdesc.h b/include/linux/irqdesc.h
index a587a33..e0d4263 100644
--- a/include/linux/irqdesc.h
+++ b/include/linux/irqdesc.h
@@ -51,6 +51,9 @@ struct irq_desc {
#ifdef CONFIG_IRQ_PREFLOW_FASTEOI
irq_preflow_handler_t preflow_handler;
#endif
+#ifdef CONFIG_IRQ_TIMINGS
+ struct irqtimings *timings;
+#endif
struct irqaction *action; /* IRQ action list */
unsigned int status_use_accessors;
unsigned int core_internal_state__do_not_mess_with_it;
diff --git a/kernel/irq/Kconfig b/kernel/irq/Kconfig
index 9a76e3b..1275fd1 100644
--- a/kernel/irq/Kconfig
+++ b/kernel/irq/Kconfig
@@ -73,6 +73,10 @@ config GENERIC_MSI_IRQ_DOMAIN
config HANDLE_DOMAIN_IRQ
bool
+config IRQ_TIMINGS
+ bool
+ default y
+
config IRQ_DOMAIN_DEBUG
bool "Expose hardware/virtual IRQ mapping via debugfs"
depends on IRQ_DOMAIN && DEBUG_FS
diff --git a/kernel/irq/handle.c b/kernel/irq/handle.c
index e25a83b..ca8b0c5 100644
--- a/kernel/irq/handle.c
+++ b/kernel/irq/handle.c
@@ -132,6 +132,17 @@ void __irq_wake_thread(struct irq_desc *desc, struct irqaction *action)
wake_up_process(action->thread);
}
+#ifdef CONFIG_IRQ_TIMINGS
+void handle_irqt_event(struct irqtimings *irqt, struct irqaction *action)
+{
+ if (irqt)
+ irqt->handler(action->irq, ktime_get(),
+ action->dev_id, irqt->data);
+}
+#else
+#define handle_irqt_event(a, b)
+#endif
+
irqreturn_t
handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action)
{
@@ -165,6 +176,7 @@ handle_irq_event_percpu(struct irq_desc *desc, struct irqaction *action)
/* Fall through to add to randomness */
case IRQ_HANDLED:
flags |= action->flags;
+ handle_irqt_event(desc->timings, action);
break;
default:
diff --git a/kernel/irq/manage.c b/kernel/irq/manage.c
index f9a59f6..21cc7bf 100644
--- a/kernel/irq/manage.c
+++ b/kernel/irq/manage.c
@@ -1017,6 +1017,60 @@ static void irq_release_resources(struct irq_desc *desc)
c->irq_release_resources(d);
}
+#ifdef CONFIG_IRQ_TIMINGS
+/*
+ * Global variable, only used by accessor functions, currently only
+ * one user is allowed and it is up to the caller to make sure to
+ * setup the irq timings which are already setup.
+ */
+static struct irqtimings_ops *irqtimings_ops;
+
+/**
+ * register_irq_timings - register the ops when an irq is setup or freed
+ *
+ * @ops: the register/unregister ops to be called when at setup or
+ * free time
+ *
+ * Returns -EBUSY if the slot is already in use, zero on success.
+ */
+int register_irq_timings(struct irqtimings_ops *ops)
+{
+ if (irqtimings_ops)
+ return -EBUSY;
+
+ irqtimings_ops = ops;
+
+ return 0;
+}
+
+/**
+ * setup_irq_timings - call the timing register callback
+ *
+ * @desc: an irq desc structure
+ *
+ * Returns -EINVAL in case of error, zero on success.
+ */
+int setup_irq_timings(unsigned int irq, struct irqaction *act)
+{
+ if (irqtimings_ops && irqtimings_ops->setup)
+ return irqtimings_ops->setup(irq, act);
+ return 0;
+}
+
+/**
+ * free_irq_timings - call the timing unregister callback
+ *
+ * @irq: the interrupt number
+ * @dev_id: the device id
+ *
+ */
+void free_irq_timings(unsigned int irq, void *dev_id)
+{
+ if (irqtimings_ops && irqtimings_ops->free)
+ irqtimings_ops->free(irq, dev_id);
+}
+#endif /* CONFIG_IRQ_TIMINGS */
+
/*
* Internal function to register an irqaction - typically used to
* allocate special interrupts that are part of the architecture.
@@ -1037,6 +1091,9 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
if (!try_module_get(desc->owner))
return -ENODEV;
+ ret = setup_irq_timings(irq, new);
+ if (ret)
+ goto out_mput;
/*
* Check whether the interrupt nests into another interrupt
* thread.
@@ -1045,7 +1102,7 @@ __setup_irq(unsigned int irq, struct irq_desc *desc, struct irqaction *new)
if (nested) {
if (!new->thread_fn) {
ret = -EINVAL;
- goto out_mput;
+ goto out_free_timings;
}
/*
* Replace the primary handler which was provided from
@@ -1323,6 +1380,10 @@ out_thread:
kthread_stop(t);
put_task_struct(t);
}
+
+out_free_timings:
+ free_irq_timings(irq, new->dev_id);
+
out_mput:
module_put(desc->owner);
return ret;
@@ -1408,6 +1469,8 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
unregister_handler_proc(irq, action);
+ free_irq_timings(irq, dev_id);
+
/* Make sure it's not being used on another CPU: */
synchronize_irq(irq);
--
1.9.1
--
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/
[toc] | [prev] | [next] | [standalone]
| From | Thomas Gleixner <tglx@linutronix.de> |
|---|---|
| Date | 2016-01-08 16:40 +0100 |
| Subject | Re: [RFC PATCH 1/2] irq: Add a framework to measure interrupt timings |
| Message-ID | <qOHiW-cO-27@gated-at.bofh.it> |
| In reply to | #1302846 |
On Wed, 6 Jan 2016, Daniel Lezcano wrote:
> +#ifdef CONFIG_IRQ_TIMINGS
> +/**
> + * timing handler to be called when an interrupt happens
> + */
> +typedef void (*irqt_handler_t)(unsigned int, ktime_t, void *, void *);
> +
> +/**
> + * struct irqtimings - per interrupt irq timings descriptor
> + * @handler: interrupt handler timings function
> + * @data: pointer to the private data to be passed to the handler
> + * @timestamp: latest interruption occurence
There is no timestamp member.
> + */
> +struct irqtimings {
> + irqt_handler_t handler;
> + void *data;
What's that data thingy for. The proposed user does not use it at all and I
have no idea why any user would want it. All it does is provide another level
of indirection in the hotpath.
> +} ____cacheline_internodealigned_in_smp;
> +/**
> + * struct irqt_ops - structure to be used by the subsystem to call the
> + * register and unregister ops when an irq is setup or freed.
> + * @setup: registering callback
> + * @free: unregistering callback
> + *
> + * The callbacks assumes the lock is held on the irq desc
Crap. It's called outside of the locked region and the proposed user locks the
descriptor itself, but that's a different story.
> +static inline void free_irq_timings(unsigned int irq, void *dev_id)
> +{
> + ;
What's the purpose of this semicolon?
> +#ifdef CONFIG_IRQ_TIMINGS
> +void handle_irqt_event(struct irqtimings *irqt, struct irqaction *action)
static ?
> +{
> + if (irqt)
This want's to use a static key.
> + irqt->handler(action->irq, ktime_get(),
> + action->dev_id, irqt->data);
> +}
> +#else
> +#define handle_irqt_event(a, b)
static inline stub if at all.
> +#ifdef CONFIG_IRQ_TIMINGS
> +/*
> + * Global variable, only used by accessor functions, currently only
> + * one user is allowed ...
That variable is static not global. And what the heck means:
> ... and it is up to the caller to make sure to
> + * setup the irq timings which are already setup.
-ENOPARSE.
> + */
> +static struct irqtimings_ops *irqtimings_ops;
> +
> +/**
> + * register_irq_timings - register the ops when an irq is setup or freed
> + *
> + * @ops: the register/unregister ops to be called when at setup or
> + * free time
> + *
> + * Returns -EBUSY if the slot is already in use, zero on success.
> + */
> +int register_irq_timings(struct irqtimings_ops *ops)
> +{
> + if (irqtimings_ops)
> + return -EBUSY;
> +
> + irqtimings_ops = ops;
> +
> + return 0;
> +}
> +
> +/**
> + * setup_irq_timings - call the timing register callback
> + *
> + * @desc: an irq desc structure
The argument list tells a different story.
> + *
> + * Returns -EINVAL in case of error, zero on success.
> + */
> +int setup_irq_timings(unsigned int irq, struct irqaction *act)
static is not in your book, right? These functions are only used in this file,
so no point for having them global visible and the stubs should be local as
well.
> +{
> + if (irqtimings_ops && irqtimings_ops->setup)
> + return irqtimings_ops->setup(irq, act);
> + return 0;
> +}
...
> @@ -1408,6 +1469,8 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
>
> unregister_handler_proc(irq, action);
>
> + free_irq_timings(irq, dev_id);
This needs to go to the point where the interrupt is already synchronized and
the action about to be destroyed.
Thanks,
tglx
[toc] | [prev] | [next] | [standalone]
| From | Daniel Lezcano <daniel.lezcano@linaro.org> |
|---|---|
| Date | 2016-01-12 12:50 +0100 |
| Subject | Re: [RFC PATCH 1/2] irq: Add a framework to measure interrupt timings |
| Message-ID | <qQ5Cy-jP-23@gated-at.bofh.it> |
| In reply to | #1304662 |
Hi Thomas,
thanks for taking some time to review the patches.
On 01/08/2016 04:31 PM, Thomas Gleixner wrote:
> On Wed, 6 Jan 2016, Daniel Lezcano wrote:
>> +#ifdef CONFIG_IRQ_TIMINGS
>> +/**
>> + * timing handler to be called when an interrupt happens
>> + */
>> +typedef void (*irqt_handler_t)(unsigned int, ktime_t, void *, void *);
>> +
>> +/**
>> + * struct irqtimings - per interrupt irq timings descriptor
>> + * @handler: interrupt handler timings function
>> + * @data: pointer to the private data to be passed to the handler
>> + * @timestamp: latest interruption occurence
>
> There is no timestamp member.
>
>> + */
>> +struct irqtimings {
>> + irqt_handler_t handler;
>> + void *data;
>
> What's that data thingy for. The proposed user does not use it at all and I
> have no idea why any user would want it. All it does is provide another level
> of indirection in the hotpath.
Yes, I agree. I added this private_data field for future use in case it
would be needed but it does not make sense now.
>> +} ____cacheline_internodealigned_in_smp;
>
>> +/**
>> + * struct irqt_ops - structure to be used by the subsystem to call the
>> + * register and unregister ops when an irq is setup or freed.
>> + * @setup: registering callback
>> + * @free: unregistering callback
>> + *
>> + * The callbacks assumes the lock is held on the irq desc
>
> Crap. It's called outside of the locked region and the proposed user locks the
> descriptor itself, but that's a different story.
>
>> +static inline void free_irq_timings(unsigned int irq, void *dev_id)
>> +{
>> + ;
>
> What's the purpose of this semicolon?
Bah, old habit. I will remove it.
>> +#ifdef CONFIG_IRQ_TIMINGS
>> +void handle_irqt_event(struct irqtimings *irqt, struct irqaction *action)
>
> static ?
>
>> +{
>> + if (irqt)
>
> This want's to use a static key.
Ok, I will look at that. I already used static keys to disable a portion
of code from sysfs but never this way.
>> + irqt->handler(action->irq, ktime_get(),
>> + action->dev_id, irqt->data);
>> +}
>> +#else
>> +#define handle_irqt_event(a, b)
>
> static inline stub if at all.
ok.
>> +#ifdef CONFIG_IRQ_TIMINGS
>> +/*
>> + * Global variable, only used by accessor functions, currently only
>> + * one user is allowed ...
>
> That variable is static not global. And what the heck means:
>
>> ... and it is up to the caller to make sure to
>> + * setup the irq timings which are already setup.
>
> -ENOPARSE.
hmm , yes ... it is not clear :)
I should have say:
"... and it is up to the caller to register the irq timing callback for
the interrupts which are already setup."
>> + */
>> +static struct irqtimings_ops *irqtimings_ops;
>> +
>> +/**
>> + * register_irq_timings - register the ops when an irq is setup or freed
>> + *
>> + * @ops: the register/unregister ops to be called when at setup or
>> + * free time
>> + *
>> + * Returns -EBUSY if the slot is already in use, zero on success.
>> + */
>> +int register_irq_timings(struct irqtimings_ops *ops)
>> +{
>> + if (irqtimings_ops)
>> + return -EBUSY;
>> +
>> + irqtimings_ops = ops;
>> +
>> + return 0;
>> +}
>> +
>> +/**
>> + * setup_irq_timings - call the timing register callback
>> + *
>> + * @desc: an irq desc structure
>
> The argument list tells a different story.
>
>> + *
>> + * Returns -EINVAL in case of error, zero on success.
>> + */
>> +int setup_irq_timings(unsigned int irq, struct irqaction *act)
>
> static is not in your book, right? These functions are only used in this file,
> so no point for having them global visible and the stubs should be local as
> well.
Ok.
>> +{
>> + if (irqtimings_ops && irqtimings_ops->setup)
>> + return irqtimings_ops->setup(irq, act);
>> + return 0;
>> +}
>
> ...
>
>> @@ -1408,6 +1469,8 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
>>
>> unregister_handler_proc(irq, action);
>>
>> + free_irq_timings(irq, dev_id);
>
> This needs to go to the point where the interrupt is already synchronized and
> the action about to be destroyed.
Ok, noted.
Thanks !
-- Daniel
--
<http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web