Re: [PATCH v2] psmouse: run kpsmoused only while needed
From: Andrew Morton <akpm@linux-foundation.org>
Date: 2009-02-14 00:06:21
Also in:
lkml
On Sat, 31 Jan 2009 16:27:22 -0800 (PST) Frederic Weisbecker [off-list ref] 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.
quoted hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
@@ -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.
quoted hunk ↗ jump to hunk
/* - * 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.