Re: net_timedelta() affected by settimeofday() (was: [patch 12/13] net: mac80211: Remove silly timespec dance)

On Thu, 2014-06-12 at 10:35 +0200, Johannes Berg wrote:

+netdev, Stephen

Well, stupid me. Fixing that netdev address.

On Thu, 2014-06-12 at 10:19 +0200, Thomas Gleixner wrote:

quoted

On Thu, 12 Jun 2014, Johannes Berg wrote:

quoted

On Wed, 2014-06-11 at 23:59 +0000, Thomas Gleixner wrote:

quoted

+	msrmnt = ktime_to_ms(net_timedelta(skb_arv));

This is probably more of a question about net_timedelta(), but is
ktime_get_real() really appropriate for duration measurements? Isn't
that non-monotonic?

Well, it's monotonic, but might be affected by settimeofday().

Right, but isn't that odd? Suddenly your delay measurement here might be
minutes, hours, or years if you settimeofday() between timestamping and
calculating the delta. That seems very strange to me, why would that be
the right behaviour in any way?

Now, it seems that there are only two current users of net_timedelta()
(in DCCP) so perhaps it's not too late to change some of this?

Maybe in general the skb timestamp should be based on a different clock
and only adjusted to real time when used in userspace?

On Thu, 12 Jun 2014, Johannes Berg wrote:

On Thu, 2014-06-12 at 10:35 +0200, Johannes Berg wrote:

quoted

+netdev, Stephen

Well, stupid me. Fixing that netdev address.

quoted

On Thu, 2014-06-12 at 10:19 +0200, Thomas Gleixner wrote:

quoted

On Thu, 12 Jun 2014, Johannes Berg wrote:

quoted

On Wed, 2014-06-11 at 23:59 +0000, Thomas Gleixner wrote:

quoted

+	msrmnt = ktime_to_ms(net_timedelta(skb_arv));

This is probably more of a question about net_timedelta(), but is
ktime_get_real() really appropriate for duration measurements? Isn't
that non-monotonic?

Well, it's monotonic, but might be affected by settimeofday().

Right, but isn't that odd? Suddenly your delay measurement here might be
minutes, hours, or years if you settimeofday() between timestamping and
calculating the delta. That seems very strange to me, why would that be
the right behaviour in any way?

Indeed. clock monotonic is the appropriate one for measurements.
 

quoted

Now, it seems that there are only two current users of net_timedelta()
(in DCCP) so perhaps it's not too late to change some of this?

Maybe in general the skb timestamp should be based on a different clock
and only adjusted to real time when used in userspace?

You have the same problem then, just at a different place:

    ts = ktime_get();

			settimeofday()
			   offset_mono_to_real = new value;

    userts = mono_to_real(ts);

But maybe that's not a real issue, as ktime_get_real() can race with
settimeofday() or NTP as well.

   ts = ktime_get_real();
				settimeofday();
   userts = ts;

So the user might see a weird timestamp for a packet, which cannot be
correlated with the user space gettimeofday().

Thanks,

	tglx

On Thu, 2014-06-12 at 10:57 +0200, Thomas Gleixner wrote:

quoted

quoted

Right, but isn't that odd? Suddenly your delay measurement here might be
minutes, hours, or years if you settimeofday() between timestamping and
calculating the delta. That seems very strange to me, why would that be
the right behaviour in any way?

Indeed. clock monotonic is the appropriate one for measurements.

And that's what we had here with ktime_get_ts()? I thought so, just
making sure.
 

quoted

quoted

Now, it seems that there are only two current users of net_timedelta()
(in DCCP) so perhaps it's not too late to change some of this?

Maybe in general the skb timestamp should be based on a different clock
and only adjusted to real time when used in userspace?

You have the same problem then, just at a different place:

    ts = ktime_get();

			settimeofday()
			   offset_mono_to_real = new value;

    userts = mono_to_real(ts);

Right. I'm not really sure if that's an issue though.

But maybe that's not a real issue, as ktime_get_real() can race with
settimeofday() or NTP as well.

   ts = ktime_get_real();
				settimeofday();
   userts = ts;

So the user might see a weird timestamp for a packet, which cannot be
correlated with the user space gettimeofday().

Right, once settimeofday() is called the timestamps from before/during
it can't really be correlated any more.

This is part of the userspace API already, but might it have been better
to expose the monotonic clock, since userspace can also get at it? Not
sure.

