[PATCH v2] psmouse: run kpsmoused only while needed

[PATCH v2] psmouse: run kpsmoused only while needed

[Frederic Weisbecker]

While looking at the workqueue tracer, I noticed that kpsmoused receives
rarely (if not never) events.

Currently, when a mouse has to resync, it uses the kpsmoused singlethreaded
workqueue. But resync are rare. While reading an old discussion, it seems
that usual workqueue events can't be used for that purpose because resync
can take too much time and could delay the other works in queue.

But if you have built psmouse driver, this workqueue will always be present
whether you have a ps/2 port or not. And its events are rare.

To avoid this pointless task, this patch makes the kpsmoused a kernel
thread only created on the fly when a resync is needed. Once the resync is done,
this thread will die. So you will almost never see it, and it will not be
an inactive task anymore.

This thread is created through a usual workqueue event (because we can't create
it from interrupt).

Changes in V2:

_ fix the "resync" mispelled in the patch and the changelog
_ don't schedule more than one resync in case of concurrent interrupts
_ if resync_pending is not cleared after a few time before disconnect, print a warning.

Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
---
 drivers/input/mouse/psmouse-base.c |   86 ++++++++++++++++++++++++------------
 drivers/input/mouse/psmouse.h      |    5 ++-
 2 files changed, 61 insertions(+), 30 deletions(-)

diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c
index f8f86de..fc4490e 100644
--- a/drivers/input/mouse/psmouse-base.c
+++ b/drivers/input/mouse/psmouse-base.c
@@ -20,6 +20,7 @@
 #include <linux/init.h>
 #include <linux/libps2.h>
 #include <linux/mutex.h>
+#include <linux/kthread.h>
 
 #include "psmouse.h"
 #include "synaptics.h"
