GNOME Bugzilla – Bug 720833
[review] th/get_monotonic_timestamp: use monotonic timestamps everywhere
Last modified: 2014-02-03 10:27:24 UTC
(filing for Thomas so we have a bug for reviews) See git branch th/get_monotonic_timestamp
Wow... this patch set is making me suspect that the entire desktop fails badly during a DST "spring forward"... If you added a method nm_connection_update_timestamp() that did basically g_object_set (nm_connection_get_setting_connection (conn), NM_SETTING_CONNECTION_TIMESTAMP, (guint64) time (NULL), NULL); then you could eliminate all the remaining uses of time() in src/, which might be good for confusion-preventing purposes? > core: add nm_utils_get_monotonic_timestamp_* functions >+static long _monotonic_timestamp_offset_nsec; >+ >+static void >+_monotonic_timestamp_get (struct timespec *tp) we don't normally use "_"-prefixing for statics. >+ * The returned value will start counting at 1 second and >+ * always be positive. Given that it starts "at an unspecified offset", it's kinda meaningless to say that it starts at "1 second" (as opposed to just starting a different unspecified offset 1 second before the first one). Just say that it's always positive. Or maybe that it starts "at an unspecified time some time before NetworkManager started, and is thus always positive". > core: use nm_utils_get_monotonic_timestamp_s for timestamp of NMPlatformIP[46]Address We should probably add gtk-docs for NMPlatformIP[46]Address, so we can document that the timestamp fields come from nm_utils_get_monotonic_timestamp_s(). > core: use nm_utils_get_monotonic_timestamp_s for nm_ap_set_last_seen >- glong last_seen; /* Last time the AP was seen in a scan in seconds */ >+ gint64 last_seen; /* Last time the AP was seen in a scan in seconds */ "/* Last time the AP was seen in a scan, as a monotonic timestamp */" ? or "as the seconds portion of a monotonic timestamp"? Although that's wordy. Maybe you should make the _s() function return a guint32, and then the reader will always know that 32-bit monotonic timestamps are seconds, and 64-bit ones are milliseconds. > core: use nm_utils_get_monotonic_timestamp_ms for nm-device-ethernet.c (last_pppoe_time) any reason you switched this from seconds to ms?
(In reply to comment #1) > Wow... this patch set is making me suspect that the entire desktop fails badly > during a DST "spring forward"... It's not that bad. time() returns POSIX time, which AFAIS is like UTC (without leap seconds) -- so, not DST. But it's affected by clock reset, and (not sure about this) maybe you could set the clock to negative values (before 1970). > If you added a method nm_connection_update_timestamp() that did basically > > g_object_set (nm_connection_get_setting_connection (conn), > NM_SETTING_CONNECTION_TIMESTAMP, (guint64) time (NULL), > NULL); > > then you could eliminate all the remaining uses of time() in src/, which might > be good for confusion-preventing purposes? I did not add an update() function, because the timestamp was used in several ways, and a simple update() did not cover all cases. Instead I added nm_setting_connection_get_timestamp_now(). > > core: add nm_utils_get_monotonic_timestamp_* functions > > >+static long _monotonic_timestamp_offset_nsec; > >+ > >+static void > >+_monotonic_timestamp_get (struct timespec *tp) > > we don't normally use "_"-prefixing for statics. Changed > >+ * The returned value will start counting at 1 second and > >+ * always be positive. > > Given that it starts "at an unspecified offset", it's kinda meaningless to say > that it starts at "1 second" (as opposed to just starting a different > unspecified offset 1 second before the first one). Just say that it's always > positive. Or maybe that it starts "at an unspecified time some time before > NetworkManager started, and is thus always positive". Changed > > core: use nm_utils_get_monotonic_timestamp_s for timestamp of NMPlatformIP[46]Address > > We should probably add gtk-docs for NMPlatformIP[46]Address, so we can document > that the timestamp fields come from nm_utils_get_monotonic_timestamp_s(). Changed. I only commented the timestamp field though. > > core: use nm_utils_get_monotonic_timestamp_s for nm_ap_set_last_seen > > >- glong last_seen; /* Last time the AP was seen in a scan in seconds */ > >+ gint64 last_seen; /* Last time the AP was seen in a scan in seconds */ > > "/* Last time the AP was seen in a scan, as a monotonic timestamp */" ? > > or "as the seconds portion of a monotonic timestamp"? Although that's wordy. > Maybe you should make the _s() function return a guint32, and then the reader > will always know that 32-bit monotonic timestamps are seconds, and 64-bit ones > are milliseconds. Changed. get_monotonic_timestamp_s now returns gint32, which gives us 60 years before overflow. > > core: use nm_utils_get_monotonic_timestamp_ms for nm-device-ethernet.c (last_pppoe_time) > > any reason you switched this from seconds to ms? I prefer the higher precision of ms where applicable. Even though later we use g_timeout_add_seconds to schedule events (which minimizes wakeups).
(I forgot to mention: repushed, please re-review)
(In reply to comment #2) > (In reply to comment #1) > > Wow... this patch set is making me suspect that the entire desktop fails badly > > during a DST "spring forward"... > > It's not that bad. time() returns POSIX time, which AFAIS is like UTC (without > leap seconds) -- so, not DST. Ah, right, of course. > I did not add an update() function, because the timestamp was used in several > ways, and a simple update() did not cover all cases. Instead I added > nm_setting_connection_get_timestamp_now(). I decided I don't actually like this. So I'd say drop that commit. Everything else looks good.
(In reply to comment #4) > > I did not add an update() function, because the timestamp was used in several > > ways, and a simple update() did not cover all cases. Instead I added > > nm_setting_connection_get_timestamp_now(). > > I decided I don't actually like this. So I'd say drop that commit. I quite like it. I slightly tend to keep it. Other opinions? PS. I rebased the branch onto master and removed get_time() from nm-linux-platform.c
Rebased branch on current master. Commit "core: use nm_utils_get_monotonic_timestamp_s for autoconnect_retry_time" needed some more changes
Great job, I like the convenience functions and all. A couple of notes... 1) You don't need to care about int32 for monotonic timestamps unless you handle values tens of years into the future. What is the rationale for negating the timestamp values in monotonic_timestamp_get() and how that works? 2) Is the off-by-one shift actually needed? I understand there may be situations when you could confuse a zero by default with a zero by measurement but I'm not aware of a situation where it could actually cause a problem. It would be IMO nicer to avoid the shift so that we stick to a well known scale of CLOCK_BOOTTIME. 3) What's the purpose of the static variables? Is it a left over of some kind?
(In reply to comment #7) > Great job, I like the convenience functions and all. A couple of notes... > > 1) You don't need to care about int32 for monotonic timestamps unless you > handle values tens of years into the future. What is the rationale for negating > the timestamp values in monotonic_timestamp_get() and how that works? It shifts the timestamps so that the very first returned timestamp starts counting at 1 second. Actually, I simplified it and reworded the code comments. See the pushed !fixup. Now it shifts the values to start counting at (1sec <= start < 2sec) to save one integer addition. > 2) Is the off-by-one shift actually needed? I understand there may be > situations when you could confuse a zero by default with a zero by measurement > but I'm not aware of a situation where it could actually cause a problem. > > It would be IMO nicer to avoid the shift so that we stick to a well known scale > of CLOCK_BOOTTIME. I don't agree. One convenience I want for get_monotonic_timestamp_s(), is that the values are always greater or equal to 1 second. This is nice for example, if you have a timestamp in a private structure of a class. Then at object creation, the value will be initialized to zero -- a non existing timestamp. You can treat 0 as "unknown" To guarantee that the values (after casting to seconds) are positive at early boot, shifting is needed. Yes, I could also add 1 second instead. But would that be any better? This way we always make use of the full ~68 years range when holding the timestamp as int32. Whenever you need a timestamp, you have to take the proper one anyway. get_monotonic_timestamp() is for a certain use case, there might be (exotic) cases, where CLOCK_BOOTTIME is better. They don't have to be the same. If we ever need it, it would be easy to convert a get_monotonic_timestamp() to CLOCK_BOOTTIME and vice versa. > 3) What's the purpose of the static variables? Is it a left over of some kind? no. The are used to remember the amount of shifting.
(In reply to comment #6) > Commit "core: use nm_utils_get_monotonic_timestamp_s for > autoconnect_retry_time" needed some more changes - if (con_stamp < now) { + if (con_stamp <= now) { That part looks like a bug in my autoconnect changes, not something related to get_monotonic_timestamp, right? If so, please commit as a separate patch. > fixup! core: add nm_utils_get_monotonic_timestamp_* functions >+ g_assert (tp->tv_nsec >= 0 || tp->tv_nsec < NM_UTILS_NS_PER_SECOND); &&, not || everything else looks good
Pushed to master as http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=adbcd9a1f0d0ad481ab4e489854f215d64c6ac3e
for the record, I IRC-acked the final merged patches
(In reply to comment #8) > > 2) Is the off-by-one shift actually needed? I understand there may be > > situations when you could confuse a zero by default with a zero by measurement > > but I'm not aware of a situation where it could actually cause a problem. > > > > It would be IMO nicer to avoid the shift so that we stick to a well known scale > > of CLOCK_BOOTTIME. > > I don't agree. One convenience I want for get_monotonic_timestamp_s(), is that > the values are always greater or equal to 1 second. This is nice for example, > if you have a timestamp in a private structure of a class. Then at object > creation, the value will be initialized to zero -- a non existing timestamp. > You can treat 0 as "unknown" To guarantee that the values (after casting to > seconds) are positive at early boot, shifting is needed. That's a description of what's already known. I was asking for an actual use case as I couldn't think of one myself. > > 3) What's the purpose of the static variables? Is it a left over of some kind? > > no. The are used to remember the amount of shifting. Could you be more verbose about their purpose?
(In reply to comment #12) > (In reply to comment #8) > > > 2) Is the off-by-one shift actually needed? I understand there may be > > > situations when you could confuse a zero by default with a zero by measurement > > > but I'm not aware of a situation where it could actually cause a problem. > > > > > > It would be IMO nicer to avoid the shift so that we stick to a well known scale > > > of CLOCK_BOOTTIME. > > > > I don't agree. One convenience I want for get_monotonic_timestamp_s(), is that > > the values are always greater or equal to 1 second. This is nice for example, > > if you have a timestamp in a private structure of a class. Then at object > > creation, the value will be initialized to zero -- a non existing timestamp. > > You can treat 0 as "unknown" To guarantee that the values (after casting to > > seconds) are positive at early boot, shifting is needed. > > That's a description of what's already known. I was asking for an actual use > case as I couldn't think of one myself. Nothing except then to guarantee positive (seconds) timestamps. A minuscule reason is to enlarge the potential range of seconds to fully 31 bit. > > > 3) What's the purpose of the static variables? Is it a left over of some kind? > > > > no. The are used to remember the amount of shifting. > > Could you be more verbose about their purpose? Do you mean http://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/src/NetworkManagerUtils.c?id=e922c120a3cf1bb3bd8def11bd0f23f1e18a0751#n757 ? ... the purpose is to store the shift-offset in this global variable, and to use it whenever calculating the timestamp.
(In reply to comment #13) > Nothing except then to guarantee positive (seconds) timestamps. A minuscule > reason is to enlarge the potential range of seconds to fully 31 bit. OK, while I still don't see a good use case for special casing the zero but that's not so important. > > > > 3) What's the purpose of the static variables? Is it a left over of some kind? > > > > > > no. The are used to remember the amount of shifting. > > > > Could you be more verbose about their purpose? > > Do you mean > http://cgit.freedesktop.org/NetworkManager/NetworkManager/tree/src/NetworkManagerUtils.c?id=e922c120a3cf1bb3bd8def11bd0f23f1e18a0751#n757 > ? ... the purpose is to store the shift-offset in this global variable, and to > use it whenever calculating the timestamp. That means the actual numbers used throughout NetworkManager is roughly in seconds from NetworkManager bootup, not seconds from system bootup? What's the rationale for that? Also, that means you translate old times to negative values, right? And therefore a zero time would become valid again, is that correct? A theoretical example would be data from DHCP leases.