device: always expose device statistics informationth/device-statistics

An unpreviledged user cannot enable the device statistics.
That limits the use of the device-statistics counter, also
as this is not private information and can be obtained otherwise
via `ip -stat -stat link show`.

Now, always internally enable a minimum RefreshRateMs  30 seconds.
Yes, there is a certain overhad associated with it, as we now
refresh every link at least every 60 seconds.

Also note, that now the new counters are read during
device_link_changed(). That means, they update whenever a netlink
message indicates a change on the link, which in turn causes a D-Bus
signal with the new values.
This means, that we possibly get updates much more often then RefreshRateMs
suggests. After all, it's RefreshRateMs, not RateLimitMs. That is, it
configures a minimal refresh-rate of events, not a maximum limit on the
events.
I think that is not a real performance issue, as we don't expect to get
flodded with netlink messages. If that happens, we can add proper
rate-limiting later.

Also, how we refresh more often then promised by RefreshRateMs. But the API
technically doesn't specify that, so if we find there is a problem with
this, we may revert it later.

2 files changed, 30 insertions, 35 deletions

diff --git a/introspection/nm-device-statistics.xml b/introspection/nm-device-statistics.xml
index 5d23bf3062..bdb19c89ce 100644
--- a/introspection/nm-device-statistics.xml
+++ b/introspection/nm-device-statistics.xml
@@ -5,10 +5,10 @@
     <!--
         RefreshRateMs:
 
-        Rate of change of the rest of properties of this interface. If zero, the
-        properties do not change. Otherwise, the properties are refreshed each
-        RefreshRateMs milliseconds in case the underlying counter has changed
-        too.
+        Refresh rate of the rest of properties of this interface. The properties
+        are guaranteed to be refreshed each RefreshRateMs milliseconds in case
+        the underlying counter has changed too.
+        If zero, there is no guaranteed refresh rate of the properties.
     -->
     <property name="RefreshRateMs" type="u" access="readwrite"/>
 
diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c
index ff4ced9d49..3337d2d5a9 100644
--- a/src/devices/nm-device.c
+++ b/src/devices/nm-device.c
@@ -803,44 +803,38 @@ _stats_update_counters (NMDevice *self,
 }
 
 static void
