Adam Jackson <[email protected]> wrote:
[...]
> +/**
> + * Unregister a device in the threaded input facility
> + *
> + * @param fd File descriptor which identifies the input device
> + *
> + * @return 1 if success; 0 otherwise.
> + */
> +int
> +InputThreadUnregisterDev(int fd)
> +{
> + InputThreadDevice *prev, *dev;
> +
> +
> + /* return silently if input thread is already finished (e.g., at
> + * DisableDevice time, evdev tries to call this function again through
> + * xf86RemoveEnabledDevice */
> + if (!inputThreadInfo)
> + return 0;
> +
> + prev = NULL;
> + dev = inputThreadInfo->devs;
> + while (dev != NULL) {
> + if (dev->fd == fd)
> + break;
> + prev = dev;
> + dev = dev->next;
> + }
> +
> + /* fd didn't match any registered device. */
> + if (dev == NULL)
> + return 0;
> +
> + if (prev == NULL)
> + inputThreadInfo->devs = dev->next;
> + else
> + prev->next = dev->next;
> +
> + FD_CLR(fd, &inputThreadInfo->fds);
> + dev->readInputProc = NULL;
> + dev->readInputArgs = NULL;
> + dev->fd = 0;
> + dev = 0;
> + free(dev);
There's a memory leak caused by free(0) right above.
I wish I could send a patch to fix it, instead of just pointing it out, but I
lost my development machine a few weeks (months?) ago and I couldn't afford a
replacement yet.
> +
> + InputThreadFillPipe(hotplugPipeWrite);
> + DebugF("input-thread: unregistered device: %d\n", fd);
> +
> + return 1;
> +}
> +
> +/**
> + * The workhorse of threaded input event generation.
> + *
> + * Or if you prefer: The WaitForSomething for input devices. :)
> + *
> + * Runs in parallel with the server main thread, listening to input devices
> in
> + * an endless loop. Whenever new input data is made available, calls the
> + * proper device driver's routines which are ultimately responsible for the
> + * generation of input events.
> + *
> + * @see InputThreadPreInit()
> + * @see InputThreadInit()
> + */
> +
> +static void*
> +InputThreadDoWork(void *arg)
> +{
> + fd_set readyFds;
> + InputThreadDevice *dev;
> +
> + FD_ZERO(&readyFds);
> +
> + while (1)
> + {
> + XFD_COPYSET(&inputThreadInfo->fds, &readyFds);
> + FD_SET(hotplugPipeRead, &readyFds);
> +
> + DebugF("input-thread: InputThreadDoWork waiting for devices\n");
> +
> + if (Select(MaxInputDevices, &readyFds, NULL, NULL, NULL) < 0) {
As you pointed before, the above occurrence of MaxInputDevices abuses the
semantics of select() which, according to the OpenGroup docs, expects as its
first argument the highest-numbered file descriptor of any of the three fd sets
it blocks on. I have a patch to fix it and, although I doubt it still applies to
master, you might find a warmer place for it then annarchy's disks. Yeah, that's
a quibble! :)
http://people.freedesktop.org/~fcarrijo/patches/input-thread-after-review-take-2/0003-inputthread-Use-our-own-count-for-the-highest-fd-amo.patch
> + if (errno == EINVAL)
> + FatalError("input-thread: InputThreadDoWork (%s)",
> strerror(errno));
> + else if (errno != EINTR)
> + ErrorF("input-thread: InputThreadDoWork (%s)\n",
> strerror(errno));
> + }
> +
> + DebugF("input-thread: InputThreadDoWork generating events\n");
> + /* Call the device drivers to generate input events for us */
> + for (dev = inputThreadInfo->devs; dev != NULL; dev = dev->next)
> + if (FD_ISSET(dev->fd, &readyFds) && dev->readInputProc)
> + dev->readInputProc(dev->readInputArgs);
> +
> + /* Kick main thread to process the generated input events and drain
> + * events from hotplug pipe */
> + InputThreadFillPipe(inputThreadInfo->writePipe);
> + InputThreadReadPipe(hotplugPipeRead);
> + }
> +}
> +
> +static void
> +InputThreadWakeup(pointer blockData, int err, pointer pReadmask)
> +{
> + InputThreadReadPipe(inputThreadInfo->readPipe);
> +}
The decision of breaking the initialization code into PreInit() and Init() was
driven by the fact that the input devices are registered with the input thread
only after the communication pipes are created. If we pthread_create'd at that
point, the input thread would block indefinitely on select(), waiting for yet
non-registered devices.
>From both design and performance points of view, I'm not sure how better it
would've been if we, instead of breaking the initialization code, had only one
setup routine, within which we pthread_create'd and pthread_cond_wait'ed based
on a conditional variable that signaled the existence of at least one input
device attached to the system.
Some people might claim that checking the conditional variable could decrease
the performance of this fast-path, but I'm yet to see numbers validating this
concern. As a (probably unnecessary) reminding, the glibc implementation of
thread synchronization primitives in Linux is based on futexes, which have the
nice characteristic of degenerating to userland calls, thus avoiding the high
cost of context-switches in all but the contended cases.
> +
> +/**
> + * Pre-initialize the facility used for threaded generation of input events
> + *
> + */
> +void
> +InputThreadPreInit(void)
> +{
> + int fds[2], hotplugPipe[2];
> +
> + if (pipe(fds) < 0)
> + FatalError("input-thread: could not create pipe");
> +
> + if (pipe(hotplugPipe) < 0)
> + FatalError("input-thread: could not create pipe");
> +
> + inputThreadInfo = malloc(sizeof(InputThreadInfo));
> + if (!inputThreadInfo)
> + FatalError("input-thread: could not allocate memory");
> +
> + inputThreadInfo->thread = 0;
> + inputThreadInfo->devs = NULL;
> + FD_ZERO(&inputThreadInfo->fds);
> +
> + /* 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.
> + */
> + inputThreadInfo->readPipe = fds[0];
> + fcntl(inputThreadInfo->readPipe, F_SETFL, O_NONBLOCK | O_CLOEXEC);
> + AddGeneralSocket(inputThreadInfo->readPipe);
> + RegisterBlockAndWakeupHandlers((BlockHandlerProcPtr)NoopDDA,
> + InputThreadWakeup, NULL);
> +
> + inputThreadInfo->writePipe = fds[1];
> +
> + hotplugPipeRead = hotplugPipe[0];
> + fcntl(hotplugPipeRead, F_SETFL, O_NONBLOCK | O_CLOEXEC);
> + hotplugPipeWrite = hotplugPipe[1];
> +}
> +
> +/**
> + * Start the threaded generation of input events. This routine complements
> what
> + * was previously done by InputThreadPreInit(), being only responsible for
> + * creating the dedicated input thread.
> + *
> + */
> +void
> +InputThreadInit(void)
> +{
> + pthread_attr_t attr;
> +
> + pthread_attr_init(&attr);
> +
> + /* For OSes that differentiate between processes and threads, the
> following
> + * lines have sense. Linux uses the 1:1 thread model. The scheduler
> handles
> + * every thread as a normal process. Therefore this probably has no
> meaning
> + * if we are under Linux.
> + */
> + if (pthread_attr_setscope(&attr, PTHREAD_SCOPE_SYSTEM) != 0)
> + ErrorF("input-thread: error setting thread scope\n");
> +
> + if (pthread_attr_setstacksize(&attr, PTHREAD_STACK_MIN) != 0)
> + ErrorF("input-thread: error setting thread stack size\n");
> +
> + DebugF("input-thread: creating thread\n");
> + pthread_create(&inputThreadInfo->thread, &attr,
> + &InputThreadDoWork, NULL);
> +
> + pthread_attr_destroy (&attr);
> +}
> +
Thanks for spending your energy in this.
_______________________________________________
[email protected]: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel
