Wakeup interface concerns (Was Re: changing the enum type)

[Richard Hughes]

On Mon, 2009-03-02 at 11:35 -0500, David Zeuthen wrote: 
> The main concern is that the presence of the signals leads you to
> believe you can just connect to connect to the bus and then they get
> delivered to you with some sane frequency. 

Well, you can. The kernel only updates this data every n seconds, where
n is an floating point number of seconds defined internally (in the
kernel) which can change. We don't do any clever processing of events /
timeout period, that's all done in the kernel.

> Of course this won't work because then the daemon would have to wake up
> all the time. Which coincidentally it used to do until I pointed that
> out.

We only wake up every few seconds to see if the kernel has written out
new data which is for the n seconds earlier. We certainly don't wake up
for every event!

> There's also a (very slight) concern the data carried in the signals may
> be privileged information; you may not be authorized to get this
> information depending on how your system is set up.

Sure, we can put this behind a PolicyKit action if we need to.

> The other observation to make, once you realize that signals won't work,
> is that having multiple different callers will cause measurement issues.
> They literally step on each others toes with the way this work.

No, sorry. This is exactly why DK-p is reading the kernel data, it's
difficult to get right and has to be done as root. Just from reading out
a table of data in /proc doesn't invalidate it, so there's no
stepping-on-toes problem. The kernel does the averaging over a time
period, not us.

> Another observation is that the wakeup interface is mostly just a power
> user feature, it's really only something you need if you want a GUI-ish
> powertop utility like you did in g-p-m.

Right. The QoS is probably also a power user thing, but I want to tie
that into other frameworks so it starts being used by default.

>  o  get rid of the signals
>  o  make GetData() also return
>      - the data in the signals
>      - amount of msec since this was last read
>     in addition to the array of structs per process

There's no point returning the time since last read, as the kernel data
won't change until the next kernel-space refresh.

>  o add methods to claim/release the wake up interface
>     - StartReadingData (OUT string cookie)
>     - StopReadingData (IN string cookie)
>    and only allow a single caller to do this. Remember to
>    watch for disconnects too.

I don't think it's a problem if multiple callers call this (given what I
said above), and at the moment we're just stopping the polling if
nothing reads the data for a 30 seconds.

In the end of the day, the API is tailored to what the kernel can give
us. A disk access interface might look more like what you suggested, but
that's because it's different data from the kernel. I guess the best
thing would be to do the averaging in the kernel as what we have now
with the vm dump isn't the most palatable sort of data.

Richard.