subject:"\[PATCH\] media\: rc\: refactor raw handler kthread"

Re: [PATCH] media: rc: refactor raw handler kthread

2016-12-27 Thread Sean Young

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 
> 
> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] media: rc: refactor raw handler kthread

2016-12-26 Thread Heiner Kallweit

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 

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 
> -#include 
>  #include 
>  
>  struct ir_raw_handler {
> @@ -37,7 +36,6 @@ struct ir_raw_handler {
>  struct ir_raw_event_ctrl {
>   struct list_headlist;   /* 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 
>  #include 
>  #include 
> -#include 
>  #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(>lock);
> -
> - if (!kfifo_len(>kfifo)) {
> - set_current_state(TASK_INTERRUPTIBLE);
> -
> - if (kthread_should_stop())
> - set_current_state(TASK_RUNNING);
> -
> - spin_unlock_irq(>lock);
> - schedule();
> - continue;
> + while (1) {
> + mutex_lock(_raw_handler_lock);
> + while (kfifo_out(>kfifo, , 1)) {
> + list_for_each_entry(handler, _raw_handler_list, list)
> + if (raw->dev->enabled_protocols &
> + handler->protocols || !handler->protocols)
> + handler->decode(raw->dev, ev);
> + raw->prev_ev = ev;
>   }
> + mutex_unlock(_raw_handler_lock);
>  
> - if(!kfifo_out(>kfifo, , 1))
> - dev_err(>dev->dev, "IR event FIFO is empty!\n");
> - spin_unlock_irq(>lock);
> + set_current_state(TASK_INTERRUPTIBLE);
>  
> - mutex_lock(_raw_handler_lock);
> - list_for_each_entry(handler, _raw_handler_list, list)
> - if (raw->dev->enabled_protocols & handler->protocols ||
> - !handler->protocols)
> - handler->decode(raw->dev, ev);
> - raw->prev_ev = ev;
> - mutex_unlock(_raw_handler_lock);
> + if (kthread_should_stop()) {
> + __set_current_state(TASK_RUNNING);
> + break;
> + } else if (!kfifo_is_empty(>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(>raw->lock, flags);
>   wake_up_process(dev->raw->thread);
> - spin_unlock_irqrestore(>raw->lock, flags);
>  }
>

Re: [PATCH] media: rc: refactor raw handler kthread

2016-11-25 Thread Sean Young

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 

Looks good to me and I've also tested it.

Tested-by: Sean Young 

> ---
>  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 
> -#include 
>  #include 
>  
>  struct ir_raw_handler {
> @@ -37,7 +36,6 @@ struct ir_raw_handler {
>  struct ir_raw_event_ctrl {
>   struct list_headlist;   /* 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 
>  #include 
>  #include 
> -#include 
>  #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(>lock);
> -
> - if (!kfifo_len(>kfifo)) {
> - set_current_state(TASK_INTERRUPTIBLE);
> -
> - if (kthread_should_stop())
> - set_current_state(TASK_RUNNING);
> -
> - spin_unlock_irq(>lock);
> - schedule();
> - continue;
> + while (1) {
> + mutex_lock(_raw_handler_lock);
> + while (kfifo_out(>kfifo, , 1)) {
> + list_for_each_entry(handler, _raw_handler_list, list)
> + if (raw->dev->enabled_protocols &
> + handler->protocols || !handler->protocols)
> + handler->decode(raw->dev, ev);
> + raw->prev_ev = ev;
>   }
> + mutex_unlock(_raw_handler_lock);
>  
> - if(!kfifo_out(>kfifo, , 1))
> - dev_err(>dev->dev, "IR event FIFO is empty!\n");
> - spin_unlock_irq(>lock);
> + set_current_state(TASK_INTERRUPTIBLE);
>  
> - mutex_lock(_raw_handler_lock);
> - list_for_each_entry(handler, _raw_handler_list, list)
> - if (raw->dev->enabled_protocols & handler->protocols ||
> - !handler->protocols)
> - handler->decode(raw->dev, ev);
> - raw->prev_ev = ev;
> - mutex_unlock(_raw_handler_lock);
> + if (kthread_should_stop()) {
> + __set_current_state(TASK_RUNNING);
> + break;
> + } else if (!kfifo_is_empty(>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(>raw->lock, flags);
>   wake_up_process(dev->raw->thread);
> - spin_unlock_irqrestore(>raw->lock, flags);
>  }
>

[PATCH] media: rc: refactor raw handler kthread

2016-08-01 Thread 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 
---
 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 @@
 #defineMAX_IR_EVENT_SIZE   512
 
 #include 
-#include 
 #include 
 
 struct ir_raw_handler {
@@ -37,7 +36,6 @@ struct ir_raw_handler {
 struct ir_raw_event_ctrl {
struct list_headlist;   /* 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 
 #include 
 #include 
-#include 
 #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(>lock);
-
-   if (!kfifo_len(>kfifo)) {
-   set_current_state(TASK_INTERRUPTIBLE);
-
-   if (kthread_should_stop())
-   set_current_state(TASK_RUNNING);
-
-   spin_unlock_irq(>lock);
-   schedule();
-   continue;
+   while (1) {
+   mutex_lock(_raw_handler_lock);
+   while (kfifo_out(>kfifo, , 1)) {
+   list_for_each_entry(handler, _raw_handler_list, list)
+   if (raw->dev->enabled_protocols &
+   handler->protocols || !handler->protocols)
+   handler->decode(raw->dev, ev);
+   raw->prev_ev = ev;
}
+   mutex_unlock(_raw_handler_lock);
 
-   if(!kfifo_out(>kfifo, , 1))
-   dev_err(>dev->dev, "IR event FIFO is empty!\n");
-   spin_unlock_irq(>lock);
+   set_current_state(TASK_INTERRUPTIBLE);
 
-   mutex_lock(_raw_handler_lock);
-   list_for_each_entry(handler, _raw_handler_list, list)
-   if (raw->dev->enabled_protocols & handler->protocols ||
-   !handler->protocols)
-   handler->decode(raw->dev, ev);
-   raw->prev_ev = ev;
-   mutex_unlock(_raw_handler_lock);
+   if (kthread_should_stop()) {
+   __set_current_state(TASK_RUNNING);
+   break;
+   } else if (!kfifo_is_empty(>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(>raw->lock, flags);
wake_up_process(dev->raw->thread);
-   spin_unlock_irqrestore(>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(>raw->lock);
dev->raw->thread = kthread_run(ir_raw_event_thread, dev->raw,

Re: [PATCH] media: rc: refactor raw handler kthread

Re: [PATCH] media: rc: refactor raw handler kthread

Re: [PATCH] media: rc: refactor raw handler kthread

[PATCH] media: rc: refactor raw handler kthread

4 matches

Site Navigation

Mail list logo

Footer information