Either way it's an issue I guess; however I'm thinking your patch is
making it worse for the measurement in this particular code (where the
userspace issue doesn't come in, it should never be accessible there)

johannes

On Thu, 12 Jun 2014, Johannes Berg wrote:

On Thu, 2014-06-12 at 10:57 +0200, Thomas Gleixner wrote:

quoted

quoted

quoted

Right, but isn't that odd? Suddenly your delay measurement here might be
minutes, hours, or years if you settimeofday() between timestamping and
calculating the delta. That seems very strange to me, why would that be
the right behaviour in any way?

Indeed. clock monotonic is the appropriate one for measurements.

And that's what we had here with ktime_get_ts()? I thought so, just
making sure.
 

quoted

quoted

quoted

Now, it seems that there are only two current users of net_timedelta()
(in DCCP) so perhaps it's not too late to change some of this?

Maybe in general the skb timestamp should be based on a different clock
and only adjusted to real time when used in userspace?

You have the same problem then, just at a different place:

    ts = ktime_get();

			settimeofday()
			   offset_mono_to_real = new value;

    userts = mono_to_real(ts);

Right. I'm not really sure if that's an issue though.

quoted

But maybe that's not a real issue, as ktime_get_real() can race with
settimeofday() or NTP as well.

   ts = ktime_get_real();
				settimeofday();
   userts = ts;

So the user might see a weird timestamp for a packet, which cannot be
correlated with the user space gettimeofday().

Right, once settimeofday() is called the timestamps from before/during
it can't really be correlated any more.

This is part of the userspace API already, but might it have been better
to expose the monotonic clock, since userspace can also get at it? Not
sure.

Either way it's an issue I guess; however I'm thinking your patch is
making it worse for the measurement in this particular code (where the
userspace issue doesn't come in, it should never be accessible there)

Fair enough. Still the timespec is silly. Here is an updated version.

Thanks,

	tglx

------------------>

Subject: net: Mac80211: Remove silly timespec dance
From: Thomas Gleixner <redacted>
Date: Wed, 11 Jun 2014 23:59:18 -0000

Converting time from one format to another seems to give coders a warm
and fuzzy feeling.

Use the proper interfaces.

Signed-off-by: Thomas Gleixner <redacted>
Cc: John Stultz <redacted>
Cc: Peter Zijlstra <redacted>
Cc: Johannes Berg <redacted>
Cc: John W. Linville <redacted>
---
 net/mac80211/status.c |    7 ++-----
 net/mac80211/tx.c     |    5 +----
 2 files changed, 3 insertions(+), 9 deletions(-)


Index: tip/net/mac80211/status.c
===================================================================

--- tip.orig/net/mac80211/status.c
+++ tip/net/mac80211/status.c

@@ -473,8 +473,6 @@ static void ieee80211_tx_latency_end_msr
 					    struct sta_info *sta,
 					    struct ieee80211_hdr *hdr)
 {
-	ktime_t skb_dprt;
-	struct timespec dprt_time;
 	u32 msrmnt;
 	u16 tid;
 	u8 *qc;

@@ -506,9 +504,8 @@ static void ieee80211_tx_latency_end_msr
 
 	tx_lat = &sta->tx_lat[tid];
 
-	ktime_get_ts(&dprt_time); /* time stamp completion time */
-	skb_dprt = ktime_set(dprt_time.tv_sec, dprt_time.tv_nsec);
-	msrmnt = ktime_to_ms(ktime_sub(skb_dprt, skb_arv));
+	/* Calculate the latency */
+	msrmnt = ktime_to_ms(ktime_sub(ktime_get(), skb_arv);
 
 	if (tx_lat->max < msrmnt) /* update stats */
 		tx_lat->max = msrmnt;

Index: tip/net/mac80211/tx.c
===================================================================

--- tip.orig/net/mac80211/tx.c
+++ tip/net/mac80211/tx.c

@@ -1767,15 +1767,12 @@ fail:
 static void ieee80211_tx_latency_start_msrmnt(struct ieee80211_local *local,
 					      struct sk_buff *skb)
 {
-	struct timespec skb_arv;
 	struct ieee80211_tx_latency_bin_ranges *tx_latency;
 
 	tx_latency = rcu_dereference(local->tx_latency);
 	if (!tx_latency)
 		return;

On Thu, 2014-06-12 at 16:09 +0200, Thomas Gleixner wrote:

Fair enough. Still the timespec is silly. Here is an updated version.

Makes sense, thanks. I've applied this (mac80211-next)

johannes