xfce-mcs-manager -> suggest signal poll interval == 1s

[Kok, Auke]

Brian J. Tarricone wrote:
> {Sorry about a possible dupe; I sent the original from the wrong email
> address.}
> 
> On Thu, 24 May 2007 21:01:08 +0200 Jasper Huijsmans wrote:
> 
>> Brian J. Tarricone wrote:
>>> On Wed, 23 May 2007 23:47:53 -0700 Brian J. Tarricone wrote:
>>>> Actually, a better fix -- that would eliminate the polling
>>>> completely -- would be to have the signal handler write the status
>>>> code to a pipe, and then have the glib mainloop watch the pipe.
>>>> Pretty sure writing to a pipe is one of the few things 'allowed' in
>>>> a signal handler...
>>> And here's a patch.  It compiles; that's all I know.  Presumably it
>>> should work too.  If someone wants to test it, I'll commit it if it
>>> works (or someone else can).
>>>
>> Looks a lot like what I did for the panel, but there are a few 
>> differences, and I have no idea if they are important, because, 
>> naturally, I just copied the code from some example ;-)
>>
>>
>> From mcs-manager to panel:
>>
>> - Check return value for write.
> 
> I thought about that, but what do we do if write() fails?  I think we'd
> just end up ignoring the signal.
> 
>> - Don't check condition in io watch callback.
> 
> Shouldn't matter.  I only specified that I want G_IO_IN events, so
> those are the only ones that should arrive.  Just did the comparison as
> a sort of sanity check.
> 
>> - g_..._read_chars instead of g_..._read, with use of a union to get 
>> chars to int value. Looks overly complicated to me, actually... Ah, a 
>> comment says there is no ..._read, so this must be old.
> 
> Yeah, using just plain _read() with a 4-byte int should be equivalent.
> 
>> - read pipe in a while loop, check at the end if status matches 
>> G_IO_STATUS AGAIN.
> 
> I'm not convinced this is necessary, since I can't see why all 4 bytes
> of the data wouldn't be available at that point, but I suppose it
> couldn't hurt.  This isn't really a critical piece of code, and if it
> fails, it's not really a big deal.
> 
>> - Quit if pipe can't be created.
> 
> In this case I think that's bad: if the pipe can't be created and we
> quit, mcs-manager isn't running, which sucks.  If we don't quit, then
> the only problem (assuming pipe() failing isn't a harbinger of larger
> problems) is that mcs-manager won't respond to signals.  Not a big
> deal, IMHO.
> 
>> - Set reading and writing end of the pipe specifically to non-blocking
>> mode.
> 
> This is probably a good idea for the writing end -- if we receive a
> bunch of signals and the pipe fills before the main loop runs our io
> watch function, the signal handler could block forever on write() -- but
> shouldn't be necessary for the reading end (actually, not setting
> nonblock for the reading end should make the G_IO_ERROR_AGAIN stuff
> above unnecessary... I think).  I can't see how the pipe would 'lose'
> data before the io watch function is run.
> 
>> - Use G_IO_IN | G_IO_PRI instead of just G_IO_IN.
> 
> Dunno; can't hurt to add it.  Should modify or remove condition check
> in the io watch function.
> 
>> - Don't unref channel.
> 
> g_io_add_watch() takes a reference on the channel, making the refcount
> 2.  I unref the channel, bringing it to 1.  Down after gtk_main(), when
> I call g_source_remove() to ditch the io watch, it decrements refcount
> to 0 and the io channel is freed.
> 
> Thanks for the comments -- I wish I'd read your email about already
> having that in the panel before I did my own implementation.
> 
> And ok, since I don't want to start working on something before I leave
> for lunch in a few minutes, I worked up a new patch.  Again, only
> compile-tested.
> 
> (I need to copy this to xfdesktop too... it's still doing unsafe stuff
> in the signal handler.)

ok, the new patch works fine, both versions actually do. So feel free to merge 
this :)

BTW here's how my box currently looks to powertop when I disable all fluff and 
have a bare xfce4 running (stock 4.4.1):

---
     PowerTOP version 1.3       (C) 2007 Intel Corporation

Cn          Avg residency (10s) Long term residency avg
C0 (cpu running)        ( 1.1%)
C1                0.0ms ( 0.0%)                   0.0ms
C2                1.5ms ( 1.3%)                   2.6ms
C3               23.5ms (97.6%)                  10.8ms

Wakeups-from-idle per second :  50.3

Top causes for wakeups:
   62.1% ( 9.0)       <interrupt> : ide1
    6.9% ( 1.0)       xfce4-panel : schedule_timeout (process_timeout)
    6.9% ( 1.0)   xfce4-mixer-plu : schedule_timeout (process_timeout)
    4.1% ( 0.6)       <interrupt> : libata
    3.4% ( 0.5)   hald-addon-stor : do_nanosleep (hrtimer_wakeup)
    3.4% ( 0.5)     <kernel core> : queue_delayed_work_on (delayed_work_timer_
    2.8% ( 0.4)         xfdesktop : schedule_timeout (process_timeout)
    1.4% ( 0.2)     <kernel core> : page_writeback_init (wb_timer_fn)
    1.4% ( 0.2)            Thunar : schedule_timeout (process_timeout)
    1.4% ( 0.2)             orage : schedule_timeout (process_timeout)
---

as you can see I still have some hdd activity in the bg, and obviously 
xfce4-panel, xfce4-mixer-plugin are still polling once a second. I am surprised 
to also see Thunar popup at 1 poll every 5 second, and we already knew about orage.

This means that if we fix these issues, theoretically my laptop would run at ~30 
wps or so. There's still work to do there, but it's starting to add up now 
significantly. As you can see my laptop is already spending over 23ms in C3 
about 50 times per second, and that means for reasonable laptops we are already 
pushing against 4 to 5 hours of battery life easily :)

keep up these patches, it's good stuff :)

Auke