[PATCH v3 00/02] Consolidate tasklet + tasklet-hi code

[PATCH v3 00/02] Consolidate tasklet + tasklet-hi code

[Sebastian Andrzej Siewior]

Ingo made a RT patch a few years back called "tasklet: Prevent tasklets
from going into infinite spin in RT" [0]. I ripped the non-RT pieces out
of it and here they are. I kept him as the original Author.

v1…v3: in v1 I had this_cpu_ptr(&tasklet_hi_vec) without disabling
       interrupts and was objected by Steven. There are (indeed) some
       users which use this outside if of the interrupt.
       So in v2 I moved this but then Julia suggested pass just the
       __percpu pointer and use this_cpu_ptr() once interrupts are
       disabled.
       This is all 1/2. 2/2 is unchanged because tasklet_action() /
       tasklet_hi_action() run always in BH and can't migrate to another
       CPU between this_cpu_() access and disabling interrupts.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git/tree/patches/tasklet-rt-prevent-tasklets-from-going-into-infinite-spin-in-rt.patch?h=linux-4.14.y-rt-patches

Sebastian

[PATCH 1/2] kernel/sofirq: consolidate common code in __tasklet_schedule() + _hi_

[Sebastian Andrzej Siewior]

From: Ingo Molnar <mingo@elte.hu>

