Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1453583 > unrolled thread

[RFC PATCH] softirq: fix tasklet_kill() usage and users

Started bySantosh Shilimkar <ssantosh@kernel.org>
First post2016-08-02 06:40 +0200
Last post2016-08-07 19:10 +0200
Articles 4 — 4 participants

Back to article view | Back to linux.kernel

Contents

  [RFC PATCH] softirq: fix tasklet_kill() usage and users Santosh Shilimkar <ssantosh@kernel.org> - 2016-08-02 06:40 +0200
    Re: [RFC PATCH] softirq: fix tasklet_kill() usage and users Santosh Shilimkar <santosh.shilimkar@oracle.com> - 2016-08-06 23:10 +0200
      Re: [RFC PATCH] softirq: fix tasklet_kill() usage and users Greg Kroah-Hartman <gregkh@linuxfoundation.org> - 2016-08-07 09:00 +0200
        Re: [RFC PATCH] softirq: fix tasklet_kill() usage and users "santosh.shilimkar@oracle.com" <santosh.shilimkar@oracle.com> - 2016-08-07 19:10 +0200

#1453583 — [RFC PATCH] softirq: fix tasklet_kill() usage and users

FromSantosh Shilimkar <ssantosh@kernel.org>
Date2016-08-02 06:40 +0200
Subject[RFC PATCH] softirq: fix tasklet_kill() usage and users
Message-ID<s1zEJ-7Ue-9@gated-at.bofh.it>
Semantically the expectation from the tasklet init/kill API
should be as below.

tasklet_init() == Init and Enable scheduling
tasklet_kill() == Disable scheduling and Destroy

tasklet_init() API exibit above behavior but not the
tasklet_kill(). The tasklet handler can still get scheduled
and run even after the tasklet_kill().

There are 2, 3 places where drivers are working around
this issue by calling tasklet_disable() which will add an
usecount and there by avoiding the handlers being called.
One of the example 'commit 1e1257860fd1
("tty/serial: at91: correct the usage of tasklet")'

tasklet_enable/tasklet_disable is a pair API and expected
to be used together. Usage of tasklet_disable() *just* to
workround tasklet scheduling after kill is probably not the
correct and inteded use of the API as done the API.
We also happen to see similar issue where in shutdown path
the tasklet_handler was getting called even after the
tasklet_kill().

We can fix this be making sure tasklet_kill() does right
thing and there by ensuring tasklet handler won't run after
tasklet_kil() with very simple change. Patch fixes the tasklet
code and also few drivers hacks to workaround the issue.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Tadeusz Struk <tadeusz.struk@intel.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Paul Bolle <pebolle@tiscali.nl>
Cc: Nicolas Ferre <nicolas.ferre@atmel.com>

Signed-off-by: Santosh Shilimkar <ssantosh@kernel.org>
---
 drivers/crypto/qat/qat_common/adf_isr.c    | 1 -
 drivers/crypto/qat/qat_common/adf_sriov.c  | 1 -
 drivers/crypto/qat/qat_common/adf_vf_isr.c | 2 --
 drivers/isdn/gigaset/interface.c           | 1 -
 drivers/tty/serial/atmel_serial.c          | 1 -
 kernel/softirq.c                           | 7 ++++---
 6 files changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/crypto/qat/qat_common/adf_isr.c b/drivers/crypto/qat/qat_common/adf_isr.c
index 06d4901..fd5e900 100644
--- a/drivers/crypto/qat/qat_common/adf_isr.c
+++ b/drivers/crypto/qat/qat_common/adf_isr.c
@@ -296,7 +296,6 @@ static void adf_cleanup_bh(struct adf_accel_dev *accel_dev)
 	int i;
 
 	for (i = 0; i < hw_data->num_banks; i++) {
-		tasklet_disable(&priv_data->banks[i].resp_handler);
 		tasklet_kill(&priv_data->banks[i].resp_handler);
 	}
 }
diff --git a/drivers/crypto/qat/qat_common/adf_sriov.c b/drivers/crypto/qat/qat_common/adf_sriov.c
index 4a526e2..9e65888 100644
--- a/drivers/crypto/qat/qat_common/adf_sriov.c
+++ b/drivers/crypto/qat/qat_common/adf_sriov.c
@@ -204,7 +204,6 @@ void adf_disable_sriov(struct adf_accel_dev *accel_dev)
 	}
 
 	for (i = 0, vf = accel_dev->pf.vf_info; i < totalvfs; i++, vf++) {
-		tasklet_disable(&vf->vf2pf_bh_tasklet);
 		tasklet_kill(&vf->vf2pf_bh_tasklet);
 		mutex_destroy(&vf->pf2vf_lock);
 	}
