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

Tue Aug 24 12:26:42 PDT 2010

On Mon, Aug 23, 2010 at 08:04:26PM +0200, ext Adam Jackson wrote:
> 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.

Agreed. 

But we'll have to think what to do with threaded_input_init(), because seems
there's no DDX init hook to stash it into. I see xquartz doing some
initialization pretty close from t_i_i, so we can probably set a new hook for
this kind of things.

> > @@ -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.

Yep, makes sense. However Peter mentioned a good point: that drivers will want
to know if the server is assuming thread or SIGIO, mostly to do malloc tricks
I guess. So we may want to just drop off the SIGIO code for once and enable
always the threaded coded. 

I'm wondering how this sounds for other platforms (I'm coding in Linux) and
other DDX. Alan, Mark Kettenis, Jon Turney and others?

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

true. I got distract by the useless 'int fd' in xf86SigioReadInput. I will
fix.

> > @@ -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.

ok.

> > @@ -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.

agreed. Got cook it.

> (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.

interesting.

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

let's fix this later on.

> > +    /* 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.

nice catch!

Thanks for your comments ajax!

             Tiago