__tasklet_schedule() and __tasklet_hi_schedule() are almost identical.
Move the common code from both function into __tasklet_schedule_common()
and let both functions invoke it with different arguments.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
[bigeasy: splitted out from RT's "tasklet: Prevent tasklets from going
into infinite spin in RT" and added commit message. Do
this_cpu_ptr(headp) in __tasklet_schedule_common() as suggested by Julia
Cartwright]
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/softirq.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/kernel/softirq.c b/kernel/softirq.c
index 24d243ef8e71..2394b009994f 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -460,29 +460,33 @@ struct tasklet_head {
 static DEFINE_PER_CPU(struct tasklet_head, tasklet_vec);
 static DEFINE_PER_CPU(struct tasklet_head, tasklet_hi_vec);
 
-void __tasklet_schedule(struct tasklet_struct *t)
+static void __tasklet_schedule_common(struct tasklet_struct *t,
+				      struct tasklet_head __percpu *headp,
+				      unsigned int softirq_nr)
 {
+	struct tasklet_head *head;
 	unsigned long flags;
 
 	local_irq_save(flags);
+	head = this_cpu_ptr(headp);
 	t->next = NULL;
-	*__this_cpu_read(tasklet_vec.tail) = t;
-	__this_cpu_write(tasklet_vec.tail, &(t->next));
-	raise_softirq_irqoff(TASKLET_SOFTIRQ);
+	*head->tail = t;
+	head->tail = &(t->next);
+	raise_softirq_irqoff(softirq_nr);
 	local_irq_restore(flags);
 }
+
+void __tasklet_schedule(struct tasklet_struct *t)
+{
+	__tasklet_schedule_common(t, &tasklet_vec,
+				  TASKLET_SOFTIRQ);
+}
 EXPORT_SYMBOL(__tasklet_schedule);
 
 void __tasklet_hi_schedule(struct tasklet_struct *t)
 {
-	unsigned long flags;
-
-	local_irq_save(flags);
-	t->next = NULL;
-	*__this_cpu_read(tasklet_hi_vec.tail) = t;
-	__this_cpu_write(tasklet_hi_vec.tail,  &(t->next));
-	raise_softirq_irqoff(HI_SOFTIRQ);
-	local_irq_restore(flags);
+	__tasklet_schedule_common(t, &tasklet_hi_vec,
+				  HI_SOFTIRQ);
 }
 EXPORT_SYMBOL(__tasklet_hi_schedule);
 
-- 
2.16.2

[tip:irq/core] softirq: Consolidate common code in __tasklet_[hi]_schedule()

[tip-bot for Ingo Molnar]

Commit-ID:  6498ddad301c7a94162915d06d1efe2e5d20f6dc
Gitweb:     https://git.kernel.org/tip/6498ddad301c7a94162915d06d1efe2e5d20f6dc
Author:     Ingo Molnar <mingo@kernel.org>
AuthorDate: Tue, 27 Feb 2018 17:48:07 +0100
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Fri, 9 Mar 2018 11:50:55 +0100

softirq: Consolidate common code in __tasklet_[hi]_schedule()

__tasklet_schedule() and __tasklet_hi_schedule() are almost identical.
Move the common code from both function into __tasklet_schedule_common()
and let both functions invoke it with different arguments.

[ bigeasy: Splitted out from RT's "tasklet: Prevent tasklets from going
  	   into infinite spin in RT" and added commit message. Use
  	   this_cpu_ptr(headp) in __tasklet_schedule_common() as suggested
  	   by Julia Cartwright ]

Signed-off-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Julia Cartwright <juliac@eso.teric.us>
Link: https://lkml.kernel.org/r/20180227164808.10093-2-bigeasy@linutronix.de

---
 kernel/softirq.c | 28 ++++++++++++++++------------
 1 file changed, 16 insertions(+), 12 deletions(-)

diff --git a/kernel/softirq.c b/kernel/softirq.c
index 24d243ef8e71..2394b009994f 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -460,29 +460,33 @@ struct tasklet_head {
 static DEFINE_PER_CPU(struct tasklet_head, tasklet_vec);
 static DEFINE_PER_CPU(struct tasklet_head, tasklet_hi_vec);
 
-void __tasklet_schedule(struct tasklet_struct *t)
+static void __tasklet_schedule_common(struct tasklet_struct *t,
+				      struct tasklet_head __percpu *headp,
+				      unsigned int softirq_nr)
 {
+	struct tasklet_head *head;
 	unsigned long flags;
 
 	local_irq_save(flags);
+	head = this_cpu_ptr(headp);
 	t->next = NULL;
-	*__this_cpu_read(tasklet_vec.tail) = t;
-	__this_cpu_write(tasklet_vec.tail, &(t->next));
-	raise_softirq_irqoff(TASKLET_SOFTIRQ);
+	*head->tail = t;
+	head->tail = &(t->next);
+	raise_softirq_irqoff(softirq_nr);
 	local_irq_restore(flags);
 }
+
+void __tasklet_schedule(struct tasklet_struct *t)
+{
+	__tasklet_schedule_common(t, &tasklet_vec,
+				  TASKLET_SOFTIRQ);
+}
 EXPORT_SYMBOL(__tasklet_schedule);
 
 void __tasklet_hi_schedule(struct tasklet_struct *t)
 {
-	unsigned long flags;
-
-	local_irq_save(flags);
-	t->next = NULL;
-	*__this_cpu_read(tasklet_hi_vec.tail) = t;
-	__this_cpu_write(tasklet_hi_vec.tail,  &(t->next));
-	raise_softirq_irqoff(HI_SOFTIRQ);
-	local_irq_restore(flags);
+	__tasklet_schedule_common(t, &tasklet_hi_vec,
+				  HI_SOFTIRQ);
 }
 EXPORT_SYMBOL(__tasklet_hi_schedule);
 

[PATCH 2/2] kernel/sofirq: consolidate common code in tasklet_action() + tasklet_hi_action()

[Sebastian Andrzej Siewior]

From: Ingo Molnar <mingo@elte.hu>

tasklet_action() + tasklet_hi_action() are almost identical.
Move the common code from both function into __tasklet_schedule_common()
and let both functions invoke it with different arguments.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
[bigeasy: splitted out from RT's "tasklet: Prevent tasklets from going
into infinite spin in RT" and added commit message]
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/softirq.c | 54 +++++++++++++++---------------------------------------
 1 file changed, 15 insertions(+), 39 deletions(-)

diff --git a/kernel/softirq.c b/kernel/softirq.c
index 2394b009994f..177de3640c78 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -490,14 +490,16 @@ void __tasklet_hi_schedule(struct tasklet_struct *t)
 }
 EXPORT_SYMBOL(__tasklet_hi_schedule);
 
-static __latent_entropy void tasklet_action(struct softirq_action *a)
+static void tasklet_action_common(struct softirq_action *a,
+				  struct tasklet_head *tl_head,
+				  unsigned int softirq_nr)
 {
 	struct tasklet_struct *list;
 
 	local_irq_disable();
-	list = __this_cpu_read(tasklet_vec.head);
-	__this_cpu_write(tasklet_vec.head, NULL);
-	__this_cpu_write(tasklet_vec.tail, this_cpu_ptr(&tasklet_vec.head));
+	list = tl_head->head;
+	tl_head->head = NULL;
+	tl_head->tail = &tl_head->head;
 	local_irq_enable();
 
 	while (list) {
@@ -519,47 +521,21 @@ static __latent_entropy void tasklet_action(struct softirq_action *a)
 
 		local_irq_disable();
 		t->next = NULL;
-		*__this_cpu_read(tasklet_vec.tail) = t;
-		__this_cpu_write(tasklet_vec.tail, &(t->next));
-		__raise_softirq_irqoff(TASKLET_SOFTIRQ);
+		*tl_head->tail = t;
+		tl_head->tail = &t->next;
+		__raise_softirq_irqoff(softirq_nr);
 		local_irq_enable();
 	}
 }
 
+static __latent_entropy void tasklet_action(struct softirq_action *a)
+{
+	tasklet_action_common(a, this_cpu_ptr(&tasklet_vec), TASKLET_SOFTIRQ);
+}
+
 static __latent_entropy void tasklet_hi_action(struct softirq_action *a)
 {
-	struct tasklet_struct *list;
-
-	local_irq_disable();
-	list = __this_cpu_read(tasklet_hi_vec.head);
-	__this_cpu_write(tasklet_hi_vec.head, NULL);
-	__this_cpu_write(tasklet_hi_vec.tail, this_cpu_ptr(&tasklet_hi_vec.head));
-	local_irq_enable();
-
-	while (list) {
-		struct tasklet_struct *t = list;
-
-		list = list->next;
-
-		if (tasklet_trylock(t)) {
-			if (!atomic_read(&t->count)) {
-				if (!test_and_clear_bit(TASKLET_STATE_SCHED,
-							&t->state))
-					BUG();
-				t->func(t->data);
-				tasklet_unlock(t);
-				continue;
-			}
-			tasklet_unlock(t);
-		}
-
-		local_irq_disable();
-		t->next = NULL;
-		*__this_cpu_read(tasklet_hi_vec.tail) = t;
-		__this_cpu_write(tasklet_hi_vec.tail, &(t->next));
-		__raise_softirq_irqoff(HI_SOFTIRQ);
-		local_irq_enable();
-	}
+	tasklet_action_common(a, this_cpu_ptr(&tasklet_hi_vec), HI_SOFTIRQ);
 }
 
 void tasklet_init(struct tasklet_struct *t,
-- 
2.16.2

[tip:irq/core] softirq: Consolidate common code in tasklet_[hi]_action()

[tip-bot for Ingo Molnar]

Commit-ID:  82b691bedf05f258f1c86c96ee574b0d7795c0a1
Gitweb:     https://git.kernel.org/tip/82b691bedf05f258f1c86c96ee574b0d7795c0a1
Author:     Ingo Molnar <mingo@kernel.org>
AuthorDate: Tue, 27 Feb 2018 17:48:08 +0100
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Fri, 9 Mar 2018 11:50:55 +0100

softirq: Consolidate common code in tasklet_[hi]_action()

tasklet_action() + tasklet_hi_action() are almost identical.  Move the
common code from both function into __tasklet_action_common() and let
both functions invoke it with different arguments.

[ bigeasy: Splitted out from RT's "tasklet: Prevent tasklets from going
	   into infinite spin in RT" and added commit message]

Signed-off-by: Ingo Molnar <mingo@kernel.org>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Julia Cartwright <juliac@eso.teric.us>
Link: https://lkml.kernel.org/r/20180227164808.10093-3-bigeasy@linutronix.de

---
 kernel/softirq.c | 54 +++++++++++++++---------------------------------------
 1 file changed, 15 insertions(+), 39 deletions(-)

diff --git a/kernel/softirq.c b/kernel/softirq.c
index 2394b009994f..177de3640c78 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -490,14 +490,16 @@ void __tasklet_hi_schedule(struct tasklet_struct *t)
 }
 EXPORT_SYMBOL(__tasklet_hi_schedule);
 
-static __latent_entropy void tasklet_action(struct softirq_action *a)
+static void tasklet_action_common(struct softirq_action *a,
+				  struct tasklet_head *tl_head,
+				  unsigned int softirq_nr)
 {
 	struct tasklet_struct *list;
 
 	local_irq_disable();
-	list = __this_cpu_read(tasklet_vec.head);
-	__this_cpu_write(tasklet_vec.head, NULL);
-	__this_cpu_write(tasklet_vec.tail, this_cpu_ptr(&tasklet_vec.head));
+	list = tl_head->head;
+	tl_head->head = NULL;
+	tl_head->tail = &tl_head->head;
 	local_irq_enable();
 
 	while (list) {
@@ -519,47 +521,21 @@ static __latent_entropy void tasklet_action(struct softirq_action *a)
 
 		local_irq_disable();
 		t->next = NULL;
-		*__this_cpu_read(tasklet_vec.tail) = t;
-		__this_cpu_write(tasklet_vec.tail, &(t->next));
-		__raise_softirq_irqoff(TASKLET_SOFTIRQ);
+		*tl_head->tail = t;
+		tl_head->tail = &t->next;
+		__raise_softirq_irqoff(softirq_nr);
 		local_irq_enable();
 	}
 }
 
-static __latent_entropy void tasklet_hi_action(struct softirq_action *a)
+static __latent_entropy void tasklet_action(struct softirq_action *a)
 {
-	struct tasklet_struct *list;
-
-	local_irq_disable();
-	list = __this_cpu_read(tasklet_hi_vec.head);
-	__this_cpu_write(tasklet_hi_vec.head, NULL);
-	__this_cpu_write(tasklet_hi_vec.tail, this_cpu_ptr(&tasklet_hi_vec.head));
-	local_irq_enable();
-
-	while (list) {
-		struct tasklet_struct *t = list;
-
-		list = list->next;
-
-		if (tasklet_trylock(t)) {
-			if (!atomic_read(&t->count)) {
-				if (!test_and_clear_bit(TASKLET_STATE_SCHED,
-							&t->state))
-					BUG();
-				t->func(t->data);
-				tasklet_unlock(t);
-				continue;
-			}
-			tasklet_unlock(t);
-		}
+	tasklet_action_common(a, this_cpu_ptr(&tasklet_vec), TASKLET_SOFTIRQ);
+}
 
-		local_irq_disable();
-		t->next = NULL;
-		*__this_cpu_read(tasklet_hi_vec.tail) = t;
-		__this_cpu_write(tasklet_hi_vec.tail, &(t->next));
-		__raise_softirq_irqoff(HI_SOFTIRQ);
-		local_irq_enable();
-	}
+static __latent_entropy void tasklet_hi_action(struct softirq_action *a)
+{
+	tasklet_action_common(a, this_cpu_ptr(&tasklet_hi_vec), HI_SOFTIRQ);
 }
 
 void tasklet_init(struct tasklet_struct *t,

Consolidate tasklet + tasklet-hi code

[Sebastian Andrzej Siewior]

Ingo made a RT patch a few years back called "tasklet: Prevent tasklets
from going into infinite spin in RT" [0]. I ripped the non-RT pieces out
of it and here they are. I kept him as the original Author.

[0] https://git.kernel.org/pub/scm/linux/kernel/git/rt/linux-rt-devel.git/tree/patches/tasklet-rt-prevent-tasklets-from-going-into-infinite-spin-in-rt.patch?h=linux-4.14.y-rt-patches

Sebastian

[PATCH 1/2] kernel/sofirq: consolidate common code in __tasklet_schedule() + _hi_

[Sebastian Andrzej Siewior]

From: Ingo Molnar <mingo@elte.hu>

__tasklet_schedule() and __tasklet_hi_schedule() are almost identical.
Move the common code from both function into __tasklet_schedule_common()
and let both functions invoke it with different arguments.

Signed-off-by: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Steven Rostedt <rostedt@goodmis.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
[bigeasy: splitted out from RT's "tasklet: Prevent tasklets from going
into infinite spin in RT" and added commit message]
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/softirq.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/kernel/softirq.c b/kernel/softirq.c
index 24d243ef8e71..145cf6a2e7c9 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -460,29 +460,31 @@ struct tasklet_head {
 static DEFINE_PER_CPU(struct tasklet_head, tasklet_vec);
 static DEFINE_PER_CPU(struct tasklet_head, tasklet_hi_vec);
 
-void __tasklet_schedule(struct tasklet_struct *t)
+static void __tasklet_schedule_common(struct tasklet_struct *t,
+				      struct tasklet_head *head,
+				      unsigned int softirq_nr)
 {
 	unsigned long flags;
 
 	local_irq_save(flags);
 	t->next = NULL;
-	*__this_cpu_read(tasklet_vec.tail) = t;
-	__this_cpu_write(tasklet_vec.tail, &(t->next));
-	raise_softirq_irqoff(TASKLET_SOFTIRQ);
+	*head->tail = t;
+	head->tail = &(t->next);
+	raise_softirq_irqoff(softirq_nr);
 	local_irq_restore(flags);
 }
+
+void __tasklet_schedule(struct tasklet_struct *t)
+{
+	__tasklet_schedule_common(t, this_cpu_ptr(&tasklet_vec),
+				  TASKLET_SOFTIRQ);
+}
 EXPORT_SYMBOL(__tasklet_schedule);
 
 void __tasklet_hi_schedule(struct tasklet_struct *t)
 {
-	unsigned long flags;
-
-	local_irq_save(flags);
-	t->next = NULL;
-	*__this_cpu_read(tasklet_hi_vec.tail) = t;
-	__this_cpu_write(tasklet_hi_vec.tail,  &(t->next));
-	raise_softirq_irqoff(HI_SOFTIRQ);
-	local_irq_restore(flags);
+	__tasklet_schedule_common(t, this_cpu_ptr(&tasklet_hi_vec),
+				  HI_SOFTIRQ);
 }
 EXPORT_SYMBOL(__tasklet_hi_schedule);
 
-- 
2.16.1

Re: [PATCH 1/2] kernel/sofirq: consolidate common code in __tasklet_schedule() + _hi_

[Steven Rostedt]

On Thu, 15 Feb 2018 18:20:41 +0100
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> -void __tasklet_schedule(struct tasklet_struct *t)
> +static void __tasklet_schedule_common(struct tasklet_struct *t,
> +				      struct tasklet_head *head,
> +				      unsigned int softirq_nr)
>  {
>  	unsigned long flags;
>  
>  	local_irq_save(flags);

If you look at the original patch, it did not move local_irq_save()
into the common function.

>  	t->next = NULL;
> -	*__this_cpu_read(tasklet_vec.tail) = t;
> -	__this_cpu_write(tasklet_vec.tail, &(t->next));
> -	raise_softirq_irqoff(TASKLET_SOFTIRQ);
> +	*head->tail = t;
> +	head->tail = &(t->next);
> +	raise_softirq_irqoff(softirq_nr);
>  	local_irq_restore(flags);
>  }
> +
> +void __tasklet_schedule(struct tasklet_struct *t)
> +{
> +	__tasklet_schedule_common(t, this_cpu_ptr(&tasklet_vec),

What can happen is, we reference (tasklet_vec) on one CPU, get
preempted (running in ksoftirqd), scheduled on another CPU, then when
inside the common code, we are executing on a different CPU than the
tasklet is for. The rasise_softirq() is happening on the wrong CPU.

The local_irq_save() can't be moved to the common function. It must be
done by each individual function.

-- Steve


> +				  TASKLET_SOFTIRQ);
> +}
>  EXPORT_SYMBOL(__tasklet_schedule);

Re: [PATCH 1/2] kernel/sofirq: consolidate common code in __tasklet_schedule() + _hi_

[Julia Cartwright]

On Thu, Feb 15, 2018 at 03:07:07PM -0500, Steven Rostedt wrote:
> On Thu, 15 Feb 2018 18:20:41 +0100
> Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> 
> > -void __tasklet_schedule(struct tasklet_struct *t)
> > +static void __tasklet_schedule_common(struct tasklet_struct *t,
> > +				      struct tasklet_head *head,
> > +				      unsigned int softirq_nr)
> >  {
> >  	unsigned long flags;
> >  
> >  	local_irq_save(flags);
> 
> If you look at the original patch, it did not move local_irq_save()
> into the common function.
> 
> >  	t->next = NULL;
> > -	*__this_cpu_read(tasklet_vec.tail) = t;
> > -	__this_cpu_write(tasklet_vec.tail, &(t->next));
> > -	raise_softirq_irqoff(TASKLET_SOFTIRQ);
> > +	*head->tail = t;
> > +	head->tail = &(t->next);
> > +	raise_softirq_irqoff(softirq_nr);
> >  	local_irq_restore(flags);
> >  }
> > +
> > +void __tasklet_schedule(struct tasklet_struct *t)
> > +{
> > +	__tasklet_schedule_common(t, this_cpu_ptr(&tasklet_vec),
> 
> What can happen is, we reference (tasklet_vec) on one CPU, get
> preempted (running in ksoftirqd), scheduled on another CPU, then when
> inside the common code, we are executing on a different CPU than the
> tasklet is for. The rasise_softirq() is happening on the wrong CPU.
> 
> The local_irq_save() can't be moved to the common function. It must be
> done by each individual function.

Well, it can be, but the percpu access needs to go with it; so an
alternative solution would be the below.

I'm also wondering whether the t->next = NULL assignment could be lifted
out from irqs-disabled region.

   Julia

diff --git a/kernel/softirq.c b/kernel/softirq.c
index fa7ed89a9fcf..177de3640c78 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -461,12 +461,14 @@ static DEFINE_PER_CPU(struct tasklet_head, tasklet_vec);
 static DEFINE_PER_CPU(struct tasklet_head, tasklet_hi_vec);
 
 static void __tasklet_schedule_common(struct tasklet_struct *t,
-				      struct tasklet_head *head,
+				      struct tasklet_head __percpu *headp,
 				      unsigned int softirq_nr)
 {
+	struct tasklet_head *head;
 	unsigned long flags;
 
 	local_irq_save(flags);
+	head = this_cpu_ptr(headp);
 	t->next = NULL;
 	*head->tail = t;
 	head->tail = &(t->next);
@@ -476,14 +478,14 @@ static void __tasklet_schedule_common(struct tasklet_struct *t,
 
 void __tasklet_schedule(struct tasklet_struct *t)
 {
-	__tasklet_schedule_common(t, this_cpu_ptr(&tasklet_vec),
+	__tasklet_schedule_common(t, &tasklet_vec,
 				  TASKLET_SOFTIRQ);
 }
 EXPORT_SYMBOL(__tasklet_schedule);
 
 void __tasklet_hi_schedule(struct tasklet_struct *t)
 {
-	__tasklet_schedule_common(t, this_cpu_ptr(&tasklet_hi_vec),
+	__tasklet_schedule_common(t, &tasklet_hi_vec,
 				  HI_SOFTIRQ);
 }
 EXPORT_SYMBOL(__tasklet_hi_schedule);

Re: [PATCH 1/2] kernel/sofirq: consolidate common code in __tasklet_schedule() + _hi_

[Sebastian Andrzej Siewior]

On 2018-02-15 15:07:07 [-0500], Steven Rostedt wrote:
> On Thu, 15 Feb 2018 18:20:41 +0100
> Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> 
> > -void __tasklet_schedule(struct tasklet_struct *t)
> > +static void __tasklet_schedule_common(struct tasklet_struct *t,
> > +				      struct tasklet_head *head,
> > +				      unsigned int softirq_nr)
> >  {
> >  	unsigned long flags;
> >  
> >  	local_irq_save(flags);
> 
> If you look at the original patch, it did not move local_irq_save()
> into the common function.
correct but…

> >  	t->next = NULL;
> > -	*__this_cpu_read(tasklet_vec.tail) = t;
> > -	__this_cpu_write(tasklet_vec.tail, &(t->next));
> > -	raise_softirq_irqoff(TASKLET_SOFTIRQ);
> > +	*head->tail = t;
> > +	head->tail = &(t->next);
> > +	raise_softirq_irqoff(softirq_nr);
> >  	local_irq_restore(flags);
> >  }
> > +
> > +void __tasklet_schedule(struct tasklet_struct *t)
> > +{
> > +	__tasklet_schedule_common(t, this_cpu_ptr(&tasklet_vec),
> 
> What can happen is, we reference (tasklet_vec) on one CPU, get
> preempted (running in ksoftirqd), scheduled on another CPU, then when
> inside the common code, we are executing on a different CPU than the
> tasklet is for. The rasise_softirq() is happening on the wrong CPU.
> 
> The local_irq_save() can't be moved to the common function. It must be
> done by each individual function.

That __tasklet_schedule_common() part is usually invoked from an
interrupt context which attempts to schedule the tasklet so it should
with invoked with interrupts off. However there was one warn_on()
because something early in the boot managed to invoke it without
interrupts disabled (which I missed). Okay, granted, fixed.

As for the second invocation (tasklet_action_common() part) is always
invoked in BH-disabled context (even if called from ksoftirqd) so you
are never preemptible() and can't switch CPUs.
So I am going to correct this patch as you suggested but I don't see the
reason to do the same in the second one.

> -- Steve
Sebastian

Re: [PATCH 1/2] kernel/sofirq: consolidate common code in __tasklet_schedule() + _hi_

[Steven Rostedt]

On Fri, 16 Feb 2018 09:53:03 +0100
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> As for the second invocation (tasklet_action_common() part) is always
> invoked in BH-disabled context (even if called from ksoftirqd) so you
> are never preemptible() and can't switch CPUs.
> So I am going to correct this patch as you suggested but I don't see the
> reason to do the same in the second one.

Should we add something like:

	WARN_ON_ONCE(!in_atomic());

?

-- Steve

Re: [PATCH 1/2] kernel/sofirq: consolidate common code in __tasklet_schedule() + _hi_

[Sebastian Andrzej Siewior]

On 2018-02-16 12:31:09 [-0500], Steven Rostedt wrote:
> On Fri, 16 Feb 2018 09:53:03 +0100
> Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:
> 
> > As for the second invocation (tasklet_action_common() part) is always
> > invoked in BH-disabled context (even if called from ksoftirqd) so you
> > are never preemptible() and can't switch CPUs.
> > So I am going to correct this patch as you suggested but I don't see the
> > reason to do the same in the second one.
> 
> Should we add something like:
> 
> 	WARN_ON_ONCE(!in_atomic());
> 
> ?

Doubt it. this_cpu_ptr() screams already with CONFIG_DEBUG_PREEMPT.

> -- Steve

Sebastian

Re: [PATCH 1/2] kernel/sofirq: consolidate common code in __tasklet_schedule() + _hi_

[Steven Rostedt]

On Fri, 16 Feb 2018 18:55:09 +0100
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:


> > Should we add something like:
> > 
> > 	WARN_ON_ONCE(!in_atomic());
> > 
> > ?  
> 
> Doubt it. this_cpu_ptr() screams already with CONFIG_DEBUG_PREEMPT.

If that's the case then, yeah I agree. I couldn't remember if
this_cpu_ptr() did that or not. I remember having an argument with
Christoph Lameter about whether or not this_cpu_* functions would
complain with preemption off, as some of the use cases were for being
used with preemption enabled. I remember there was some kind of
compromise but didn't remember exactly what that was.

-- Steve