diff --git a/drivers/crypto/qat/qat_common/adf_vf_isr.c b/drivers/crypto/qat/qat_common/adf_vf_isr.c
index aa689ca..81e63bf 100644
--- a/drivers/crypto/qat/qat_common/adf_vf_isr.c
+++ b/drivers/crypto/qat/qat_common/adf_vf_isr.c
@@ -191,7 +191,6 @@ static int adf_setup_pf2vf_bh(struct adf_accel_dev *accel_dev)
 
 static void adf_cleanup_pf2vf_bh(struct adf_accel_dev *accel_dev)
 {
-	tasklet_disable(&accel_dev->vf.pf2vf_bh_tasklet);
 	tasklet_kill(&accel_dev->vf.pf2vf_bh_tasklet);
 	mutex_destroy(&accel_dev->vf.vf2pf_lock);
 }
@@ -268,7 +267,6 @@ static void adf_cleanup_bh(struct adf_accel_dev *accel_dev)
 {
 	struct adf_etr_data *priv_data = accel_dev->transport;
 
-	tasklet_disable(&priv_data->banks[0].resp_handler);
 	tasklet_kill(&priv_data->banks[0].resp_handler);
 }
 
diff --git a/drivers/isdn/gigaset/interface.c b/drivers/isdn/gigaset/interface.c
index 600c79b..2ce63b6 100644
--- a/drivers/isdn/gigaset/interface.c
+++ b/drivers/isdn/gigaset/interface.c
@@ -524,7 +524,6 @@ void gigaset_if_free(struct cardstate *cs)
 	if (!drv->have_tty)
 		return;
 
-	tasklet_disable(&cs->if_wake_tasklet);
 	tasklet_kill(&cs->if_wake_tasklet);
 	cs->tty_dev = NULL;
 	tty_unregister_device(drv->tty, cs->minor_index);
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 954941d..27e638e 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -1915,7 +1915,6 @@ static void atmel_shutdown(struct uart_port *port)
 	 * Clear out any scheduled tasklets before
 	 * we destroy the buffers
 	 */
-	tasklet_disable(&atmel_port->tasklet);
 	tasklet_kill(&atmel_port->tasklet);
 
 	/*
diff --git a/kernel/softirq.c b/kernel/softirq.c
index 17caf4b..21397eb 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -498,7 +498,7 @@ static void tasklet_action(struct softirq_action *a)
 		list = list->next;
 
 		if (tasklet_trylock(t)) {
-			if (!atomic_read(&t->count)) {
+			if (atomic_read(&t->count) == 1) {
 				if (!test_and_clear_bit(TASKLET_STATE_SCHED,
 							&t->state))
 					BUG();
@@ -534,7 +534,7 @@ static void tasklet_hi_action(struct softirq_action *a)
 		list = list->next;
 
 		if (tasklet_trylock(t)) {
-			if (!atomic_read(&t->count)) {
+			if (atomic_read(&t->count) == 1) {
 				if (!test_and_clear_bit(TASKLET_STATE_SCHED,
 							&t->state))
 					BUG();
@@ -559,7 +559,7 @@ void tasklet_init(struct tasklet_struct *t,
 {
 	t->next = NULL;
 	t->state = 0;
-	atomic_set(&t->count, 0);
+	atomic_set(&t->count, 1);
 	t->func = func;
 	t->data = data;
 }
@@ -576,6 +576,7 @@ void tasklet_kill(struct tasklet_struct *t)
 		} while (test_bit(TASKLET_STATE_SCHED, &t->state));
 	}
 	tasklet_unlock_wait(t);
+	atomic_dec(&t->count);
 	clear_bit(TASKLET_STATE_SCHED, &t->state);
 }
 EXPORT_SYMBOL(tasklet_kill);
-- 
1.9.1

[toc] | [next] | [standalone]

#1457364

FromSantosh Shilimkar <santosh.shilimkar@oracle.com>
Date2016-08-06 23:10 +0200
Message-ID<s3h0Z-2zz-21@gated-at.bofh.it>
In reply to#1453583
ping !!

On 8/1/2016 9:13 PM, Santosh Shilimkar wrote:
> Semantically the expectation from the tasklet init/kill API
> should be as below.
>
> tasklet_init() == Init and Enable scheduling
> tasklet_kill() == Disable scheduling and Destroy
>
> tasklet_init() API exibit above behavior but not the
> tasklet_kill(). The tasklet handler can still get scheduled
> and run even after the tasklet_kill().
>
> There are 2, 3 places where drivers are working around
> this issue by calling tasklet_disable() which will add an
> usecount and there by avoiding the handlers being called.
> One of the example 'commit 1e1257860fd1
> ("tty/serial: at91: correct the usage of tasklet")'
>
> tasklet_enable/tasklet_disable is a pair API and expected
> to be used together. Usage of tasklet_disable() *just* to
> workround tasklet scheduling after kill is probably not the
> correct and inteded use of the API as done the API.
> We also happen to see similar issue where in shutdown path
> the tasklet_handler was getting called even after the
> tasklet_kill().
>
> We can fix this be making sure tasklet_kill() does right
> thing and there by ensuring tasklet handler won't run after
> tasklet_kil() with very simple change. Patch fixes the tasklet
> code and also few drivers hacks to workaround the issue.
>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Tadeusz Struk <tadeusz.struk@intel.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Paul Bolle <pebolle@tiscali.nl>
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
>
> Signed-off-by: Santosh Shilimkar <ssantosh@kernel.org>
> ---
>  drivers/crypto/qat/qat_common/adf_isr.c    | 1 -
>  drivers/crypto/qat/qat_common/adf_sriov.c  | 1 -
>  drivers/crypto/qat/qat_common/adf_vf_isr.c | 2 --
>  drivers/isdn/gigaset/interface.c           | 1 -
>  drivers/tty/serial/atmel_serial.c          | 1 -
>  kernel/softirq.c                           | 7 ++++---
>  6 files changed, 4 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/crypto/qat/qat_common/adf_isr.c b/drivers/crypto/qat/qat_common/adf_isr.c
> index 06d4901..fd5e900 100644
> --- a/drivers/crypto/qat/qat_common/adf_isr.c
> +++ b/drivers/crypto/qat/qat_common/adf_isr.c
> @@ -296,7 +296,6 @@ static void adf_cleanup_bh(struct adf_accel_dev *accel_dev)
>  	int i;
>
>  	for (i = 0; i < hw_data->num_banks; i++) {
> -		tasklet_disable(&priv_data->banks[i].resp_handler);
>  		tasklet_kill(&priv_data->banks[i].resp_handler);
>  	}
>  }
> diff --git a/drivers/crypto/qat/qat_common/adf_sriov.c b/drivers/crypto/qat/qat_common/adf_sriov.c
> index 4a526e2..9e65888 100644
> --- a/drivers/crypto/qat/qat_common/adf_sriov.c
> +++ b/drivers/crypto/qat/qat_common/adf_sriov.c
> @@ -204,7 +204,6 @@ void adf_disable_sriov(struct adf_accel_dev *accel_dev)
>  	}
>
>  	for (i = 0, vf = accel_dev->pf.vf_info; i < totalvfs; i++, vf++) {
> -		tasklet_disable(&vf->vf2pf_bh_tasklet);
>  		tasklet_kill(&vf->vf2pf_bh_tasklet);
>  		mutex_destroy(&vf->pf2vf_lock);
>  	}
> diff --git a/drivers/crypto/qat/qat_common/adf_vf_isr.c b/drivers/crypto/qat/qat_common/adf_vf_isr.c
> index aa689ca..81e63bf 100644
> --- a/drivers/crypto/qat/qat_common/adf_vf_isr.c
> +++ b/drivers/crypto/qat/qat_common/adf_vf_isr.c
> @@ -191,7 +191,6 @@ static int adf_setup_pf2vf_bh(struct adf_accel_dev *accel_dev)
>
>  static void adf_cleanup_pf2vf_bh(struct adf_accel_dev *accel_dev)
>  {
> -	tasklet_disable(&accel_dev->vf.pf2vf_bh_tasklet);
>  	tasklet_kill(&accel_dev->vf.pf2vf_bh_tasklet);
>  	mutex_destroy(&accel_dev->vf.vf2pf_lock);
>  }
> @@ -268,7 +267,6 @@ static void adf_cleanup_bh(struct adf_accel_dev *accel_dev)
>  {
>  	struct adf_etr_data *priv_data = accel_dev->transport;
>
> -	tasklet_disable(&priv_data->banks[0].resp_handler);
>  	tasklet_kill(&priv_data->banks[0].resp_handler);
>  }
>
> diff --git a/drivers/isdn/gigaset/interface.c b/drivers/isdn/gigaset/interface.c
> index 600c79b..2ce63b6 100644
> --- a/drivers/isdn/gigaset/interface.c
> +++ b/drivers/isdn/gigaset/interface.c
> @@ -524,7 +524,6 @@ void gigaset_if_free(struct cardstate *cs)
>  	if (!drv->have_tty)
>  		return;
>
> -	tasklet_disable(&cs->if_wake_tasklet);
>  	tasklet_kill(&cs->if_wake_tasklet);
>  	cs->tty_dev = NULL;
>  	tty_unregister_device(drv->tty, cs->minor_index);
> diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
> index 954941d..27e638e 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -1915,7 +1915,6 @@ static void atmel_shutdown(struct uart_port *port)
>  	 * Clear out any scheduled tasklets before
>  	 * we destroy the buffers
>  	 */
> -	tasklet_disable(&atmel_port->tasklet);
>  	tasklet_kill(&atmel_port->tasklet);
>
>  	/*
> diff --git a/kernel/softirq.c b/kernel/softirq.c
> index 17caf4b..21397eb 100644
> --- a/kernel/softirq.c
> +++ b/kernel/softirq.c
> @@ -498,7 +498,7 @@ static void tasklet_action(struct softirq_action *a)
>  		list = list->next;
>
>  		if (tasklet_trylock(t)) {
> -			if (!atomic_read(&t->count)) {
> +			if (atomic_read(&t->count) == 1) {
>  				if (!test_and_clear_bit(TASKLET_STATE_SCHED,
>  							&t->state))
>  					BUG();
> @@ -534,7 +534,7 @@ static void tasklet_hi_action(struct softirq_action *a)
>  		list = list->next;
>
>  		if (tasklet_trylock(t)) {
> -			if (!atomic_read(&t->count)) {
> +			if (atomic_read(&t->count) == 1) {
>  				if (!test_and_clear_bit(TASKLET_STATE_SCHED,
>  							&t->state))
>  					BUG();
> @@ -559,7 +559,7 @@ void tasklet_init(struct tasklet_struct *t,
>  {
>  	t->next = NULL;
>  	t->state = 0;
> -	atomic_set(&t->count, 0);
> +	atomic_set(&t->count, 1);
>  	t->func = func;
>  	t->data = data;
>  }
> @@ -576,6 +576,7 @@ void tasklet_kill(struct tasklet_struct *t)
>  		} while (test_bit(TASKLET_STATE_SCHED, &t->state));
>  	}
>  	tasklet_unlock_wait(t);
> +	atomic_dec(&t->count);
>  	clear_bit(TASKLET_STATE_SCHED, &t->state);
>  }
>  EXPORT_SYMBOL(tasklet_kill);
>

[toc] | [prev] | [next] | [standalone]

#1457435

FromGreg Kroah-Hartman <gregkh@linuxfoundation.org>
Date2016-08-07 09:00 +0200
Message-ID<s3qdX-kk-3@gated-at.bofh.it>
In reply to#1457364
On Sat, Aug 06, 2016 at 11:07:14AM -0700, Santosh Shilimkar wrote:
> ping !!

ping on a "RFC" after only 5 days in the middle of the merge window?
That's bold...

Personally, I rarely respond to RFC patches as obviously the submitter
doesn't think they solve the issue at hand :)

[toc] | [prev] | [next] | [standalone]

#1457478

From"santosh.shilimkar@oracle.com" <santosh.shilimkar@oracle.com>
Date2016-08-07 19:10 +0200
Message-ID<s3zKi-6Be-13@gated-at.bofh.it>
In reply to#1457435
On 8/6/16 11:52 PM, Greg Kroah-Hartman wrote:
> On Sat, Aug 06, 2016 at 11:07:14AM -0700, Santosh Shilimkar wrote:
>> ping !!
>
> ping on a "RFC" after only 5 days in the middle of the merge window?
> That's bold...
>
> Personally, I rarely respond to RFC patches as obviously the submitter
> doesn't think they solve the issue at hand :)
>
I see. The patch does solve the problem in this particular case
though and also fixes few drivers miss-use of the api IMHO.
I marked it RFC to get comments on approach but probably should
have just sent as a patch. I will keep that in mind for future.

Regards,
Santosh

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web