While testing a patch I have noticed a burst (three in a row) of "Setup poll for '/org/freedesktop/UPower/devices/battery_BATX' every XX seconds" debug messages. I had a quick look and it seems that the firing of the battery refresh (i.e. the battery polling) causes the 'change_idle_timeout' (up-daemon.c @ 729) being emitted three times in a row, which will completely remove and re-install polling on each invocation. The most obvious fix would be to not listen to the "notify::warning-level" while the poll callback is fired.
Created attachment 129831 [details] [review] daemon: only reset poll if warning-level changed When a device is refreshed because the poll timeout has been reached, the warning-level change notification can also be fired, which then will reset (i.e. disable, re-enable) polling. For batteries this can happen three times in a row. Now we reset polling only if the calculated timeout actually differs from the current one.
Created attachment 129832 [details] [review] daemon: move two functions up No semantic change.
Created attachment 129833 [details] [review] daemon: more efficient poll resetting If the poll timeout is actually changed because the warning level has changed, only reset the actual poll code, not the warning level notifications and the booking structures.
Comment on attachment 129832 [details] [review] daemon: move two functions up Review of attachment 129832 [details] [review]: ----------------------------------------------------------------- The patch needs rebasing (probably redoing) as well, as it doesn't apply cleanly anymore, and you wouldn't want to revert changes or introduce new ones. ::: src/up-daemon.c @@ -75,4 @@ > static gboolean up_daemon_get_warning_level_local(UpDaemon *daemon); > static gboolean up_daemon_get_on_ac_local (UpDaemon *daemon); > > -static guint calculate_timeout (UpDevice *device); Can you make this patch the first one in the series? This would avoid adding the forward declaration to remove it in the next patch.
Comment on attachment 129833 [details] [review] daemon: more efficient poll resetting Review of attachment 129833 [details] [review]: ----------------------------------------------------------------- ::: src/up-daemon.c @@ +776,4 @@ > g_debug ("Resetting for polling for '%s' (warning-level change)", > up_device_get_object_path (device)); > > + g_source_remove (data->id); This is pretty ugly, and it's really counter-intuitive to dig into the gory implementation details when we used to hide it so well. Maybe a up_daemon_update_poll_timeout pre/post with some explanation would be better? It needs a comment in the code in any case.
Comment on attachment 129831 [details] [review] daemon: only reset poll if warning-level changed Review of attachment 129831 [details] [review]: ----------------------------------------------------------------- Some test cases would also be good :) ::: src/up-daemon.c @@ +744,5 @@ > > + g_debug ("change_idle_timeout called for: %s", > + up_device_get_object_path (device)); > + > + timeout = calculate_timeout (device); If the warning-level can change in response to anything but a poll (a uevent?), then there's a problem. stop/start the polling would also reset the timeout clock, not just change the rate of the timeout.
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/upower/upower/issues/40.
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.