[PATCH] xfree86: use a thread for the generation of input events

[Adam Jackson]

On Mon, 2010-08-23 at 11:17 +0300, Tiago Vignatti wrote:

Thanks for posting this, I'm eager to see this land for 1.10.

> @@ -260,6 +266,9 @@ int main(int argc, char *argv[], char *envp[])
>  	    InitRootWindow(screenInfo.screens[i]->root);
>  
>          InitCoreDevices();
> +
> +	threaded_input_pre_init();
> +
>  	InitInput(argc, argv);
>  	InitAndStartDevices();
>  

Style nit, would be nice to see that in InitInput() instead.  main()'s
already unreadable.

> @@ -311,6 +322,8 @@ int main(int argc, char *argv[], char *envp[])
>  
>          CloseInput();
>  
> +	threaded_input_fini();
> +
>  	for (i = 0; i < screenInfo.numScreens; i++)
>  	    screenInfo.screens[i]->root = NullWindow;
>  	CloseDownDevices();

Ditto.

> @@ -307,9 +321,14 @@ xf86SigioReadInput(int fd, void *closure)
>  void
>  xf86AddEnabledDevice(InputInfoPtr pInfo)
>  {
> +#ifdef INPUT_THREAD
> +    threaded_input_register_device(pInfo->fd, xf86ThreadReadInput, pInfo);
> +#else
> +
>      if (!xf86InstallSIGIOHandler (pInfo->fd, xf86SigioReadInput, pInfo)) {
>  	AddEnabledDevice(pInfo->fd);
>      }
> +#endif
>  }

I feel like this should follow the same pattern:

    if (!threaded_input_register_device(pInfo->fd, xf86ThreadReadInput, pInfo) &&
        !xf86InstallSIGIOHandler(pInfo->fd, xf86SigioReadInput, pInfo))
        AddEnabledDevice(pInfo->fd);

And then you'd build the server such that either or both of those two
functions is #define'd to 0.  Which we kind of already do for SIGIO
except it's a stub function instead of a #define.  If you do that then
you can make the malloc failure in t_i_r_d not a FatalError, although
you'd need to make any input from main thread take the mutex.

xf86ThreadReadInput is redundant here, I think you can just pass in
pInfo->read_input directly.  Unless you expect the input driver to
modify that function pointer at runtime, but forbidding that seems like
a perfectly reasonable invariant.

> @@ -319,9 +338,13 @@ xf86AddEnabledDevice(InputInfoPtr pInfo)
>  void
>  xf86RemoveEnabledDevice(InputInfoPtr pInfo)
>  {
> +#ifdef INPUT_THREAD
> +    threaded_input_unregister_device(pInfo->fd);
> +#else
>      if (!xf86RemoveSIGIOHandler (pInfo->fd)) {
>  	RemoveEnabledDevice(pInfo->fd);
>      }
> +#endif
>  }

Ditto.
 
> diff --git a/include/os.h b/include/os.h
> index efa202c..0b0113e 100644
> --- a/include/os.h
> +++ b/include/os.h
> @@ -165,6 +165,24 @@ extern _X_EXPORT void MakeClientGrabPervious(ClientPtr /*client*/);
>  extern void ListenOnOpenFD(int /* fd */, int /* noxauth */);
>  #endif
>  
> +#ifdef INPUT_THREAD
> +typedef void (*read_input)(void*);

read_input_proc please?  read_input is likely to be a function name,
it's not going to confuse the compiler but it may confuse the reader.
 
> @@ -226,6 +230,7 @@ WaitForSomething(int *pClientsReady)
>  	}
>  	else 
>  	{
> +	    threaded_input_drain_pipe();
>  	    i = Select (MaxClients, &LastSelectMask, NULL, NULL, wt);
>  	}
>  	selecterr = GetErrno();

Don't do this.  Use a WakeupHandler to pick up events on the input event
pipe.  Otherwise you're slowing down every run through the dispatch loop
by a syscall.

(Actually, you should lift the xf86AddGeneralHandler stuff to the dix
level, and register the input pipe with that, so we can pull all the
knowledge of fd_sets out of Wakeup/BlockHandler, so we can switch the
main loop to poll too.)

And then once you do, fix thread_input_drain_pipe() to read in chunks
until EAGAIN or short read instead of just pulling 10 events at a time.
Not that it's super likely, but you might as well.

> diff --git a/os/connection.c b/os/connection.c
> index 77910be..81340b6 100644
> --- a/os/connection.c
> +++ b/os/connection.c
> @@ -145,6 +145,10 @@ int MaxClients = 0;
>  Bool NewOutputPending;		/* not yet attempted to write some new output */
>  Bool AnyClientsWriteBlocked;	/* true if some client blocked on write */
>  
> +#ifdef INPUT_THREAD
> +int MaxInputDevices = 0;
> +#endif
> +
>  static Bool RunFromSmartParent;	/* send SIGUSR1 to parent process */
>  Bool PartialNetwork;	/* continue even if unable to bind all addrs */
>  static Pid_t ParentProcess;
> @@ -302,6 +306,10 @@ InitConnectionLimits(void)
>      if (lastfdesc > MAXSELECT)
>  	lastfdesc = MAXSELECT;
>  
> +#ifdef INPUT_THREAD
> +    MaxInputDevices = lastfdesc;
> +#endif
> +
>      if (lastfdesc > MAXCLIENTS)
>      {
>  	lastfdesc = MAXCLIENTS;

Ew.  You're only doing this as an artifact of using select().  We still
haven't switched the main loop to poll() because a) ABI and b) win32,
but there's no reason to be that broken here.

> +    /* By making read head non-blocking, we ensure that while the main thread
> +     * is busy servicing client requests, the dedicated input thread can work
> +     * in parallel.
> +     */
> +    threaded_input->read_pipe = fds[0];
> +    fcntl(threaded_input->read_pipe, F_SETFL, O_NONBLOCK);
> +    AddGeneralSocket(threaded_input->read_pipe);
> +    threaded_input->write_pipe = fds[1];
> +
> +    hotplugPipeRead = hotplugPipe[0];
> +    fcntl(hotplugPipeRead, F_SETFL, O_NONBLOCK);
> +    hotplugPipeWrite = hotplugPipe[1];

O_NONBLOCK | O_CLOEXEC.

- ajax
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part
URL: <http://lists.x.org/archives/xorg-devel/attachments/20100823/1f39f8cc/attachment.pgp>