[patch 04/13] net: mac80211: Use ktime_get_ts()

[patch 04/13] net: mac80211: Use ktime_get_ts()

[Thomas Gleixner]

do_posix_clock_monotonic_gettime() is a leftover from the initial
posix timer implementation which maps to ktime_get_ts().

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: "John W. Linville" <linville@tuxdriver.com>
Cc: linux-wireless@vger.kernel.org
---
 net/mac80211/cfg.c         |    2 +-
 net/mac80211/debugfs_sta.c |    2 +-
 net/mac80211/sta_info.c    |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

Index: linux/net/mac80211/cfg.c
===================================================================
--- linux.orig/net/mac80211/cfg.c
+++ linux/net/mac80211/cfg.c
@@ -484,7 +484,7 @@ static void sta_set_sinfo(struct sta_inf
 			STATION_INFO_STA_FLAGS |
 			STATION_INFO_BEACON_LOSS_COUNT;
 
-	do_posix_clock_monotonic_gettime(&uptime);
+	ktime_get_ts(&uptime);
 	sinfo->connected_time = uptime.tv_sec - sta->last_connected;
 
 	sinfo->inactive_time = jiffies_to_msecs(jiffies - sta->last_rx);
Index: linux/net/mac80211/debugfs_sta.c
===================================================================
--- linux.orig/net/mac80211/debugfs_sta.c
+++ linux/net/mac80211/debugfs_sta.c
@@ -124,7 +124,7 @@ static ssize_t sta_connected_time_read(s
 	long connected_time_secs;
 	char buf[100];
 	int res;
-	do_posix_clock_monotonic_gettime(&uptime);
+	ktime_get_ts(&uptime);
 	connected_time_secs = uptime.tv_sec - sta->last_connected;
 	time_to_tm(connected_time_secs, 0, &result);
 	result.tm_year -= 70;
Index: linux/net/mac80211/sta_info.c
===================================================================
--- linux.orig/net/mac80211/sta_info.c
+++ linux/net/mac80211/sta_info.c
@@ -357,7 +357,7 @@ struct sta_info *sta_info_alloc(struct i
 
 	sta->sta_state = IEEE80211_STA_NONE;
 
-	do_posix_clock_monotonic_gettime(&uptime);
+	ktime_get_ts(&uptime);
 	sta->last_connected = uptime.tv_sec;
 	ewma_init(&sta->avg_signal, 1024, 8);
 	for (i = 0; i < ARRAY_SIZE(sta->chain_signal_avg); i++)

[patch 12/13] net: mac80211: Remove silly timespec dance

[Thomas Gleixner]

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 <tglx@linutronix.de>
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: "John W. Linville" <linville@tuxdriver.com>
Cc: linux-wireless@vger.kernel.org
---
 net/mac80211/status.c |    7 ++-----
 net/mac80211/tx.c     |    5 +----
 2 files changed, 3 insertions(+), 9 deletions(-)

Index: linux/net/mac80211/status.c
===================================================================
--- linux.orig/net/mac80211/status.c
+++ linux/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(net_timedelta(skb_arv));
 
 	if (tx_lat->max < msrmnt) /* update stats */
 		tx_lat->max = msrmnt;
Index: linux/net/mac80211/tx.c
===================================================================
--- linux.orig/net/mac80211/tx.c
+++ linux/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;
-
-	ktime_get_ts(&skb_arv);
-	skb->tstamp = ktime_set(skb_arv.tv_sec, skb_arv.tv_nsec);
+	__net_timestamp(skb);
 }
 
 /**

[patch 11/13] wireless: mwifiex: Use the proper interfaces

[Thomas Gleixner]

Why is converting time formats so desired if there are proper
interfaces for this?

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Bing Zhao <bzhao@marvell.com>
Cc: "John W. Linville" <linville@tuxdriver.com>
Cc: linux-wireless@vger.kernel.org
---
 drivers/net/wireless/mwifiex/cfg80211.c |    4 +---
 drivers/net/wireless/mwifiex/main.c     |    4 +---
 drivers/net/wireless/mwifiex/tdls.c     |    6 ++----
 drivers/net/wireless/mwifiex/uap_txrx.c |    4 +---
 drivers/net/wireless/mwifiex/wmm.c      |    9 +--------
 5 files changed, 6 insertions(+), 21 deletions(-)

Index: linux/drivers/net/wireless/mwifiex/cfg80211.c
===================================================================
--- linux.orig/drivers/net/wireless/mwifiex/cfg80211.c
+++ linux/drivers/net/wireless/mwifiex/cfg80211.c
@@ -151,7 +151,6 @@ mwifiex_form_mgmt_frame(struct sk_buff *
 	u8 addr[ETH_ALEN] = {0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF};
 	u16 pkt_len;
 	u32 tx_control = 0, pkt_type = PKT_TYPE_MGMT;
-	struct timeval tv;
 
 	pkt_len = len + ETH_ALEN;
 
@@ -173,8 +172,7 @@ mwifiex_form_mgmt_frame(struct sk_buff *
 	       len - sizeof(struct ieee80211_hdr_3addr));
 
 	skb->priority = LOW_PRIO_TID;
-	do_gettimeofday(&tv);
-	skb->tstamp = timeval_to_ktime(tv);
+	__net_timestamp(skb);
 
 	return 0;
 }
Index: linux/drivers/net/wireless/mwifiex/main.c
===================================================================
--- linux.orig/drivers/net/wireless/mwifiex/main.c
+++ linux/drivers/net/wireless/mwifiex/main.c
@@ -611,7 +611,6 @@ mwifiex_hard_start_xmit(struct sk_buff *
 	struct mwifiex_private *priv = mwifiex_netdev_get_priv(dev);
 	struct sk_buff *new_skb;
 	struct mwifiex_txinfo *tx_info;
-	struct timeval tv;
 
 	dev_dbg(priv->adapter->dev, "data: %lu BSS(%d-%d): Data <= kernel\n",
 		jiffies, priv->bss_type, priv->bss_num);
@@ -658,8 +657,7 @@ mwifiex_hard_start_xmit(struct sk_buff *
 	 * firmware for aggregate delay calculation for stats and
 	 * MSDU lifetime expiry.
 	 */
-	do_gettimeofday(&tv);
-	skb->tstamp = timeval_to_ktime(tv);
+	__net_timestamp(skb);
 
 	mwifiex_queue_tx_pkt(priv, skb);
 
Index: linux/drivers/net/wireless/mwifiex/tdls.c
===================================================================
--- linux.orig/drivers/net/wireless/mwifiex/tdls.c
+++ linux/drivers/net/wireless/mwifiex/tdls.c
@@ -552,8 +552,7 @@ int mwifiex_send_tdls_data_frame(struct
 	tx_info->bss_num = priv->bss_num;
 	tx_info->bss_type = priv->bss_type;
 
-	do_gettimeofday(&tv);
-	skb->tstamp = timeval_to_ktime(tv);
+	__net_timestamp(skb);
 	mwifiex_queue_tx_pkt(priv, skb);
 
 	return 0;
@@ -710,8 +709,7 @@ int mwifiex_send_tdls_action_frame(struc
 	pkt_len = skb->len - MWIFIEX_MGMT_FRAME_HEADER_SIZE - sizeof(pkt_len);
 	memcpy(skb->data + MWIFIEX_MGMT_FRAME_HEADER_SIZE, &pkt_len,
 	       sizeof(pkt_len));
-	do_gettimeofday(&tv);
-	skb->tstamp = timeval_to_ktime(tv);
+	__net_timestamp(skb);
 	mwifiex_queue_tx_pkt(priv, skb);
 
 	return 0;
Index: linux/drivers/net/wireless/mwifiex/uap_txrx.c
===================================================================
--- linux.orig/drivers/net/wireless/mwifiex/uap_txrx.c
+++ linux/drivers/net/wireless/mwifiex/uap_txrx.c
@@ -96,7 +96,6 @@ static void mwifiex_uap_queue_bridged_pk
 	struct sk_buff *new_skb;
 	struct mwifiex_txinfo *tx_info;
 	int hdr_chop;
-	struct timeval tv;
 	struct ethhdr *p_ethhdr;
 
 	uap_rx_pd = (struct uap_rxpd *)(skb->data);
@@ -192,8 +191,7 @@ static void mwifiex_uap_queue_bridged_pk
 		tx_info->pkt_len = skb->len;
 	}
 
-	do_gettimeofday(&tv);
-	skb->tstamp = timeval_to_ktime(tv);
+	__net_timestamp(skb);
 	mwifiex_wmm_add_buf_txqueue(priv, skb);
 	atomic_inc(&adapter->tx_pending);
 	atomic_inc(&adapter->pending_bridged_pkts);
Index: linux/drivers/net/wireless/mwifiex/wmm.c
===================================================================
--- linux.orig/drivers/net/wireless/mwifiex/wmm.c
+++ linux/drivers/net/wireless/mwifiex/wmm.c
@@ -886,15 +886,8 @@ u8
 mwifiex_wmm_compute_drv_pkt_delay(struct mwifiex_private *priv,
 				  const struct sk_buff *skb)
 {
+	u32 queue_delay = ktime_to_ms(net_timedelta(skb->tstamp));
 	u8 ret_val;
-	struct timeval out_tstamp, in_tstamp;
-	u32 queue_delay;
-
-	do_gettimeofday(&out_tstamp);
-	in_tstamp = ktime_to_timeval(skb->tstamp);
-
-	queue_delay = (out_tstamp.tv_sec - in_tstamp.tv_sec) * 1000;
-	queue_delay += (out_tstamp.tv_usec - in_tstamp.tv_usec) / 1000;
 
 	/*
 	 * Queue delay is passed as a uint8 in units of 2ms (ms shifted

RE: [patch 11/13] wireless: mwifiex: Use the proper interfaces

[Bing Zhao]

Hi Thomas,

Thanks for your patch.

> Why is converting time formats so desired if there are proper
> interfaces for this?
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: Bing Zhao <bzhao@marvell.com>
> Cc: "John W. Linville" <linville@tuxdriver.com>
> Cc: linux-wireless@vger.kernel.org

[...]

> Index: linux/drivers/net/wireless/mwifiex/main.c
> ===================================================================
> --- linux.orig/drivers/net/wireless/mwifiex/main.c
> +++ linux/drivers/net/wireless/mwifiex/main.c
> @@ -611,7 +611,6 @@ mwifiex_hard_start_xmit(struct sk_buff *
>  	struct mwifiex_private *priv = mwifiex_netdev_get_priv(dev);
>  	struct sk_buff *new_skb;
>  	struct mwifiex_txinfo *tx_info;
> -	struct timeval tv;
> 
>  	dev_dbg(priv->adapter->dev, "data: %lu BSS(%d-%d): Data <= kernel\n",
>  		jiffies, priv->bss_type, priv->bss_num);
> @@ -658,8 +657,7 @@ mwifiex_hard_start_xmit(struct sk_buff *
>  	 * firmware for aggregate delay calculation for stats and
>  	 * MSDU lifetime expiry.
>  	 */
> -	do_gettimeofday(&tv);
> -	skb->tstamp = timeval_to_ktime(tv);
> +	__net_timestamp(skb);
> 
>  	mwifiex_queue_tx_pkt(priv, skb);
> 
> Index: linux/drivers/net/wireless/mwifiex/tdls.c
> ===================================================================
> --- linux.orig/drivers/net/wireless/mwifiex/tdls.c
> +++ linux/drivers/net/wireless/mwifiex/tdls.c
> @@ -552,8 +552,7 @@ int mwifiex_send_tdls_data_frame(struct
>  	tx_info->bss_num = priv->bss_num;
>  	tx_info->bss_type = priv->bss_type;
> 
> -	do_gettimeofday(&tv);
> -	skb->tstamp = timeval_to_ktime(tv);
> +	__net_timestamp(skb);

I guess we need to remove "struct timeval tv" local variable too.

>  	mwifiex_queue_tx_pkt(priv, skb);
> 
>  	return 0;
> @@ -710,8 +709,7 @@ int mwifiex_send_tdls_action_frame(struc
>  	pkt_len = skb->len - MWIFIEX_MGMT_FRAME_HEADER_SIZE - sizeof(pkt_len);
>  	memcpy(skb->data + MWIFIEX_MGMT_FRAME_HEADER_SIZE, &pkt_len,
>  	       sizeof(pkt_len));
> -	do_gettimeofday(&tv);
> -	skb->tstamp = timeval_to_ktime(tv);
> +	__net_timestamp(skb);

And here too.

Could you please remove these two "struct timeval tv" and send v2 with my ACK?

Acked-by: Bing Zhao <bzhao@marvell.com>

Thanks,
Bing

>  	mwifiex_queue_tx_pkt(priv, skb);
> 
>  	return 0;

Re: [patch 12/13] net: mac80211: Remove silly timespec dance

[Johannes Berg]

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

> +	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?

johannes

Re: [patch 04/13] net: mac80211: Use ktime_get_ts()

[Johannes Berg]

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

> do_posix_clock_monotonic_gettime() is a leftover from the initial
> posix timer implementation which maps to ktime_get_ts().

I didn't even know we *had* such code, heh.

I've applied it to my tree - note that some code moved into sta_info.c
recently there.

johannes

Re: [patch 12/13] net: mac80211: Remove silly timespec dance

[Thomas Gleixner]

On Thu, 12 Jun 2014, Johannes Berg wrote:

> On Wed, 2014-06-11 at 23:59 +0000, Thomas Gleixner wrote:
> 
> > +	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().

Thanks,

	tglx

[patch V2] wireless: mwifiex: Use the proper interfaces

[Thomas Gleixner]

Why is converting time formats so desired if there are proper
interfaces for this?

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Acked-by: Bing Zhao <bzhao@marvell.com>
Cc: "John W. Linville" <linville@tuxdriver.com>
Cc: linux-wireless@vger.kernel.org
---
 drivers/net/wireless/mwifiex/cfg80211.c |    4 +---
 drivers/net/wireless/mwifiex/main.c     |    4 +---
 drivers/net/wireless/mwifiex/tdls.c     |    8 ++------
 drivers/net/wireless/mwifiex/uap_txrx.c |    4 +---
 drivers/net/wireless/mwifiex/wmm.c      |    9 +--------
 5 files changed, 6 insertions(+), 23 deletions(-)

Index: linux/drivers/net/wireless/mwifiex/cfg80211.c
===================================================================
--- linux.orig/drivers/net/wireless/mwifiex/cfg80211.c
+++ linux/drivers/net/wireless/mwifiex/cfg80211.c
@@ -151,7 +151,6 @@ mwifiex_form_mgmt_frame(struct sk_buff *
 	u8 addr[ETH_ALEN] = {0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF};
 	u16 pkt_len;
 	u32 tx_control = 0, pkt_type = PKT_TYPE_MGMT;
-	struct timeval tv;
 
 	pkt_len = len + ETH_ALEN;
 
@@ -173,8 +172,7 @@ mwifiex_form_mgmt_frame(struct sk_buff *
 	       len - sizeof(struct ieee80211_hdr_3addr));
 
 	skb->priority = LOW_PRIO_TID;
-	do_gettimeofday(&tv);
-	skb->tstamp = timeval_to_ktime(tv);
+	__net_timestamp(skb);
 
 	return 0;
 }
Index: linux/drivers/net/wireless/mwifiex/main.c
===================================================================
--- linux.orig/drivers/net/wireless/mwifiex/main.c
+++ linux/drivers/net/wireless/mwifiex/main.c
@@ -611,7 +611,6 @@ mwifiex_hard_start_xmit(struct sk_buff *
 	struct mwifiex_private *priv = mwifiex_netdev_get_priv(dev);
 	struct sk_buff *new_skb;
 	struct mwifiex_txinfo *tx_info;
-	struct timeval tv;
 
 	dev_dbg(priv->adapter->dev, "data: %lu BSS(%d-%d): Data <= kernel\n",
 		jiffies, priv->bss_type, priv->bss_num);
@@ -658,8 +657,7 @@ mwifiex_hard_start_xmit(struct sk_buff *
 	 * firmware for aggregate delay calculation for stats and
 	 * MSDU lifetime expiry.
 	 */
-	do_gettimeofday(&tv);
-	skb->tstamp = timeval_to_ktime(tv);
+	__net_timestamp(skb);
 
 	mwifiex_queue_tx_pkt(priv, skb);
 
Index: linux/drivers/net/wireless/mwifiex/tdls.c
===================================================================
--- linux.orig/drivers/net/wireless/mwifiex/tdls.c
+++ linux/drivers/net/wireless/mwifiex/tdls.c
@@ -474,7 +474,6 @@ int mwifiex_send_tdls_data_frame(struct
 {
 	struct sk_buff *skb;
 	struct mwifiex_txinfo *tx_info;
-	struct timeval tv;
 	int ret;
 	u16 skb_len;
 
@@ -552,8 +551,7 @@ int mwifiex_send_tdls_data_frame(struct
 	tx_info->bss_num = priv->bss_num;
 	tx_info->bss_type = priv->bss_type;
 
-	do_gettimeofday(&tv);
-	skb->tstamp = timeval_to_ktime(tv);
+	__net_timestamp(skb);
 	mwifiex_queue_tx_pkt(priv, skb);
 
 	return 0;
@@ -645,7 +643,6 @@ int mwifiex_send_tdls_action_frame(struc
 {
 	struct sk_buff *skb;
 	struct mwifiex_txinfo *tx_info;
-	struct timeval tv;
 	u8 *pos;
 	u32 pkt_type, tx_control;
 	u16 pkt_len, skb_len;
@@ -710,8 +707,7 @@ int mwifiex_send_tdls_action_frame(struc
 	pkt_len = skb->len - MWIFIEX_MGMT_FRAME_HEADER_SIZE - sizeof(pkt_len);
 	memcpy(skb->data + MWIFIEX_MGMT_FRAME_HEADER_SIZE, &pkt_len,
 	       sizeof(pkt_len));
-	do_gettimeofday(&tv);
-	skb->tstamp = timeval_to_ktime(tv);
+	__net_timestamp(skb);
 	mwifiex_queue_tx_pkt(priv, skb);
 
 	return 0;
Index: linux/drivers/net/wireless/mwifiex/uap_txrx.c
===================================================================
--- linux.orig/drivers/net/wireless/mwifiex/uap_txrx.c
+++ linux/drivers/net/wireless/mwifiex/uap_txrx.c
@@ -96,7 +96,6 @@ static void mwifiex_uap_queue_bridged_pk
 	struct sk_buff *new_skb;
 	struct mwifiex_txinfo *tx_info;
 	int hdr_chop;
-	struct timeval tv;
 	struct ethhdr *p_ethhdr;
 
 	uap_rx_pd = (struct uap_rxpd *)(skb->data);
@@ -192,8 +191,7 @@ static void mwifiex_uap_queue_bridged_pk
 		tx_info->pkt_len = skb->len;
 	}
 
-	do_gettimeofday(&tv);
-	skb->tstamp = timeval_to_ktime(tv);
+	__net_timestamp(skb);
 	mwifiex_wmm_add_buf_txqueue(priv, skb);
 	atomic_inc(&adapter->tx_pending);
 	atomic_inc(&adapter->pending_bridged_pkts);
Index: linux/drivers/net/wireless/mwifiex/wmm.c
===================================================================
--- linux.orig/drivers/net/wireless/mwifiex/wmm.c
+++ linux/drivers/net/wireless/mwifiex/wmm.c
@@ -886,15 +886,8 @@ u8
 mwifiex_wmm_compute_drv_pkt_delay(struct mwifiex_private *priv,
 				  const struct sk_buff *skb)
 {
+	u32 queue_delay = ktime_to_ms(net_timedelta(skb->tstamp));
 	u8 ret_val;
-	struct timeval out_tstamp, in_tstamp;
-	u32 queue_delay;
-
-	do_gettimeofday(&out_tstamp);
-	in_tstamp = ktime_to_timeval(skb->tstamp);
-
-	queue_delay = (out_tstamp.tv_sec - in_tstamp.tv_sec) * 1000;
-	queue_delay += (out_tstamp.tv_usec - in_tstamp.tv_usec) / 1000;
 
 	/*
 	 * Queue delay is passed as a uint8 in units of 2ms (ms shifted

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

[Johannes Berg]

+netdev, Stephen

On Thu, 2014-06-12 at 10:19 +0200, Thomas Gleixner wrote:
> On Thu, 12 Jun 2014, Johannes Berg wrote:
> 
> > On Wed, 2014-06-11 at 23:59 +0000, Thomas Gleixner wrote:
> > 
> > > +	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?

johannes

Re: [patch V2] wireless: mwifiex: Use the proper interfaces

[Johannes Berg]

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

> +	u32 queue_delay = ktime_to_ms(net_timedelta(skb->tstamp));

FWIW, I think the same as patch 12 applies here. net_timedelta() doesn't
really seem to be a good way to calculate time deltas.

And yes - I've seen situations where this matters, e.g. when running
ntpdate after system startup.

johannes

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

[Johannes Berg]

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:
> > On Thu, 12 Jun 2014, Johannes Berg wrote:
> > 
> > > On Wed, 2014-06-11 at 23:59 +0000, Thomas Gleixner wrote:
> > > 
> > > > +	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?

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

[Thomas Gleixner]

On Thu, 12 Jun 2014, Johannes Berg wrote:

> 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:
> > > On Thu, 12 Jun 2014, Johannes Berg wrote:
> > > 
> > > > On Wed, 2014-06-11 at 23:59 +0000, Thomas Gleixner wrote:
> > > > 
> > > > > +	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.
 
> > 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

Re: [patch 04/13] net: mac80211: Use ktime_get_ts()

[Luis R. Rodriguez]

On Wed, Jun 11, 2014 at 11:53 PM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Wed, 2014-06-11 at 23:59 +0000, Thomas Gleixner wrote:
>
>> do_posix_clock_monotonic_gettime() is a leftover from the initial
>> posix timer implementation which maps to ktime_get_ts().
>
> I didn't even know we *had* such code, heh.

If we want want maintainers to police this stuff we can add an SmPL
rule for this upstream and have maintainer use 'make coccicheck
M=path/'. The way of the future. The way of the future...

  Luis

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

[Johannes Berg]

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

> > > 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.
 
> > > 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

Re: [Cocci] [patch 04/13] net: mac80211: Use ktime_get_ts()

[Julia.Lawall]

"Luis R. Rodriguez" <mcgrof@do-not-panic.com> a écrit :

> On Wed, Jun 11, 2014 at 11:53 PM, Johannes Berg
> <johannes@sipsolutions.net> wrote:
>> On Wed, 2014-06-11 at 23:59 +0000, Thomas Gleixner wrote:
>>
>>> do_posix_clock_monotonic_gettime() is a leftover from the initial
>>> posix timer implementation which maps to ktime_get_ts().
>>
>> I didn't even know we *had* such code, heh.
>
> If we want want maintainers to police this stuff we can add an SmPL
> rule for this upstream and have maintainer use 'make coccicheck
> M=path/'. The way of the future. The way of the future...

If I could have a little more context, I can do something.

julia

>
>   Luis
> _______________________________________________
> Cocci mailing list
> Cocci@systeme.lip6.fr
> https://systeme.lip6.fr/mailman/listinfo/cocci
>

Re: [Cocci] [patch 04/13] net: mac80211: Use ktime_get_ts()

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 883 bytes --]

On Thu, 12 Jun 2014, Julia.Lawall@lip6.fr wrote:

> "Luis R. Rodriguez" <mcgrof@do-not-panic.com> a écrit :
> 
> > On Wed, Jun 11, 2014 at 11:53 PM, Johannes Berg
> > <johannes@sipsolutions.net> wrote:
> > > On Wed, 2014-06-11 at 23:59 +0000, Thomas Gleixner wrote:
> > > 
> > > > do_posix_clock_monotonic_gettime() is a leftover from the initial
> > > > posix timer implementation which maps to ktime_get_ts().
> > > 
> > > I didn't even know we *had* such code, heh.
> > 
> > If we want want maintainers to police this stuff we can add an SmPL
> > rule for this upstream and have maintainer use 'make coccicheck
> > M=path/'. The way of the future. The way of the future...
> 
> If I could have a little more context, I can do something.

I already killed all instances of do_posix_clock_monotonic_gettime().
The define will be gone soon, so nothing to worry about.

Thanks,

	tglx

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

[Thomas Gleixner]

On Thu, 12 Jun 2014, Johannes Berg wrote:

> On Thu, 2014-06-12 at 10:57 +0200, Thomas Gleixner wrote:
> 
> > > > 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.
>  
> > > > 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)

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

Thanks,

	tglx

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

Subject: net: Mac80211: Remove silly timespec dance
From: Thomas Gleixner <tglx@linutronix.de>
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 <tglx@linutronix.de>
Cc: John Stultz <john.stultz@linaro.org>
Cc: Peter Zijlstra <peterz@infradead.org>
Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: John W. Linville <linville@tuxdriver.com>
---
 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;
-
-	ktime_get_ts(&skb_arv);
-	skb->tstamp = ktime_set(skb_arv.tv_sec, skb_arv.tv_nsec);
+	skb->tstamp = ktime_get();
 }
 
 /**

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

[Johannes Berg]

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

RE: [patch V2] wireless: mwifiex: Use the proper interfaces

[Bing Zhao]

DQo+ID4gKwl1MzIgcXVldWVfZGVsYXkgPSBrdGltZV90b19tcyhuZXRfdGltZWRlbHRhKHNrYi0+
dHN0YW1wKSk7DQo+IA0KPiBGV0lXLCBJIHRoaW5rIHRoZSBzYW1lIGFzIHBhdGNoIDEyIGFwcGxp
ZXMgaGVyZS4gbmV0X3RpbWVkZWx0YSgpIGRvZXNuJ3QNCj4gcmVhbGx5IHNlZW0gdG8gYmUgYSBn
b29kIHdheSB0byBjYWxjdWxhdGUgdGltZSBkZWx0YXMuDQoNClNoYWxsIHdlIGNoYW5nZSBuZXRf
dGltZWRlbHRhIHRvIGt0aW1lX3N1YigpIHVzaW5nIGt0aW1lX2dldCgpPw0KDQorCXUzMiBxdWV1
ZV9kZWxheSA9IGt0aW1lX3RvX21zKGt0aW1lX3N1YihrdGltZV9nZXQoKSwgc2tiLT50c3RhbXAp
KTsNCg0KVGhhbmtzLA0KQmluZw0KDQo+IA0KPiBBbmQgeWVzIC0gSSd2ZSBzZWVuIHNpdHVhdGlv
bnMgd2hlcmUgdGhpcyBtYXR0ZXJzLCBlLmcuIHdoZW4gcnVubmluZw0KPiBudHBkYXRlIGFmdGVy
IHN5c3RlbSBzdGFydHVwLg0KPiANCj4gam9oYW5uZXMNCj4gDQoNCg==