Re: [PATCH v2] psmouse: run kpsmoused only while needed
From: Frédéric Weisbecker
Date: Thu Feb 12 2009 - 20:03:28 EST
2009/2/1 Frederic Weisbecker <fweisbec@xxxxxxxxx>:
> 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@xxxxxxxxx>
> ---
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/