Re: [RFC PATCH 7/9] irq_work: Make self-IPIs optable
2012/10/29 Steven Rostedt :
> On Mon, 2012-10-29 at 14:28 +0100, Frederic Weisbecker wrote:
>> On irq work initialization, let the user choose to define it
>> as "lazy" or not. "Lazy" means that we don't want to send
>> an IPI (provided the arch can anyway) when we enqueue this
>> work but we rather prefer to wait for the next timer tick
>> to execute our work if possible.
>>
>> This is going to be a benefit for non-urgent enqueuers
>> (like printk in the future) that may prefer not to raise
>> an IPI storm in case of frequent enqueuing on short periods
>> of time.
>>
>> Signed-off-by: Frederic Weisbecker
>> Cc: Peter Zijlstra
>> Cc: Thomas Gleixner
>> Cc: Ingo Molnar
>> Cc: Andrew Morton
>> Cc: Steven Rostedt
>> Cc: Paul Gortmaker
>> ---
>> include/linux/irq_work.h | 31 ++
>> kernel/irq_work.c| 53
>> -
>> kernel/time/tick-sched.c |3 +-
>> 3 files changed, 70 insertions(+), 17 deletions(-)
>>
>> diff --git a/include/linux/irq_work.h b/include/linux/irq_work.h
>> index b39ea0b..7b60c87 100644
>> --- a/include/linux/irq_work.h
>> +++ b/include/linux/irq_work.h
>> @@ -4,6 +4,20 @@
>> #include
>> #include
>>
>> +/*
>> + * An entry can be in one of four states:
>> + *
>
> Can you add a comment to what the pointer value is. I know you just
> moved it to the header, but it's still confusing.
Which pointer value? You mean the flag? You mean the below need more
details or?
>
>> + * free NULL, 0 -> {claimed} : free to be used
>> + * claimed NULL, 3 -> {pending} : claimed to be enqueued
>> + * pending next, 3 -> {busy} : queued, pending callback
>> + * busy NULL, 2 -> {free, claimed} : callback in progress, can be
>> claimed
>> + */
>> +
>> +#define IRQ_WORK_PENDING 1UL
>> +#define IRQ_WORK_BUSY2UL
>> +#define IRQ_WORK_FLAGS 3UL
>> +#define IRQ_WORK_LAZY4UL /* Doesn't want IPI, wait for tick
>> */
[...]
>> @@ -66,10 +56,28 @@ static void __irq_work_queue(struct irq_work *work)
>> preempt_disable();
>>
>> empty = llist_add(>llnode, &__get_cpu_var(irq_work_list));
>> - /* The list was empty, raise self-interrupt to start processing. */
>> - if (empty)
>> +
>> + /*
>> + * In any case, raise an IPI if requested and possible in case
>> + * the queue is empty or it's filled with lazy works.
>> + */
>> + if (!(work->flags & IRQ_WORK_LAZY) && arch_irq_work_has_ipi()) {
>> arch_irq_work_raise();
>
> Doesn't this mean that now if we queue up a bunch of work (say in
> tracing), that we will send out an IPI for each queue? We only want to
> send out an IPI if the list isn't empty. Perhaps we should make two
> lists. One for lazy work and one for immediate work. Then, when adding a
> non-lazy work item, we can check the empty variable for that. No need to
> check the result for the lazy queue. That will be done during tick.
Indeed, if an IPI is pending while we queue another work, we'll raise
another one. I would prefer to avoid the complication of adding
another queue though. Perhaps a per cpu "ipi_pending" flag would be
enough. I'll try something.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 7/9] irq_work: Make self-IPIs optable
2012/10/29 Steven Rostedt rost...@goodmis.org:
On Mon, 2012-10-29 at 14:28 +0100, Frederic Weisbecker wrote:
On irq work initialization, let the user choose to define it
as lazy or not. Lazy means that we don't want to send
an IPI (provided the arch can anyway) when we enqueue this
work but we rather prefer to wait for the next timer tick
to execute our work if possible.
This is going to be a benefit for non-urgent enqueuers
(like printk in the future) that may prefer not to raise
an IPI storm in case of frequent enqueuing on short periods
of time.
Signed-off-by: Frederic Weisbecker fweis...@gmail.com
Cc: Peter Zijlstra pet...@infradead.org
Cc: Thomas Gleixner t...@linutronix.de
Cc: Ingo Molnar mi...@kernel.org
Cc: Andrew Morton a...@linux-foundation.org
Cc: Steven Rostedt rost...@goodmis.org
Cc: Paul Gortmaker paul.gortma...@windriver.com
---
include/linux/irq_work.h | 31 ++
kernel/irq_work.c| 53
-
kernel/time/tick-sched.c |3 +-
3 files changed, 70 insertions(+), 17 deletions(-)
diff --git a/include/linux/irq_work.h b/include/linux/irq_work.h
index b39ea0b..7b60c87 100644
--- a/include/linux/irq_work.h
+++ b/include/linux/irq_work.h
@@ -4,6 +4,20 @@
#include linux/llist.h
#include asm/irq_work.h
+/*
+ * An entry can be in one of four states:
+ *
Can you add a comment to what the pointer value is. I know you just
moved it to the header, but it's still confusing.
Which pointer value? You mean the flag? You mean the below need more
details or?
+ * free NULL, 0 - {claimed} : free to be used
+ * claimed NULL, 3 - {pending} : claimed to be enqueued
+ * pending next, 3 - {busy} : queued, pending callback
+ * busy NULL, 2 - {free, claimed} : callback in progress, can be
claimed
+ */
+
+#define IRQ_WORK_PENDING 1UL
+#define IRQ_WORK_BUSY2UL
+#define IRQ_WORK_FLAGS 3UL
+#define IRQ_WORK_LAZY4UL /* Doesn't want IPI, wait for tick
*/
[...]
@@ -66,10 +56,28 @@ static void __irq_work_queue(struct irq_work *work)
preempt_disable();
empty = llist_add(work-llnode, __get_cpu_var(irq_work_list));
- /* The list was empty, raise self-interrupt to start processing. */
- if (empty)
+
+ /*
+ * In any case, raise an IPI if requested and possible in case
+ * the queue is empty or it's filled with lazy works.
+ */
+ if (!(work-flags IRQ_WORK_LAZY) arch_irq_work_has_ipi()) {
arch_irq_work_raise();
Doesn't this mean that now if we queue up a bunch of work (say in
tracing), that we will send out an IPI for each queue? We only want to
send out an IPI if the list isn't empty. Perhaps we should make two
lists. One for lazy work and one for immediate work. Then, when adding a
non-lazy work item, we can check the empty variable for that. No need to
check the result for the lazy queue. That will be done during tick.
Indeed, if an IPI is pending while we queue another work, we'll raise
another one. I would prefer to avoid the complication of adding
another queue though. Perhaps a per cpu ipi_pending flag would be
enough. I'll try something.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [RFC PATCH 7/9] irq_work: Make self-IPIs optable
On Mon, 2012-10-29 at 14:28 +0100, Frederic Weisbecker wrote:
> On irq work initialization, let the user choose to define it
> as "lazy" or not. "Lazy" means that we don't want to send
> an IPI (provided the arch can anyway) when we enqueue this
> work but we rather prefer to wait for the next timer tick
> to execute our work if possible.
>
> This is going to be a benefit for non-urgent enqueuers
> (like printk in the future) that may prefer not to raise
> an IPI storm in case of frequent enqueuing on short periods
> of time.
>
> Signed-off-by: Frederic Weisbecker
> Cc: Peter Zijlstra
> Cc: Thomas Gleixner
> Cc: Ingo Molnar
> Cc: Andrew Morton
> Cc: Steven Rostedt
> Cc: Paul Gortmaker
> ---
> include/linux/irq_work.h | 31 ++
> kernel/irq_work.c| 53 -
> kernel/time/tick-sched.c |3 +-
> 3 files changed, 70 insertions(+), 17 deletions(-)
>
> diff --git a/include/linux/irq_work.h b/include/linux/irq_work.h
> index b39ea0b..7b60c87 100644
> --- a/include/linux/irq_work.h
> +++ b/include/linux/irq_work.h
> @@ -4,6 +4,20 @@
> #include
> #include
>
> +/*
> + * An entry can be in one of four states:
> + *
Can you add a comment to what the pointer value is. I know you just
moved it to the header, but it's still confusing.
> + * free NULL, 0 -> {claimed} : free to be used
> + * claimed NULL, 3 -> {pending} : claimed to be enqueued
> + * pending next, 3 -> {busy} : queued, pending callback
> + * busy NULL, 2 -> {free, claimed} : callback in progress, can be
> claimed
> + */
> +
> +#define IRQ_WORK_PENDING 1UL
> +#define IRQ_WORK_BUSY2UL
> +#define IRQ_WORK_FLAGS 3UL
> +#define IRQ_WORK_LAZY4UL /* Doesn't want IPI, wait for tick
> */
> +
> struct irq_work {
> unsigned long flags;
> struct llist_node llnode;
> @@ -17,8 +31,25 @@ void init_irq_work(struct irq_work *work, void
> (*func)(struct irq_work *))
> work->func = func;
> }
>
> +#define DEFINE_IRQ_WORK(w, f)\
> + struct irq_work w = { \
> + .func = f, \
> + }
> +
> +#define DEFINE_IRQ_WORK_LAZY(w, f) \
> + struct irq_work w = { \
> + .flags = IRQ_WORK_LAZY, \
> + .func = f, \
> + }
> +
> bool irq_work_queue(struct irq_work *work);
> void irq_work_run(void);
> void irq_work_sync(struct irq_work *work);
>
> +#ifdef CONFIG_IRQ_WORK
> +bool irq_work_needs_cpu(void);
> +#else
> +static bool irq_work_needs_cpu(void) { return false; }
> +#endif
> +
> #endif /* _LINUX_IRQ_WORK_H */
> diff --git a/kernel/irq_work.c b/kernel/irq_work.c
> index d00011c..ce72b20 100644
> --- a/kernel/irq_work.c
> +++ b/kernel/irq_work.c
> @@ -12,20 +12,10 @@
> #include
> #include
> #include
> +#include
> +#include
> #include
>
> -/*
> - * An entry can be in one of four states:
> - *
> - * free NULL, 0 -> {claimed} : free to be used
> - * claimed NULL, 3 -> {pending} : claimed to be enqueued
> - * pending next, 3 -> {busy} : queued, pending callback
> - * busy NULL, 2 -> {free, claimed} : callback in progress, can be
> claimed
> - */
> -
> -#define IRQ_WORK_PENDING 1UL
> -#define IRQ_WORK_BUSY2UL
> -#define IRQ_WORK_FLAGS 3UL
>
> static DEFINE_PER_CPU(struct llist_head, irq_work_list);
>
> @@ -66,10 +56,28 @@ static void __irq_work_queue(struct irq_work *work)
> preempt_disable();
>
> empty = llist_add(>llnode, &__get_cpu_var(irq_work_list));
> - /* The list was empty, raise self-interrupt to start processing. */
> - if (empty)
> +
> + /*
> + * In any case, raise an IPI if requested and possible in case
> + * the queue is empty or it's filled with lazy works.
> + */
> + if (!(work->flags & IRQ_WORK_LAZY) && arch_irq_work_has_ipi()) {
> arch_irq_work_raise();
Doesn't this mean that now if we queue up a bunch of work (say in
tracing), that we will send out an IPI for each queue? We only want to
send out an IPI if the list isn't empty. Perhaps we should make two
lists. One for lazy work and one for immediate work. Then, when adding a
non-lazy work item, we can check the empty variable for that. No need to
check the result for the lazy queue. That will be done during tick.
Perhaps then do:
empty = false;
if (work->flags & IRQ_WORK_LAZY)
llist_add(>llnode, &__get_cpu_var(irq_work_lazy_list));
else
empty = llist_add(>llnode, &__get_cpu_var(irq_work_list));
if (empty) {
}
You can add your arch_irq_work_has_ipi() stuff as well.
-- Steve
> + goto out;
> + }llist_add(>llnode, &__get_cpu_var(irq_work_list));
>
> + if (empty) {
> + /*
> + * If the arch uses some other obscure way than IPI to raise
> + *
Re: [RFC PATCH 7/9] irq_work: Make self-IPIs optable
On Mon, 2012-10-29 at 14:28 +0100, Frederic Weisbecker wrote:
On irq work initialization, let the user choose to define it
as lazy or not. Lazy means that we don't want to send
an IPI (provided the arch can anyway) when we enqueue this
work but we rather prefer to wait for the next timer tick
to execute our work if possible.
This is going to be a benefit for non-urgent enqueuers
(like printk in the future) that may prefer not to raise
an IPI storm in case of frequent enqueuing on short periods
of time.
Signed-off-by: Frederic Weisbecker fweis...@gmail.com
Cc: Peter Zijlstra pet...@infradead.org
Cc: Thomas Gleixner t...@linutronix.de
Cc: Ingo Molnar mi...@kernel.org
Cc: Andrew Morton a...@linux-foundation.org
Cc: Steven Rostedt rost...@goodmis.org
Cc: Paul Gortmaker paul.gortma...@windriver.com
---
include/linux/irq_work.h | 31 ++
kernel/irq_work.c| 53 -
kernel/time/tick-sched.c |3 +-
3 files changed, 70 insertions(+), 17 deletions(-)
diff --git a/include/linux/irq_work.h b/include/linux/irq_work.h
index b39ea0b..7b60c87 100644
--- a/include/linux/irq_work.h
+++ b/include/linux/irq_work.h
@@ -4,6 +4,20 @@
#include linux/llist.h
#include asm/irq_work.h
+/*
+ * An entry can be in one of four states:
+ *
Can you add a comment to what the pointer value is. I know you just
moved it to the header, but it's still confusing.
+ * free NULL, 0 - {claimed} : free to be used
+ * claimed NULL, 3 - {pending} : claimed to be enqueued
+ * pending next, 3 - {busy} : queued, pending callback
+ * busy NULL, 2 - {free, claimed} : callback in progress, can be
claimed
+ */
+
+#define IRQ_WORK_PENDING 1UL
+#define IRQ_WORK_BUSY2UL
+#define IRQ_WORK_FLAGS 3UL
+#define IRQ_WORK_LAZY4UL /* Doesn't want IPI, wait for tick
*/
+
struct irq_work {
unsigned long flags;
struct llist_node llnode;
@@ -17,8 +31,25 @@ void init_irq_work(struct irq_work *work, void
(*func)(struct irq_work *))
work-func = func;
}
+#define DEFINE_IRQ_WORK(w, f)\
+ struct irq_work w = { \
+ .func = f, \
+ }
+
+#define DEFINE_IRQ_WORK_LAZY(w, f) \
+ struct irq_work w = { \
+ .flags = IRQ_WORK_LAZY, \
+ .func = f, \
+ }
+
bool irq_work_queue(struct irq_work *work);
void irq_work_run(void);
void irq_work_sync(struct irq_work *work);
+#ifdef CONFIG_IRQ_WORK
+bool irq_work_needs_cpu(void);
+#else
+static bool irq_work_needs_cpu(void) { return false; }
+#endif
+
#endif /* _LINUX_IRQ_WORK_H */
diff --git a/kernel/irq_work.c b/kernel/irq_work.c
index d00011c..ce72b20 100644
--- a/kernel/irq_work.c
+++ b/kernel/irq_work.c
@@ -12,20 +12,10 @@
#include linux/percpu.h
#include linux/hardirq.h
#include linux/irqflags.h
+#include linux/tick.h
+#include linux/sched.h
#include asm/processor.h
-/*
- * An entry can be in one of four states:
- *
- * free NULL, 0 - {claimed} : free to be used
- * claimed NULL, 3 - {pending} : claimed to be enqueued
- * pending next, 3 - {busy} : queued, pending callback
- * busy NULL, 2 - {free, claimed} : callback in progress, can be
claimed
- */
-
-#define IRQ_WORK_PENDING 1UL
-#define IRQ_WORK_BUSY2UL
-#define IRQ_WORK_FLAGS 3UL
static DEFINE_PER_CPU(struct llist_head, irq_work_list);
@@ -66,10 +56,28 @@ static void __irq_work_queue(struct irq_work *work)
preempt_disable();
empty = llist_add(work-llnode, __get_cpu_var(irq_work_list));
- /* The list was empty, raise self-interrupt to start processing. */
- if (empty)
+
+ /*
+ * In any case, raise an IPI if requested and possible in case
+ * the queue is empty or it's filled with lazy works.
+ */
+ if (!(work-flags IRQ_WORK_LAZY) arch_irq_work_has_ipi()) {
arch_irq_work_raise();
Doesn't this mean that now if we queue up a bunch of work (say in
tracing), that we will send out an IPI for each queue? We only want to
send out an IPI if the list isn't empty. Perhaps we should make two
lists. One for lazy work and one for immediate work. Then, when adding a
non-lazy work item, we can check the empty variable for that. No need to
check the result for the lazy queue. That will be done during tick.
Perhaps then do:
empty = false;
if (work-flags IRQ_WORK_LAZY)
llist_add(work-llnode, __get_cpu_var(irq_work_lazy_list));
else
empty = llist_add(work-llnode, __get_cpu_var(irq_work_list));
if (empty) {
}
You can add your arch_irq_work_has_ipi() stuff as well.
-- Steve
+ goto out;
+ }llist_add(work-llnode, __get_cpu_var(irq_work_list));
+ if
