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

[Frederic Weisbecker]

On Mon, Feb 02, 2009 at 11:03:35PM -0800, Andrew Morton wrote:
> On Thu, 29 Jan 2009 00:08:39 +0100 Frederic Weisbecker <fweisbec@xxxxxxxxx> wrote:
> 
> > > > @@ -1131,7 +1155,13 @@ 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->recync_pending_queue, &wait,
> > > > +				TASK_UNINTERRUPTIBLE);
> > > > +	if (atomic_read(&psmouse->nb_recync_pending))
> > > > +		schedule();
> > > > +	finish_wait(&psmouse->recync_pending_queue, &wait);
> > > 
> > > So... we're requiring that nb_recync_pending is zero at this stage?
> > > 
> > > I wonder if the code manages to do that.  A little WARN_ON(), maybe?
> > > 
> > > >  	mutex_lock(&psmouse_mutex);
> > > >  
> > > 
> > 
> > After reading how work the async jobs (kernel/async.c), I think it would be better
> > to actually use it instead of creating a thread through a workqueue and wait for a
> > counter to be zero to be sure all is flushed.
> > 
> > The async functions provide local execution and synchronisation domains through special cookies,
> > which means long tasks of mouse resync will not starve other works.
> > 
> > What do you think about it?
> 
> Yes, it would be better to use the async infrastructure.
> 
> If only to see how the code ends up looking - it _should_ be simpler/cleaner
> than the open-coded implementation.  If it isn't, we should ask the async code
> "why not?".


Right, it would have make it cleaner, by managing itself the thread creation and
synchronize_cookie bits.
I discussed it with Arjan because the main requirement for kpsmoused to use it
was to ensure that async will never run the function in the current context which
is hardirq here. Mouse resyncing is a slow work and it needs to be done in user
context.

If async failed to allocate the given function as a queued node or if we are
overflowing the queue of jobs, async runs the passed function synchronously.

But we need other callsites which would require it before adapting async to manage
that. Only implementing that for kpsmoused would be pointless.

So I recently submitted a v2 (cc'd you) of this patch which addressed your review but I kept
the on-the-fly thread approach.

--
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/