-_stats_refresh (NMDevice *self)
+_stats_update_counters_from_pllink (NMDevice *self, const NMPlatformLink *pllink)
 {
-	const NMPlatformLink *pllink;
-	int ifindex;
-
-	ifindex = nm_device_get_ip_ifindex (self);
-	if (ifindex > 0) {
-		nm_platform_link_refresh (NM_PLATFORM_GET, ifindex);
-		pllink = nm_platform_link_get (NM_PLATFORM_GET, ifindex);
-		if (pllink) {
-			_stats_update_counters (self, pllink->tx_bytes, pllink->rx_bytes);
-			return;
-		}
-	}
-	_stats_update_counters (self, 0, 0);
+	_stats_update_counters (self, pllink->tx_bytes, pllink->rx_bytes);
 }
 
 static gboolean
 _stats_timeout_cb (gpointer user_data)
 {
 	NMDevice *self = user_data;
+	int ifindex;
 
 	_LOGT (LOGD_DEVICE, "stats: refresh");
-	_stats_refresh (self);
+
+	ifindex = nm_device_get_ip_ifindex (self);
+	if (ifindex > 0)
+		nm_platform_link_refresh (NM_PLATFORM_GET, ifindex);
+
 	return G_SOURCE_CONTINUE;
 }
 
 static guint
 _stats_refresh_rate_real (guint refresh_rate_ms)
 {
+	const guint STATS_REFRESH_RATE_MS_MAX = 30000;
 	const guint STATS_REFRESH_RATE_MS_MIN = 200;
 
-	if (refresh_rate_ms == 0)
-		return 0;
+	if (refresh_rate_ms == 0 || refresh_rate_ms > STATS_REFRESH_RATE_MS_MAX)
+		return STATS_REFRESH_RATE_MS_MAX;
 
 	if (refresh_rate_ms < STATS_REFRESH_RATE_MS_MIN) {
 		/* you cannot set the refresh-rate arbitrarly small. E.g.
-		 * setting to 1ms is just killing. */
+		 * setting to 1ms is just killing. Have a lowest number. */
 		return STATS_REFRESH_RATE_MS_MIN;
 	}
 
@@ -851,6 +845,7 @@ static void
 _stats_set_refresh_rate (NMDevice *self, guint refresh_rate_ms)
 {
 	NMDevicePrivate *priv;
+	int ifindex;
 	guint old_rate;
 
 	priv = NM_DEVICE_GET_PRIVATE (self);
@@ -871,16 +866,14 @@ _stats_set_refresh_rate (NMDevice *self, guint refresh_rate_ms)
 	if (_stats_refresh_rate_real (old_rate) == refresh_rate_ms)
 		return;
 
-	if (!refresh_rate_ms) {
-		nm_clear_g_source (&priv->stats.timeout_id);
-		_stats_update_counters (self, 0, 0);
-		return;
-	}
-
 	nm_clear_g_source (&priv->stats.timeout_id);
 
-	/* trigger an inital refresh of the data when the refresh-rate changes */
-	_stats_refresh (self);
+	/* trigger an inital refresh of the data whenever the refresh-rate changes.
+	 * As we process the result in an idle handler with device_link_changed(),
+	 * we don't get the result right away. */
+	ifindex = nm_device_get_ip_ifindex (self);
+	if (ifindex > 0)
+		nm_platform_link_refresh (NM_PLATFORM_GET, ifindex);
 
 	priv->stats.timeout_id = g_timeout_add (refresh_rate_ms, _stats_timeout_cb, self);
 }
@@ -1830,6 +1823,9 @@ device_link_changed (NMDevice *self)
 		_notify (self, PROP_DRIVER);
 	}
 
+	if (ifindex == nm_device_get_ip_ifindex (self))
+		_stats_update_counters_from_pllink (self, &info);
+
 	had_hw_addr = (priv->hw_addr != NULL);
 	nm_device_update_hw_address (self);
 	got_hw_addr = (!had_hw_addr && priv->hw_addr);
@@ -1958,6 +1954,8 @@ device_ip_link_changed (NMDevice *self)
 	if (!pllink)
 		return G_SOURCE_REMOVE;
 
+	_stats_update_counters_from_pllink (self, pllink);
+
 	if (pllink->name[0] && g_strcmp0 (priv->ip_iface, pllink->name)) {
 		_LOGI (LOGD_DEVICE, "interface index %d renamed ip_iface (%d) from '%s' to '%s'",
 		       priv->ifindex, nm_device_get_ip_ifindex (self),
@@ -2251,6 +2249,7 @@ realize_start_setup (NMDevice *self, const NMPlatformLink *plink)
 	if (plink) {
 		g_return_if_fail (link_type_compatible (self, plink->type, NULL, NULL));
 		update_device_from_platform_link (self, plink);
+		_stats_update_counters_from_pllink (self, plink);
 	}
 
 	if (priv->ifindex > 0) {
@@ -2321,11 +2320,7 @@ realize_start_setup (NMDevice *self, const NMPlatformLink *plink)
 
 	nm_assert (!priv->stats.timeout_id);
 	real_rate = _stats_refresh_rate_real (priv->stats.refresh_rate_ms);
-	if (real_rate) {
-		if (plink)
-			_stats_update_counters (self, plink->tx_bytes, plink->rx_bytes);
-		priv->stats.timeout_id = g_timeout_add (real_rate, _stats_timeout_cb, self);
-	}
+	priv->stats.timeout_id = g_timeout_add (real_rate, _stats_timeout_cb, self);
 
 	klass->realize_start_notify (self, plink);