@@ -104,8 +105,6 @@ static struct attribute_group psmouse_attribute_group = {
  */
 static DEFINE_MUTEX(psmouse_mutex);
 
-static struct workqueue_struct *kpsmoused_wq;
-
 struct psmouse_protocol {
 	enum psmouse_type type;
 	const char *name;
@@ -203,11 +202,6 @@ static psmouse_ret_t psmouse_process_byte(struct psmouse *psmouse)
 	return PSMOUSE_FULL_PACKET;
 }
 
-void psmouse_queue_work(struct psmouse *psmouse, struct delayed_work *work,
-		unsigned long delay)
-{
-	queue_delayed_work(kpsmoused_wq, work, delay);
-}
 
 /*
  * __psmouse_set_state() sets new psmouse state and resets all flags.
@@ -313,7 +307,10 @@ static irqreturn_t psmouse_interrupt(struct serio *serio,
 		       psmouse->name, psmouse->phys, psmouse->pktcnt);
 		psmouse->badbyte = psmouse->packet[0];
 		__psmouse_set_state(psmouse, PSMOUSE_RESYNCING);
-		psmouse_queue_work(psmouse, &psmouse->resync_work, 0);
+		if (atomic_inc_return(&psmouse->resync_pending) == 1)
+			schedule_work(&psmouse->resync_work);
+		else
+			atomic_dec(&psmouse->resync_pending);
 		goto out;
 	}
 
@@ -350,7 +347,11 @@ static irqreturn_t psmouse_interrupt(struct serio *serio,
 	    time_after(jiffies, psmouse->last + psmouse->resync_time * HZ)) {
 		psmouse->badbyte = psmouse->packet[0];
 		__psmouse_set_state(psmouse, PSMOUSE_RESYNCING);
-		psmouse_queue_work(psmouse, &psmouse->resync_work, 0);
+
+		if (atomic_inc_return(&psmouse->resync_pending) == 1)
+			schedule_work(&psmouse->resync_work);
+		else
+			atomic_dec(&psmouse->resync_pending);
 		goto out;
 	}
 
@@ -979,13 +980,16 @@ static int psmouse_poll(struct psmouse *psmouse)
 
 
 /*
- * psmouse_resync() attempts to re-validate current protocol.
+ * psmouse_resync_thread() attempts to re-validate current protocol.
+ * This thread is created on the fly when needed because its job can take too
+ * much time on events workqueues, and the resync is rare enough to avoid
+ * the need of a private workqueue.
  */
 
-static void psmouse_resync(struct work_struct *work)
+static int psmouse_resync_thread(void *v)
 {
-	struct psmouse *parent = NULL, *psmouse =
-		container_of(work, struct psmouse, resync_work.work);
+	struct psmouse *psmouse = v;
+	struct psmouse *parent = NULL;
 	struct serio *serio = psmouse->ps2dev.serio;
 	psmouse_ret_t rc = PSMOUSE_GOOD_DATA;
 	int failed = 0, enabled = 0;
@@ -1072,6 +1076,30 @@ static void psmouse_resync(struct work_struct *work)
 		psmouse_activate(parent);
  out:
 	mutex_unlock(&psmouse_mutex);
+
+	/*
+	 * While disconnecting, the driver wants to be sure all resync are done
+	 */
+	atomic_dec(&psmouse->resync_pending);
+	wake_up(&psmouse->resync_pending_queue);
+	return 0;
+}
+
+/* Launch the resync thread */
+static void psmouse_resync_thread_helper(struct work_struct *work)
+{
+	struct task_struct *t;
+	struct psmouse *psmouse;
+
+	psmouse = container_of(work, struct psmouse, resync_work);
+
+	t = kthread_run(psmouse_resync_thread, psmouse, "kpsmoused");
+	if (t == ERR_PTR(-ENOMEM)) {
+		printk(KERN_WARNING "psmouse.c: failed to create kpsmoused"
+				    " thread\n");
+		atomic_dec(&psmouse->resync_pending);
+		wake_up(&psmouse->resync_pending_queue);
+	}
 }
 
 /*
@@ -1120,6 +1148,8 @@ static void psmouse_cleanup(struct serio *serio)
 static void psmouse_disconnect(struct serio *serio)
 {
 	struct psmouse *psmouse, *parent = NULL;
+	long timeout;
+	DEFINE_WAIT(wait);
 
 	psmouse = serio_get_drvdata(serio);
 
@@ -1131,7 +1161,17 @@ static void psmouse_disconnect(struct serio *serio)
 
 	/* make sure we don't have a resync in progress */
 	mutex_unlock(&psmouse_mutex);
-	flush_workqueue(kpsmoused_wq);
+
+	prepare_to_wait(&psmouse->resync_pending_queue, &wait,
+				TASK_UNINTERRUPTIBLE);
+	/* 4 seconds are more than enough to ensure a resync is completed */
+	if (atomic_read(&psmouse->resync_pending)) {
+		timeout = schedule_timeout(HZ * 4);
+		if (!timeout)
+			WARN_ON(1);
+	}
+	finish_wait(&psmouse->resync_pending_queue, &wait);
+
 	mutex_lock(&psmouse_mutex);
 
 	if (serio->parent && serio->id.type == SERIO_PS_PSTHRU) {
@@ -1244,7 +1284,8 @@ static int psmouse_connect(struct serio *serio, struct serio_driver *drv)
 		goto err_free;
 
 	ps2_init(&psmouse->ps2dev, serio);
-	INIT_DELAYED_WORK(&psmouse->resync_work, psmouse_resync);
+	INIT_WORK(&psmouse->resync_work, psmouse_resync_thread_helper);
+	init_waitqueue_head(&psmouse->resync_pending_queue);
 	psmouse->dev = input_dev;
 	snprintf(psmouse->phys, sizeof(psmouse->phys), "%s/input0", serio->phys);
 
@@ -1647,25 +1688,12 @@ static int psmouse_get_maxproto(char *buffer, struct kernel_param *kp)
 
 static int __init psmouse_init(void)
 {
-	int err;
-
-	kpsmoused_wq = create_singlethread_workqueue("kpsmoused");
-	if (!kpsmoused_wq) {
-		printk(KERN_ERR "psmouse: failed to create kpsmoused workqueue\n");
-		return -ENOMEM;
-	}
-
-	err = serio_register_driver(&psmouse_drv);
-	if (err)
-		destroy_workqueue(kpsmoused_wq);
-
-	return err;
+	return serio_register_driver(&psmouse_drv);
 }
 
 static void __exit psmouse_exit(void)
 {
 	serio_unregister_driver(&psmouse_drv);
-	destroy_workqueue(kpsmoused_wq);
 }
 
 module_init(psmouse_init);
diff --git a/drivers/input/mouse/psmouse.h b/drivers/input/mouse/psmouse.h
index 54ed267..dfb9b8c 100644
--- a/drivers/input/mouse/psmouse.h
+++ b/drivers/input/mouse/psmouse.h
@@ -39,7 +39,10 @@ struct psmouse {
 	void *private;
 	struct input_dev *dev;
 	struct ps2dev ps2dev;
-	struct delayed_work resync_work;
+	struct work_struct resync_work;
+	/* help to wait for mouse resync jobs before disconnect */
+	atomic_t resync_pending;
+	wait_queue_head_t resync_pending_queue;
 	char *vendor;
 	char *name;
 	unsigned char packet[8];
-- 
1.6.1

Re: [PATCH v2] psmouse: run kpsmoused only while needed

[Frédéric Weisbecker]

2009/2/1 Frederic Weisbecker <fweisbec@gmail.com>:
> While looking at the workqueue tracer, I noticed that kpsmoused receives
> rarely (if not never) events.
>
> Currently, when a mouse has to resync, it uses the kpsmoused singlethreaded
> workqueue. But resync are rare. While reading an old discussion, it seems
> that usual workqueue events can't be used for that purpose because resync
> can take too much time and could delay the other works in queue.
>
> But if you have built psmouse driver, this workqueue will always be present
> whether you have a ps/2 port or not. And its events are rare.
>
> To avoid this pointless task, this patch makes the kpsmoused a kernel
> thread only created on the fly when a resync is needed. Once the resync is done,
> this thread will die. So you will almost never see it, and it will not be
> an inactive task anymore.
>
> This thread is created through a usual workqueue event (because we can't create
> it from interrupt).
>
> Changes in V2:
>
> _ fix the "resync" mispelled in the patch and the changelog
> _ don't schedule more than one resync in case of concurrent interrupts
> _ if resync_pending is not cleared after a few time before disconnect, print a warning.
>
> Signed-off-by: Frederic Weisbecker <fweisbec@gmail.com>
> ---
>  drivers/input/mouse/psmouse-base.c |   86 ++++++++++++++++++++++++------------
>  drivers/input/mouse/psmouse.h      |    5 ++-
>  2 files changed, 61 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c
> index f8f86de..fc4490e 100644
> --- a/drivers/input/mouse/psmouse-base.c
> +++ b/drivers/input/mouse/psmouse-base.c
> @@ -20,6 +20,7 @@
>  #include <linux/init.h>
>  #include <linux/libps2.h>
>  #include <linux/mutex.h>
> +#include <linux/kthread.h>
>
>  #include "psmouse.h"
>  #include "synaptics.h"
> @@ -104,8 +105,6 @@ static struct attribute_group psmouse_attribute_group = {
>  */
>  static DEFINE_MUTEX(psmouse_mutex);
>
> -static struct workqueue_struct *kpsmoused_wq;
> -
>  struct psmouse_protocol {
>        enum psmouse_type type;
>        const char *name;
> @@ -203,11 +202,6 @@ static psmouse_ret_t psmouse_process_byte(struct psmouse *psmouse)
>        return PSMOUSE_FULL_PACKET;
>  }
>
> -void psmouse_queue_work(struct psmouse *psmouse, struct delayed_work *work,
> -               unsigned long delay)
> -{
> -       queue_delayed_work(kpsmoused_wq, work, delay);
> -}
>
>  /*
>  * __psmouse_set_state() sets new psmouse state and resets all flags.
> @@ -313,7 +307,10 @@ static irqreturn_t psmouse_interrupt(struct serio *serio,
>                       psmouse->name, psmouse->phys, psmouse->pktcnt);
>                psmouse->badbyte = psmouse->packet[0];
>                __psmouse_set_state(psmouse, PSMOUSE_RESYNCING);
> -               psmouse_queue_work(psmouse, &psmouse->resync_work, 0);
> +               if (atomic_inc_return(&psmouse->resync_pending) == 1)
> +                       schedule_work(&psmouse->resync_work);
> +               else
> +                       atomic_dec(&psmouse->resync_pending);
>                goto out;
>        }
>
> @@ -350,7 +347,11 @@ static irqreturn_t psmouse_interrupt(struct serio *serio,
>            time_after(jiffies, psmouse->last + psmouse->resync_time * HZ)) {
>                psmouse->badbyte = psmouse->packet[0];
>                __psmouse_set_state(psmouse, PSMOUSE_RESYNCING);
> -               psmouse_queue_work(psmouse, &psmouse->resync_work, 0);
> +
> +               if (atomic_inc_return(&psmouse->resync_pending) == 1)
> +                       schedule_work(&psmouse->resync_work);
> +               else
> +                       atomic_dec(&psmouse->resync_pending);
>                goto out;
>        }
>
> @@ -979,13 +980,16 @@ static int psmouse_poll(struct psmouse *psmouse)
>
>
>  /*
> - * psmouse_resync() attempts to re-validate current protocol.
> + * psmouse_resync_thread() attempts to re-validate current protocol.
> + * This thread is created on the fly when needed because its job can take too
> + * much time on events workqueues, and the resync is rare enough to avoid
> + * the need of a private workqueue.
>  */
>
> -static void psmouse_resync(struct work_struct *work)
> +static int psmouse_resync_thread(void *v)
>  {
> -       struct psmouse *parent = NULL, *psmouse =
> -               container_of(work, struct psmouse, resync_work.work);
> +       struct psmouse *psmouse = v;
> +       struct psmouse *parent = NULL;
>        struct serio *serio = psmouse->ps2dev.serio;
>        psmouse_ret_t rc = PSMOUSE_GOOD_DATA;
>        int failed = 0, enabled = 0;
> @@ -1072,6 +1076,30 @@ static void psmouse_resync(struct work_struct *work)
>                psmouse_activate(parent);
>  out:
>        mutex_unlock(&psmouse_mutex);
> +
> +       /*
> +        * While disconnecting, the driver wants to be sure all resync are done
> +        */
> +       atomic_dec(&psmouse->resync_pending);
> +       wake_up(&psmouse->resync_pending_queue);
> +       return 0;
> +}
> +
> +/* Launch the resync thread */
> +static void psmouse_resync_thread_helper(struct work_struct *work)
> +{
> +       struct task_struct *t;
> +       struct psmouse *psmouse;
> +
> +       psmouse = container_of(work, struct psmouse, resync_work);
> +
> +       t = kthread_run(psmouse_resync_thread, psmouse, "kpsmoused");
> +       if (t == ERR_PTR(-ENOMEM)) {
> +               printk(KERN_WARNING "psmouse.c: failed to create kpsmoused"
> +                                   " thread\n");
> +               atomic_dec(&psmouse->resync_pending);
> +               wake_up(&psmouse->resync_pending_queue);
> +       }
>  }
>
>  /*
> @@ -1120,6 +1148,8 @@ static void psmouse_cleanup(struct serio *serio)
>  static void psmouse_disconnect(struct serio *serio)
>  {
>        struct psmouse *psmouse, *parent = NULL;
> +       long timeout;
> +       DEFINE_WAIT(wait);
>
>        psmouse = serio_get_drvdata(serio);
>
> @@ -1131,7 +1161,17 @@ static void psmouse_disconnect(struct serio *serio)
>
>        /* make sure we don't have a resync in progress */
>        mutex_unlock(&psmouse_mutex);
> -       flush_workqueue(kpsmoused_wq);
> +
> +       prepare_to_wait(&psmouse->resync_pending_queue, &wait,
> +                               TASK_UNINTERRUPTIBLE);
> +       /* 4 seconds are more than enough to ensure a resync is completed */
> +       if (atomic_read(&psmouse->resync_pending)) {
> +               timeout = schedule_timeout(HZ * 4);
> +               if (!timeout)
> +                       WARN_ON(1);
> +       }
> +       finish_wait(&psmouse->resync_pending_queue, &wait);
> +
>        mutex_lock(&psmouse_mutex);
>
>        if (serio->parent && serio->id.type == SERIO_PS_PSTHRU) {
> @@ -1244,7 +1284,8 @@ static int psmouse_connect(struct serio *serio, struct serio_driver *drv)
>                goto err_free;
>
>        ps2_init(&psmouse->ps2dev, serio);
> -       INIT_DELAYED_WORK(&psmouse->resync_work, psmouse_resync);
> +       INIT_WORK(&psmouse->resync_work, psmouse_resync_thread_helper);
> +       init_waitqueue_head(&psmouse->resync_pending_queue);
>        psmouse->dev = input_dev;
>        snprintf(psmouse->phys, sizeof(psmouse->phys), "%s/input0", serio->phys);
>
> @@ -1647,25 +1688,12 @@ static int psmouse_get_maxproto(char *buffer, struct kernel_param *kp)
>
>  static int __init psmouse_init(void)
>  {
> -       int err;
> -
> -       kpsmoused_wq = create_singlethread_workqueue("kpsmoused");
> -       if (!kpsmoused_wq) {
> -               printk(KERN_ERR "psmouse: failed to create kpsmoused workqueue\n");
> -               return -ENOMEM;
> -       }
> -
> -       err = serio_register_driver(&psmouse_drv);
> -       if (err)
> -               destroy_workqueue(kpsmoused_wq);
> -
> -       return err;
> +       return serio_register_driver(&psmouse_drv);
>  }
>
>  static void __exit psmouse_exit(void)
>  {
>        serio_unregister_driver(&psmouse_drv);
> -       destroy_workqueue(kpsmoused_wq);
>  }
>
>  module_init(psmouse_init);
> diff --git a/drivers/input/mouse/psmouse.h b/drivers/input/mouse/psmouse.h
> index 54ed267..dfb9b8c 100644
> --- a/drivers/input/mouse/psmouse.h
> +++ b/drivers/input/mouse/psmouse.h
> @@ -39,7 +39,10 @@ struct psmouse {
>        void *private;
>        struct input_dev *dev;
>        struct ps2dev ps2dev;
> -       struct delayed_work resync_work;
> +       struct work_struct resync_work;
> +       /* help to wait for mouse resync jobs before disconnect */
> +       atomic_t resync_pending;
> +       wait_queue_head_t resync_pending_queue;
>        char *vendor;
>        char *name;
>        unsigned char packet[8];


It looks like Dmitry is busy these times.
Perhaps someone else can carry this patch?

Thanks.

Re: [PATCH v2] psmouse: run kpsmoused only while needed

[Andrew Morton]

On Sat, 31 Jan 2009 16:27:22 -0800 (PST)
Frederic Weisbecker <fweisbec@gmail.com> wrote:

> While looking at the workqueue tracer, I noticed that kpsmoused receives
> rarely (if not never) events.
> 
> Currently, when a mouse has to resync, it uses the kpsmoused singlethreaded
> workqueue. But resync are rare. While reading an old discussion, it seems
> that usual workqueue events can't be used for that purpose because resync
> can take too much time and could delay the other works in queue.
> 
> But if you have built psmouse driver, this workqueue will always be present
> whether you have a ps/2 port or not. And its events are rare.
> 
> To avoid this pointless task, this patch makes the kpsmoused a kernel
> thread only created on the fly when a resync is needed. Once the resync is done,
> this thread will die. So you will almost never see it, and it will not be
> an inactive task anymore.
> 
> This thread is created through a usual workqueue event (because we can't create
> it from interrupt).
> 
> Changes in V2:
> 
> _ fix the "resync" mispelled in the patch and the changelog
> _ don't schedule more than one resync in case of concurrent interrupts
> _ if resync_pending is not cleared after a few time before disconnect, print a warning.
> 

This patch seems complicated.


> 
> diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c
> index f8f86de..fc4490e 100644
> --- a/drivers/input/mouse/psmouse-base.c
> +++ b/drivers/input/mouse/psmouse-base.c
> @@ -20,6 +20,7 @@
>  #include <linux/init.h>
>  #include <linux/libps2.h>
>  #include <linux/mutex.h>
> +#include <linux/kthread.h>
>  
>  #include "psmouse.h"
>  #include "synaptics.h"
> @@ -104,8 +105,6 @@ static struct attribute_group psmouse_attribute_group = {
>   */
>  static DEFINE_MUTEX(psmouse_mutex);
>  
> -static struct workqueue_struct *kpsmoused_wq;
> -
>  struct psmouse_protocol {
>  	enum psmouse_type type;
>  	const char *name;
> @@ -203,11 +202,6 @@ static psmouse_ret_t psmouse_process_byte(struct psmouse *psmouse)
>  	return PSMOUSE_FULL_PACKET;
>  }
>  
> -void psmouse_queue_work(struct psmouse *psmouse, struct delayed_work *work,
> -		unsigned long delay)
> -{
> -	queue_delayed_work(kpsmoused_wq, work, delay);
> -}
>  
>  /*
>   * __psmouse_set_state() sets new psmouse state and resets all flags.
> @@ -313,7 +307,10 @@ static irqreturn_t psmouse_interrupt(struct serio *serio,
>  		       psmouse->name, psmouse->phys, psmouse->pktcnt);
>  		psmouse->badbyte = psmouse->packet[0];
>  		__psmouse_set_state(psmouse, PSMOUSE_RESYNCING);
> -		psmouse_queue_work(psmouse, &psmouse->resync_work, 0);
> +		if (atomic_inc_return(&psmouse->resync_pending) == 1)
> +			schedule_work(&psmouse->resync_work);
> +		else
> +			atomic_dec(&psmouse->resync_pending);
>  		goto out;
>  	}

This little trick looks inherently racy.  Suppose
psmouse_resync_thread_helper() is concurrently fiddling with
->resync_pending.

Dunno - maybe it isn't racy.  I can't really be bothered working it
out, because it should be obviosuly non-racy!

Can we remove ->resync_pending?  Just unconditionally do the
schedule_work()?  schedule_work() will take care of things
appropriately, won't it?

There is, I guess, a small possibility that we'll end up with two (or
more!) kernel threads running at the same time.  But the psmouse code
should be able to handle that appropriately.

> @@ -350,7 +347,11 @@ static irqreturn_t psmouse_interrupt(struct serio *serio,
>  	    time_after(jiffies, psmouse->last + psmouse->resync_time * HZ)) {
>  		psmouse->badbyte = psmouse->packet[0];
>  		__psmouse_set_state(psmouse, PSMOUSE_RESYNCING);
> -		psmouse_queue_work(psmouse, &psmouse->resync_work, 0);
> +
> +		if (atomic_inc_return(&psmouse->resync_pending) == 1)
> +			schedule_work(&psmouse->resync_work);
> +		else
> +			atomic_dec(&psmouse->resync_pending);
>  		goto out;
>  	}
>  

etc.

>  
>  
>  /*
> - * psmouse_resync() attempts to re-validate current protocol.
> + * psmouse_resync_thread() attempts to re-validate current protocol.
> + * This thread is created on the fly when needed because its job can take too
> + * much time on events workqueues, and the resync is rare enough to avoid
> + * the need of a private workqueue.
>   */
>  
> -static void psmouse_resync(struct work_struct *work)
> +static int psmouse_resync_thread(void *v)
>  {
> -	struct psmouse *parent = NULL, *psmouse =
> -		container_of(work, struct psmouse, resync_work.work);
> +	struct psmouse *psmouse = v;
> +	struct psmouse *parent = NULL;
>  	struct serio *serio = psmouse->ps2dev.serio;
>  	psmouse_ret_t rc = PSMOUSE_GOOD_DATA;
>  	int failed = 0, enabled = 0;
> @@ -1072,6 +1076,30 @@ static void psmouse_resync(struct work_struct *work)
>  		psmouse_activate(parent);
>   out:
>  	mutex_unlock(&psmouse_mutex);
> +
> +	/*
> +	 * While disconnecting, the driver wants to be sure all resync are done
> +	 */
> +	atomic_dec(&psmouse->resync_pending);
> +	wake_up(&psmouse->resync_pending_queue);
> +	return 0;
> +}
> +
> +/* Launch the resync thread */
> +static void psmouse_resync_thread_helper(struct work_struct *work)
> +{
> +	struct task_struct *t;
> +	struct psmouse *psmouse;
> +
> +	psmouse = container_of(work, struct psmouse, resync_work);
> +
> +	t = kthread_run(psmouse_resync_thread, psmouse, "kpsmoused");
> +	if (t == ERR_PTR(-ENOMEM)) {
> +		printk(KERN_WARNING "psmouse.c: failed to create kpsmoused"
> +				    " thread\n");
> +		atomic_dec(&psmouse->resync_pending);
> +		wake_up(&psmouse->resync_pending_queue);
> +	}
>  }

I expect I asked before "can we use the kernel/async.c code for this".

It's such an obvious question that it should be covered in the
changelog.

Re: [PATCH v2] psmouse: run kpsmoused only while needed

[Frederic Weisbecker]

On Fri, Feb 13, 2009 at 04:05:26PM -0800, Andrew Morton wrote:
> On Sat, 31 Jan 2009 16:27:22 -0800 (PST)
> Frederic Weisbecker <fweisbec@gmail.com> wrote:
> 
> > While looking at the workqueue tracer, I noticed that kpsmoused receives
> > rarely (if not never) events.
> > 
> > Currently, when a mouse has to resync, it uses the kpsmoused singlethreaded
> > workqueue. But resync are rare. While reading an old discussion, it seems
> > that usual workqueue events can't be used for that purpose because resync
> > can take too much time and could delay the other works in queue.
> > 
> > But if you have built psmouse driver, this workqueue will always be present
> > whether you have a ps/2 port or not. And its events are rare.
> > 
> > To avoid this pointless task, this patch makes the kpsmoused a kernel
> > thread only created on the fly when a resync is needed. Once the resync is done,
> > this thread will die. So you will almost never see it, and it will not be
> > an inactive task anymore.
> > 
> > This thread is created through a usual workqueue event (because we can't create
> > it from interrupt).
> > 
> > Changes in V2:
> > 
> > _ fix the "resync" mispelled in the patch and the changelog
> > _ don't schedule more than one resync in case of concurrent interrupts
> > _ if resync_pending is not cleared after a few time before disconnect, print a warning.
> > 
> 
> This patch seems complicated.


Yeah, I know :-/

 
> 
> > 
> > diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c
> > index f8f86de..fc4490e 100644
> > --- a/drivers/input/mouse/psmouse-base.c
> > +++ b/drivers/input/mouse/psmouse-base.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/init.h>
> >  #include <linux/libps2.h>
> >  #include <linux/mutex.h>
> > +#include <linux/kthread.h>
> >  
> >  #include "psmouse.h"
> >  #include "synaptics.h"
> > @@ -104,8 +105,6 @@ static struct attribute_group psmouse_attribute_group = {
> >   */
> >  static DEFINE_MUTEX(psmouse_mutex);
> >  
> > -static struct workqueue_struct *kpsmoused_wq;
> > -
> >  struct psmouse_protocol {
> >  	enum psmouse_type type;
> >  	const char *name;
> > @@ -203,11 +202,6 @@ static psmouse_ret_t psmouse_process_byte(struct psmouse *psmouse)
> >  	return PSMOUSE_FULL_PACKET;
> >  }
> >  
> > -void psmouse_queue_work(struct psmouse *psmouse, struct delayed_work *work,
> > -		unsigned long delay)
> > -{
> > -	queue_delayed_work(kpsmoused_wq, work, delay);
> > -}
> >  
> >  /*
> >   * __psmouse_set_state() sets new psmouse state and resets all flags.
> > @@ -313,7 +307,10 @@ static irqreturn_t psmouse_interrupt(struct serio *serio,
> >  		       psmouse->name, psmouse->phys, psmouse->pktcnt);
> >  		psmouse->badbyte = psmouse->packet[0];
> >  		__psmouse_set_state(psmouse, PSMOUSE_RESYNCING);
> > -		psmouse_queue_work(psmouse, &psmouse->resync_work, 0);
> > +		if (atomic_inc_return(&psmouse->resync_pending) == 1)
> > +			schedule_work(&psmouse->resync_work);
> > +		else
> > +			atomic_dec(&psmouse->resync_pending);
> >  		goto out;
> >  	}
> 
> This little trick looks inherently racy.  Suppose
> psmouse_resync_thread_helper() is concurrently fiddling with
> ->resync_pending.
> 
> Dunno - maybe it isn't racy.  I can't really be bothered working it
> out, because it should be obviosuly non-racy!
> 
> Can we remove ->resync_pending?  Just unconditionally do the
> schedule_work()?  schedule_work() will take care of things
> appropriately, won't it?
> There is, I guess, a small possibility that we'll end up with two (or
> more!) kernel threads running at the same time.  But the psmouse code
> should be able to handle that appropriately.


Well, that's ok. But I still need to keep a reference counting of the threads
created, just to be sure all is finished and avoid freeing the mouse struct too early.

 
> > @@ -350,7 +347,11 @@ static irqreturn_t psmouse_interrupt(struct serio *serio,
> >  	    time_after(jiffies, psmouse->last + psmouse->resync_time * HZ)) {
> >  		psmouse->badbyte = psmouse->packet[0];
> >  		__psmouse_set_state(psmouse, PSMOUSE_RESYNCING);
> > -		psmouse_queue_work(psmouse, &psmouse->resync_work, 0);
> > +
> > +		if (atomic_inc_return(&psmouse->resync_pending) == 1)
> > +			schedule_work(&psmouse->resync_work);
> > +		else
> > +			atomic_dec(&psmouse->resync_pending);
> >  		goto out;
> >  	}
> >  
> 
> etc.
> 
> >  
> >  
> >  /*
> > - * psmouse_resync() attempts to re-validate current protocol.
> > + * psmouse_resync_thread() attempts to re-validate current protocol.
> > + * This thread is created on the fly when needed because its job can take too
> > + * much time on events workqueues, and the resync is rare enough to avoid
> > + * the need of a private workqueue.
> >   */
> >  
> > -static void psmouse_resync(struct work_struct *work)
> > +static int psmouse_resync_thread(void *v)
> >  {
> > -	struct psmouse *parent = NULL, *psmouse =
> > -		container_of(work, struct psmouse, resync_work.work);
> > +	struct psmouse *psmouse = v;
> > +	struct psmouse *parent = NULL;
> >  	struct serio *serio = psmouse->ps2dev.serio;
> >  	psmouse_ret_t rc = PSMOUSE_GOOD_DATA;
> >  	int failed = 0, enabled = 0;
> > @@ -1072,6 +1076,30 @@ static void psmouse_resync(struct work_struct *work)
> >  		psmouse_activate(parent);
> >   out:
> >  	mutex_unlock(&psmouse_mutex);
> > +
> > +	/*
> > +	 * While disconnecting, the driver wants to be sure all resync are done
> > +	 */
> > +	atomic_dec(&psmouse->resync_pending);
> > +	wake_up(&psmouse->resync_pending_queue);
> > +	return 0;
> > +}
> > +
> > +/* Launch the resync thread */
> > +static void psmouse_resync_thread_helper(struct work_struct *work)
> > +{
> > +	struct task_struct *t;
> > +	struct psmouse *psmouse;
> > +
> > +	psmouse = container_of(work, struct psmouse, resync_work);
> > +
> > +	t = kthread_run(psmouse_resync_thread, psmouse, "kpsmoused");
> > +	if (t == ERR_PTR(-ENOMEM)) {
> > +		printk(KERN_WARNING "psmouse.c: failed to create kpsmoused"
> > +				    " thread\n");
> > +		atomic_dec(&psmouse->resync_pending);
> > +		wake_up(&psmouse->resync_pending_queue);
> > +	}
> >  }
> 
> I expect I asked before "can we use the kernel/async.c code for this".
> It's such an obvious question that it should be covered in the
> changelog.


Yes, I answered you about that on the V1 discussion of this patch.
The problem with async is that we don't have any guarantee that the job
will be executed asynchronously. If we are running low on memory, or if there are too
much jobs pending, async can decide to execute synchronously the function.

That would have been handy: I would just need to call async_synchronize_cookie_special()
to ensure all is finished.


> 
> 
>