Date:	Fri, 24 Feb 2012 20:25:30 -0800
From:	Arve Hjønnevåg <arve@...roid.com>
To:	Matt Helsley <matthltc@...ibm.com>
Cc:	"Rafael J. Wysocki" <rjw@...k.pl>,
	Linux PM list <linux-pm@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	Magnus Damm <magnus.damm@...il.com>, markgross@...gnar.org,
	Matthew Garrett <mjg@...hat.com>,
	Greg KH <gregkh@...uxfoundation.org>,
	John Stultz <john.stultz@...aro.org>,
	Brian Swetland <swetland@...gle.com>,
	Neil Brown <neilb@...e.de>,
	Alan Stern <stern@...land.harvard.edu>,
	Dmitry Torokhov <dmitry.torokhov@...il.com>
Subject: Re: [RFC][PATCH 4/7] Input / PM: Add ioctl to block suspend while
 event queue is not empty

On Thu, Feb 23, 2012 at 9:16 PM, Matt Helsley <matthltc@...ibm.com> wrote:
> On Wed, Feb 22, 2012 at 12:34:58AM +0100, Rafael J. Wysocki wrote:
>> From: Arve Hjønnevåg <arve@...roid.com>
>>
>> Add a new ioctl, EVIOCSWAKEUPSRC, to attach a wakeup source object to
>> an evdev client event queue, such that it will be active whenever the
>> queue is not empty.  Then, all events in the queue will be regarded
>> as wakeup events in progress and pm_get_wakeup_count() will block (or
>> return false if woken up by a signal) until they are removed from the
>> queue.  In consequence, if the checking of wakeup events is enabled
>> (e.g. throught the /sys/power/wakeup_count interface), the system
>> won't be able to go into a sleep state until the queue is empty.
>>
>> This allows user space processes to handle situations in which they
>> want to do a select() on an evdev descriptor, so they go to sleep
>> until there are some events to read from the device's queue, and then
>> they don't want the system to go into a sleep state until all the
>> events are read (presumably for further processing).  Of course, if
>> they don't want the system to go into a sleep state _after_ all the
>> events have been read from the queue, they have to use a separate
>> mechanism that will prevent the system from doing that and it has
>> to be activated before reading the first event (that also may be the
>> last one).
>
> I haven't seen this idea mentioned before but I must admit I haven't
> been following this thread too closely so apologies (and don't bother
> rehashing) if it has:
>
> Could you just add this to epoll so that any fd userspace chooses would be
> capable of doing this without introducing potentially ecclectic ioctl
> interfaces?
>

This is an interesting idea, but I'm not sure how well it would work.

I looked at the epoll code and it looks like it is possible to
activate the wakeup-source from the wait queue function it uses. The
epoll callback will happen without holding evdev client buffer_lock,
so the wakeup-source and buffer state will not always be in sync (this
may be OK, but require more thought). This callback is also called if
no data was added to the queue we are polling on because another
client has grabbed the input device (is this a bug or intended?).

There is no call into the epoll code when input queue is emptied, so
we can't deactivate the wakeup-source until epoll_wait is called
again. This also should be workable, but result in different stats.

It does not look like the normal poll and select interfaces can be
extended the same way (since they remove themselves from the
wait-queue before returning to user-space), so user-space has to be
changed to use epoll even if select or poll would be a better fit.

I don't know how many other drivers this would work for. The input
driver will wake up user-space from the same thread or interrupt
handler that queued the event, but other drivers may defer this to
another thread which makes an epoll wakeup-source insufficient.

...
>> +     snprintf(name, sizeof(name), "%s-%d",
>> +              dev_name(&evdev->dev), task_tgid_vnr(current));
>
> This does not look like it will work well with tasks in different pid
> namespaces. What should happen, I think, is the wakeup_source should hold a
> reference to either the struct pid of current or current itself. Then
> when someone reads the file you should get the pid vnr in the reader's
> pid namespace. That way instead of a bogus pid vnr 0 would show up if
> "current" here is not in the reader's pid namepsace.
>

The pid here is only used for debugging purposes, and used less than
the dev_name. I don't think tracking pid namespaces is worth the
trouble here, so if this is a real problem we can just drop the pid
from the name for now.

-- 
Arve Hjønnevåg
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists