* [PATCH] media: rc: refactor raw handler kthread
@ 2016-08-02 5:44 Heiner Kallweit
2016-11-25 9:59 ` Sean Young
2016-12-26 13:01 ` Heiner Kallweit
0 siblings, 2 replies; 4+ messages in thread
From: Heiner Kallweit @ 2016-08-02 5:44 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: linux-media
I think we can get rid of the spinlock protecting the kthread from being
interrupted by a wakeup in certain parts.
Even with the current implementation of the kthread the only lost wakeup
scenario could happen if the wakeup occurs between the kfifo_len check
and setting the state to TASK_INTERRUPTIBLE.
In the changed version we could lose a wakeup if it occurs between
processing the fifo content and setting the state to TASK_INTERRUPTIBLE.
This scenario is covered by an additional check for available events in
the fifo and setting the state to TASK_RUNNING in this case.
In addition the changed version flushes the kfifo before ending
when the kthread is stopped.
With this patch we gain:
- Get rid of the spinlock
- Simplify code
- Don't grep / release the mutex for each individual event but just once
for the complete fifo content. This reduces overhead if a driver e.g.
triggers processing after writing the content of a hw fifo to the kfifo.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/media/rc/rc-core-priv.h | 2 --
drivers/media/rc/rc-ir-raw.c | 46 +++++++++++++++--------------------------
2 files changed, 17 insertions(+), 31 deletions(-)
diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
index 585d5e5..577128a 100644
--- a/drivers/media/rc/rc-core-priv.h
+++ b/drivers/media/rc/rc-core-priv.h
@@ -20,7 +20,6 @@
#define MAX_IR_EVENT_SIZE 512
#include <linux/slab.h>
-#include <linux/spinlock.h>
#include <media/rc-core.h>
struct ir_raw_handler {
@@ -37,7 +36,6 @@ struct ir_raw_handler {
struct ir_raw_event_ctrl {
struct list_head list; /* to keep track of raw clients */
struct task_struct *thread;
- spinlock_t lock;
/* fifo for the pulse/space durations */
DECLARE_KFIFO(kfifo, struct ir_raw_event, MAX_IR_EVENT_SIZE);
ktime_t last_event; /* when last event occurred */
diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
index 205ecc6..71a3e17 100644
--- a/drivers/media/rc/rc-ir-raw.c
+++ b/drivers/media/rc/rc-ir-raw.c
@@ -17,7 +17,6 @@
#include <linux/mutex.h>
#include <linux/kmod.h>
#include <linux/sched.h>
-#include <linux/freezer.h>
#include "rc-core-priv.h"
/* Used to keep track of IR raw clients, protected by ir_raw_handler_lock */
@@ -35,32 +34,26 @@ static int ir_raw_event_thread(void *data)
struct ir_raw_handler *handler;
struct ir_raw_event_ctrl *raw = (struct ir_raw_event_ctrl *)data;
- while (!kthread_should_stop()) {
-
- spin_lock_irq(&raw->lock);
-
- if (!kfifo_len(&raw->kfifo)) {
- set_current_state(TASK_INTERRUPTIBLE);
-
- if (kthread_should_stop())
- set_current_state(TASK_RUNNING);
-
- spin_unlock_irq(&raw->lock);
- schedule();
- continue;
+ while (1) {
+ mutex_lock(&ir_raw_handler_lock);
+ while (kfifo_out(&raw->kfifo, &ev, 1)) {
+ list_for_each_entry(handler, &ir_raw_handler_list, list)
+ if (raw->dev->enabled_protocols &
+ handler->protocols || !handler->protocols)
+ handler->decode(raw->dev, ev);
+ raw->prev_ev = ev;
}
+ mutex_unlock(&ir_raw_handler_lock);
- if(!kfifo_out(&raw->kfifo, &ev, 1))
- dev_err(&raw->dev->dev, "IR event FIFO is empty!\n");
- spin_unlock_irq(&raw->lock);
+ set_current_state(TASK_INTERRUPTIBLE);
- mutex_lock(&ir_raw_handler_lock);
- list_for_each_entry(handler, &ir_raw_handler_list, list)
- if (raw->dev->enabled_protocols & handler->protocols ||
- !handler->protocols)
- handler->decode(raw->dev, ev);
- raw->prev_ev = ev;
- mutex_unlock(&ir_raw_handler_lock);
+ if (kthread_should_stop()) {
+ __set_current_state(TASK_RUNNING);
+ break;
+ } else if (!kfifo_is_empty(&raw->kfifo))
+ set_current_state(TASK_RUNNING);
+
+ schedule();
}
return 0;
@@ -219,14 +212,10 @@ EXPORT_SYMBOL_GPL(ir_raw_event_set_idle);
*/
void ir_raw_event_handle(struct rc_dev *dev)
{
- unsigned long flags;
-
if (!dev->raw)
return;
- spin_lock_irqsave(&dev->raw->lock, flags);
wake_up_process(dev->raw->thread);
- spin_unlock_irqrestore(&dev->raw->lock, flags);
}
EXPORT_SYMBOL_GPL(ir_raw_event_handle);
@@ -274,7 +263,6 @@ int ir_raw_event_register(struct rc_dev *dev)
dev->change_protocol = change_protocol;
INIT_KFIFO(dev->raw->kfifo);
- spin_lock_init(&dev->raw->lock);
dev->raw->thread = kthread_run(ir_raw_event_thread, dev->raw,
"rc%u", dev->minor);
--
2.9.0
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] media: rc: refactor raw handler kthread
2016-08-02 5:44 [PATCH] media: rc: refactor raw handler kthread Heiner Kallweit
@ 2016-11-25 9:59 ` Sean Young
2016-12-26 13:01 ` Heiner Kallweit
1 sibling, 0 replies; 4+ messages in thread
From: Sean Young @ 2016-11-25 9:59 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: Mauro Carvalho Chehab, linux-media
On Tue, Aug 02, 2016 at 07:44:07AM +0200, Heiner Kallweit wrote:
> I think we can get rid of the spinlock protecting the kthread from being
> interrupted by a wakeup in certain parts.
> Even with the current implementation of the kthread the only lost wakeup
> scenario could happen if the wakeup occurs between the kfifo_len check
> and setting the state to TASK_INTERRUPTIBLE.
>
> In the changed version we could lose a wakeup if it occurs between
> processing the fifo content and setting the state to TASK_INTERRUPTIBLE.
> This scenario is covered by an additional check for available events in
> the fifo and setting the state to TASK_RUNNING in this case.
>
> In addition the changed version flushes the kfifo before ending
> when the kthread is stopped.
>
> With this patch we gain:
> - Get rid of the spinlock
> - Simplify code
> - Don't grep / release the mutex for each individual event but just once
> for the complete fifo content. This reduces overhead if a driver e.g.
> triggers processing after writing the content of a hw fifo to the kfifo.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Looks good to me and I've also tested it.
Tested-by: Sean Young <sean@mess.org>
> ---
> drivers/media/rc/rc-core-priv.h | 2 --
> drivers/media/rc/rc-ir-raw.c | 46 +++++++++++++++--------------------------
> 2 files changed, 17 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
> index 585d5e5..577128a 100644
> --- a/drivers/media/rc/rc-core-priv.h
> +++ b/drivers/media/rc/rc-core-priv.h
> @@ -20,7 +20,6 @@
> #define MAX_IR_EVENT_SIZE 512
>
> #include <linux/slab.h>
> -#include <linux/spinlock.h>
> #include <media/rc-core.h>
>
> struct ir_raw_handler {
> @@ -37,7 +36,6 @@ struct ir_raw_handler {
> struct ir_raw_event_ctrl {
> struct list_head list; /* to keep track of raw clients */
> struct task_struct *thread;
> - spinlock_t lock;
> /* fifo for the pulse/space durations */
> DECLARE_KFIFO(kfifo, struct ir_raw_event, MAX_IR_EVENT_SIZE);
> ktime_t last_event; /* when last event occurred */
> diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
> index 205ecc6..71a3e17 100644
> --- a/drivers/media/rc/rc-ir-raw.c
> +++ b/drivers/media/rc/rc-ir-raw.c
> @@ -17,7 +17,6 @@
> #include <linux/mutex.h>
> #include <linux/kmod.h>
> #include <linux/sched.h>
> -#include <linux/freezer.h>
> #include "rc-core-priv.h"
>
> /* Used to keep track of IR raw clients, protected by ir_raw_handler_lock */
> @@ -35,32 +34,26 @@ static int ir_raw_event_thread(void *data)
> struct ir_raw_handler *handler;
> struct ir_raw_event_ctrl *raw = (struct ir_raw_event_ctrl *)data;
>
> - while (!kthread_should_stop()) {
> -
> - spin_lock_irq(&raw->lock);
> -
> - if (!kfifo_len(&raw->kfifo)) {
> - set_current_state(TASK_INTERRUPTIBLE);
> -
> - if (kthread_should_stop())
> - set_current_state(TASK_RUNNING);
> -
> - spin_unlock_irq(&raw->lock);
> - schedule();
> - continue;
> + while (1) {
> + mutex_lock(&ir_raw_handler_lock);
> + while (kfifo_out(&raw->kfifo, &ev, 1)) {
> + list_for_each_entry(handler, &ir_raw_handler_list, list)
> + if (raw->dev->enabled_protocols &
> + handler->protocols || !handler->protocols)
> + handler->decode(raw->dev, ev);
> + raw->prev_ev = ev;
> }
> + mutex_unlock(&ir_raw_handler_lock);
>
> - if(!kfifo_out(&raw->kfifo, &ev, 1))
> - dev_err(&raw->dev->dev, "IR event FIFO is empty!\n");
> - spin_unlock_irq(&raw->lock);
> + set_current_state(TASK_INTERRUPTIBLE);
>
> - mutex_lock(&ir_raw_handler_lock);
> - list_for_each_entry(handler, &ir_raw_handler_list, list)
> - if (raw->dev->enabled_protocols & handler->protocols ||
> - !handler->protocols)
> - handler->decode(raw->dev, ev);
> - raw->prev_ev = ev;
> - mutex_unlock(&ir_raw_handler_lock);
> + if (kthread_should_stop()) {
> + __set_current_state(TASK_RUNNING);
> + break;
> + } else if (!kfifo_is_empty(&raw->kfifo))
> + set_current_state(TASK_RUNNING);
> +
> + schedule();
> }
>
> return 0;
> @@ -219,14 +212,10 @@ EXPORT_SYMBOL_GPL(ir_raw_event_set_idle);
> */
> void ir_raw_event_handle(struct rc_dev *dev)
> {
> - unsigned long flags;
> -
> if (!dev->raw)
> return;
>
> - spin_lock_irqsave(&dev->raw->lock, flags);
> wake_up_process(dev->raw->thread);
> - spin_unlock_irqrestore(&dev->raw->lock, flags);
> }
> EXPORT_SYMBOL_GPL(ir_raw_event_handle);
>
> @@ -274,7 +263,6 @@ int ir_raw_event_register(struct rc_dev *dev)
> dev->change_protocol = change_protocol;
> INIT_KFIFO(dev->raw->kfifo);
>
> - spin_lock_init(&dev->raw->lock);
> dev->raw->thread = kthread_run(ir_raw_event_thread, dev->raw,
> "rc%u", dev->minor);
>
> --
> 2.9.0
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] media: rc: refactor raw handler kthread
2016-08-02 5:44 [PATCH] media: rc: refactor raw handler kthread Heiner Kallweit
2016-11-25 9:59 ` Sean Young
@ 2016-12-26 13:01 ` Heiner Kallweit
2016-12-27 11:17 ` Sean Young
1 sibling, 1 reply; 4+ messages in thread
From: Heiner Kallweit @ 2016-12-26 13:01 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: linux-media, Sean Young
Am 02.08.2016 um 07:44 schrieb Heiner Kallweit:
> I think we can get rid of the spinlock protecting the kthread from being
> interrupted by a wakeup in certain parts.
> Even with the current implementation of the kthread the only lost wakeup
> scenario could happen if the wakeup occurs between the kfifo_len check
> and setting the state to TASK_INTERRUPTIBLE.
>
> In the changed version we could lose a wakeup if it occurs between
> processing the fifo content and setting the state to TASK_INTERRUPTIBLE.
> This scenario is covered by an additional check for available events in
> the fifo and setting the state to TASK_RUNNING in this case.
>
> In addition the changed version flushes the kfifo before ending
> when the kthread is stopped.
>
> With this patch we gain:
> - Get rid of the spinlock
> - Simplify code
> - Don't grep / release the mutex for each individual event but just once
> for the complete fifo content. This reduces overhead if a driver e.g.
> triggers processing after writing the content of a hw fifo to the kfifo.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Sean added a review comment and his "Tested-by" a month ago.
Anything else missing before it can be applied?
> ---
> drivers/media/rc/rc-core-priv.h | 2 --
> drivers/media/rc/rc-ir-raw.c | 46 +++++++++++++++--------------------------
> 2 files changed, 17 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
> index 585d5e5..577128a 100644
> --- a/drivers/media/rc/rc-core-priv.h
> +++ b/drivers/media/rc/rc-core-priv.h
> @@ -20,7 +20,6 @@
> #define MAX_IR_EVENT_SIZE 512
>
> #include <linux/slab.h>
> -#include <linux/spinlock.h>
> #include <media/rc-core.h>
>
> struct ir_raw_handler {
> @@ -37,7 +36,6 @@ struct ir_raw_handler {
> struct ir_raw_event_ctrl {
> struct list_head list; /* to keep track of raw clients */
> struct task_struct *thread;
> - spinlock_t lock;
> /* fifo for the pulse/space durations */
> DECLARE_KFIFO(kfifo, struct ir_raw_event, MAX_IR_EVENT_SIZE);
> ktime_t last_event; /* when last event occurred */
> diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
> index 205ecc6..71a3e17 100644
> --- a/drivers/media/rc/rc-ir-raw.c
> +++ b/drivers/media/rc/rc-ir-raw.c
> @@ -17,7 +17,6 @@
> #include <linux/mutex.h>
> #include <linux/kmod.h>
> #include <linux/sched.h>
> -#include <linux/freezer.h>
> #include "rc-core-priv.h"
>
> /* Used to keep track of IR raw clients, protected by ir_raw_handler_lock */
> @@ -35,32 +34,26 @@ static int ir_raw_event_thread(void *data)
> struct ir_raw_handler *handler;
> struct ir_raw_event_ctrl *raw = (struct ir_raw_event_ctrl *)data;
>
> - while (!kthread_should_stop()) {
> -
> - spin_lock_irq(&raw->lock);
> -
> - if (!kfifo_len(&raw->kfifo)) {
> - set_current_state(TASK_INTERRUPTIBLE);
> -
> - if (kthread_should_stop())
> - set_current_state(TASK_RUNNING);
> -
> - spin_unlock_irq(&raw->lock);
> - schedule();
> - continue;
> + while (1) {
> + mutex_lock(&ir_raw_handler_lock);
> + while (kfifo_out(&raw->kfifo, &ev, 1)) {
> + list_for_each_entry(handler, &ir_raw_handler_list, list)
> + if (raw->dev->enabled_protocols &
> + handler->protocols || !handler->protocols)
> + handler->decode(raw->dev, ev);
> + raw->prev_ev = ev;
> }
> + mutex_unlock(&ir_raw_handler_lock);
>
> - if(!kfifo_out(&raw->kfifo, &ev, 1))
> - dev_err(&raw->dev->dev, "IR event FIFO is empty!\n");
> - spin_unlock_irq(&raw->lock);
> + set_current_state(TASK_INTERRUPTIBLE);
>
> - mutex_lock(&ir_raw_handler_lock);
> - list_for_each_entry(handler, &ir_raw_handler_list, list)
> - if (raw->dev->enabled_protocols & handler->protocols ||
> - !handler->protocols)
> - handler->decode(raw->dev, ev);
> - raw->prev_ev = ev;
> - mutex_unlock(&ir_raw_handler_lock);
> + if (kthread_should_stop()) {
> + __set_current_state(TASK_RUNNING);
> + break;
> + } else if (!kfifo_is_empty(&raw->kfifo))
> + set_current_state(TASK_RUNNING);
> +
> + schedule();
> }
>
> return 0;
> @@ -219,14 +212,10 @@ EXPORT_SYMBOL_GPL(ir_raw_event_set_idle);
> */
> void ir_raw_event_handle(struct rc_dev *dev)
> {
> - unsigned long flags;
> -
> if (!dev->raw)
> return;
>
> - spin_lock_irqsave(&dev->raw->lock, flags);
> wake_up_process(dev->raw->thread);
> - spin_unlock_irqrestore(&dev->raw->lock, flags);
> }
> EXPORT_SYMBOL_GPL(ir_raw_event_handle);
>
> @@ -274,7 +263,6 @@ int ir_raw_event_register(struct rc_dev *dev)
> dev->change_protocol = change_protocol;
> INIT_KFIFO(dev->raw->kfifo);
>
> - spin_lock_init(&dev->raw->lock);
> dev->raw->thread = kthread_run(ir_raw_event_thread, dev->raw,
> "rc%u", dev->minor);
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] media: rc: refactor raw handler kthread
2016-12-26 13:01 ` Heiner Kallweit
@ 2016-12-27 11:17 ` Sean Young
0 siblings, 0 replies; 4+ messages in thread
From: Sean Young @ 2016-12-27 11:17 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: Mauro Carvalho Chehab, linux-media
On Mon, Dec 26, 2016 at 02:01:31PM +0100, Heiner Kallweit wrote:
> Am 02.08.2016 um 07:44 schrieb Heiner Kallweit:
> > I think we can get rid of the spinlock protecting the kthread from being
> > interrupted by a wakeup in certain parts.
> > Even with the current implementation of the kthread the only lost wakeup
> > scenario could happen if the wakeup occurs between the kfifo_len check
> > and setting the state to TASK_INTERRUPTIBLE.
> >
> > In the changed version we could lose a wakeup if it occurs between
> > processing the fifo content and setting the state to TASK_INTERRUPTIBLE.
> > This scenario is covered by an additional check for available events in
> > the fifo and setting the state to TASK_RUNNING in this case.
> >
> > In addition the changed version flushes the kfifo before ending
> > when the kthread is stopped.
> >
> > With this patch we gain:
> > - Get rid of the spinlock
> > - Simplify code
> > - Don't grep / release the mutex for each individual event but just once
> > for the complete fifo content. This reduces overhead if a driver e.g.
> > triggers processing after writing the content of a hw fifo to the kfifo.
> >
> > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>
> Sean added a review comment and his "Tested-by" a month ago.
> Anything else missing before it can be applied?
I have it applied here:
https://git.linuxtv.org/syoung/media_tree.git/log/?h=for-v4.11a
I'll ask Mauro to pull that tree soon, now that 4.10-rc1 has been
merged. I need to do some testing.
Sean
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2016-12-27 11:17 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-08-02 5:44 [PATCH] media: rc: refactor raw handler kthread Heiner Kallweit
2016-11-25 9:59 ` Sean Young
2016-12-26 13:01 ` Heiner Kallweit
2016-12-27 11:17 ` Sean Young
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.