diff mbox series

[PATCH-v3,1/2] wireless: Support assoc-at-ms timer in sta-info

Message ID 20190415172123.6532-1-greearb@candelatech.com (mailing list archive)
State Changes Requested
Delegated to: Johannes Berg
Headers show
Series [PATCH-v3,1/2] wireless: Support assoc-at-ms timer in sta-info | expand

Commit Message

Ben Greear April 15, 2019, 5:21 p.m. UTC 
From: Ben Greear <greearb@candelatech.com>

Report time stamp of when sta became associated.

Signed-off-by: Ben Greear <greearb@candelatech.com>
---
 include/net/cfg80211.h       | 2 ++
 include/uapi/linux/nl80211.h | 2 ++
 net/wireless/nl80211.c       | 1 +
 3 files changed, 5 insertions(+)

Comments

Johannes Berg June 14, 2019, 1:38 p.m. UTC | #1 
On Mon, 2019-04-15 at 10:21 -0700, greearb@candelatech.com wrote:
> From: Ben Greear <greearb@candelatech.com>
> 
> Report time stamp of when sta became associated.

I guess that makes sense.

> Signed-off-by: Ben Greear <greearb@candelatech.com>
> ---
>  include/net/cfg80211.h       | 2 ++
>  include/uapi/linux/nl80211.h | 2 ++
>  net/wireless/nl80211.c       | 1 +
>  3 files changed, 5 insertions(+)
> 
> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
> index f49eb1464b7a..a3ad78b9d9f4 100644
> --- a/include/net/cfg80211.h
> +++ b/include/net/cfg80211.h
> @@ -1268,6 +1268,7 @@ struct cfg80211_tid_stats {
>   *	indicate the relevant values in this struct for them
>   * @connected_time: time(in secs) since a station is last connected
>   * @inactive_time: time since last station activity (tx/rx) in milliseconds
> + * @assoc_at_ms: time in ms of the last association

I think the "_at_ms" doesn't really make sense. "time in ms" also
doesn't make sense as documentation. I think you need to say what should
be assigned here.

Also, you're actually using ktime_get_real() for this, which again
doesn't make much sense. For exposing an absolute time, I think we're
better off with CLOCK_BOOTTIME like we use elsewhere:

 * @boottime_ns: CLOCK_BOOTTIME timestamp the frame was received at, this is
 *      needed only for beacons and probe responses that update the scan cache.


> + * @NL80211_STA_INFO_ASSOC_AT_MS: Timestamp of last association

_ASSOC_TIMESTAMP or something would make more sense too as the attribute
name, and clearly the documentation needs to spell out the semantics
here too.

johannes
Ben Greear June 14, 2019, 2:46 p.m. UTC | #2 
On 6/14/19 6:38 AM, Johannes Berg wrote:
> On Mon, 2019-04-15 at 10:21 -0700, greearb@candelatech.com wrote:
>> From: Ben Greear <greearb@candelatech.com>
>>
>> Report time stamp of when sta became associated.
> 
> I guess that makes sense.
> 
>> Signed-off-by: Ben Greear <greearb@candelatech.com>
>> ---
>>   include/net/cfg80211.h       | 2 ++
>>   include/uapi/linux/nl80211.h | 2 ++
>>   net/wireless/nl80211.c       | 1 +
>>   3 files changed, 5 insertions(+)
>>
>> diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
>> index f49eb1464b7a..a3ad78b9d9f4 100644
>> --- a/include/net/cfg80211.h
>> +++ b/include/net/cfg80211.h
>> @@ -1268,6 +1268,7 @@ struct cfg80211_tid_stats {
>>    *	indicate the relevant values in this struct for them
>>    * @connected_time: time(in secs) since a station is last connected
>>    * @inactive_time: time since last station activity (tx/rx) in milliseconds
>> + * @assoc_at_ms: time in ms of the last association
> 
> I think the "_at_ms" doesn't really make sense. "time in ms" also
> doesn't make sense as documentation. I think you need to say what should
> be assigned here.
> 
> Also, you're actually using ktime_get_real() for this, which again
> doesn't make much sense. For exposing an absolute time, I think we're
> better off with CLOCK_BOOTTIME like we use elsewhere:

The point of my patch was to allow 'iw' to return a more precise time
that the station has been associated, so I am not sure that BOOTIME is
a good thing to use for that?

Here are the pertinent parts of my iw patches:


diff --git a/station.c b/station.c
index 25cbbc3..e7738cc 100644
--- a/station.c
+++ b/station.c
@@ -314,6 +314,12 @@ static int print_sta_handler(struct nl_msg *msg, void *arg)
                 [NL80211_STA_INFO_ACK_SIGNAL_AVG] = { .type = NLA_U8 },
         };
         char *chain;
+       struct timeval now;
+       unsigned long long now_ms;
+
+       gettimeofday(&now, NULL);
+       now_ms = now.tv_sec * 1000;
+       now_ms += (now.tv_usec / 1000);

         nla_parse(tb, NL80211_ATTR_MAX, genlmsg_attrdata(gnlh, 0),
                   genlmsg_attrlen(gnlh, 0), NULL);
@@ -557,8 +563,11 @@ static int print_sta_handler(struct nl_msg *msg, void *arg)
         if (sinfo[NL80211_STA_INFO_CONNECTED_TIME])
                 printf("\n\tconnected time:\t%u seconds",
                         nla_get_u32(sinfo[NL80211_STA_INFO_CONNECTED_TIME]));
+       if (sinfo[NL80211_STA_INFO_ASSOC_AT_MS])
+               printf("\n\tassociated at:\t%llu ms",
+                        (unsigned long long)nla_get_u64(sinfo[NL80211_STA_INFO_ASSOC_AT_MS]));

-       printf("\n");
+       printf("\n\tcurrent time:\t%llu ms\n", now_ms);
         return NL_SKIP;
  }


Thanks,
Ben

> 
>   * @boottime_ns: CLOCK_BOOTTIME timestamp the frame was received at, this is
>   *      needed only for beacons and probe responses that update the scan cache.
> 
> 
>> + * @NL80211_STA_INFO_ASSOC_AT_MS: Timestamp of last association
> 
> _ASSOC_TIMESTAMP or something would make more sense too as the attribute
> name, and clearly the documentation needs to spell out the semantics
> here too.
> 
> johannes
>
Johannes Berg June 14, 2019, 2:56 p.m. UTC | #3 
On Fri, 2019-06-14 at 07:46 -0700, Ben Greear wrote:
> 
> The point of my patch was to allow 'iw' to return a more precise time
> that the station has been associated, so I am not sure that BOOTIME is
> a good thing to use for that?

Depends what you want, really.

> +       if (sinfo[NL80211_STA_INFO_ASSOC_AT_MS])
> +               printf("\n\tassociated at:\t%llu ms",
> +                        (unsigned long long)nla_get_u64(sinfo[NL80211_STA_INFO_ASSOC_AT_MS]));
> 
> -       printf("\n");
> +       printf("\n\tcurrent time:\t%llu ms\n", now_ms);

Since you just print the time in milliseconds, and the current time in
milliseconds, you don't even really have any value in the wall clock.
Quite the opposite - this lends itself to subtracting to try to figure
out how long it was associated, which is the completely wrong thing to
do with wall clock - timezone adjustment could've happened inbetween,
for example!

I really see no point in trying to give the wall clock at assoc time. If
no timezone adjustment happened, you can just as well give the boottime
and have the userspace figure out the wall clock. If timezone adjustment
happened, then any calculations are wrong anyway, what would the point
be?

johannes
Ben Greear June 14, 2019, 3:14 p.m. UTC | #4 
On 6/14/19 7:56 AM, Johannes Berg wrote:
> On Fri, 2019-06-14 at 07:46 -0700, Ben Greear wrote:
>>
>> The point of my patch was to allow 'iw' to return a more precise time
>> that the station has been associated, so I am not sure that BOOTIME is
>> a good thing to use for that?
> 
> Depends what you want, really.
> 
>> +       if (sinfo[NL80211_STA_INFO_ASSOC_AT_MS])
>> +               printf("\n\tassociated at:\t%llu ms",
>> +                        (unsigned long long)nla_get_u64(sinfo[NL80211_STA_INFO_ASSOC_AT_MS]));
>>
>> -       printf("\n");
>> +       printf("\n\tcurrent time:\t%llu ms\n", now_ms);
> 
> Since you just print the time in milliseconds, and the current time in
> milliseconds, you don't even really have any value in the wall clock.
> Quite the opposite - this lends itself to subtracting to try to figure
> out how long it was associated, which is the completely wrong thing to
> do with wall clock - timezone adjustment could've happened inbetween,
> for example!
> 
> I really see no point in trying to give the wall clock at assoc time. If
> no timezone adjustment happened, you can just as well give the boottime
> and have the userspace figure out the wall clock. If timezone adjustment
> happened, then any calculations are wrong anyway, what would the point
> be?

So, maybe I return instead the elapsed time in the netlink API instead of a
timestamp.  I think that will give me the value that I am looking for,
and I can still print out the 'real' time in iw so any tools reading that
output and do some simple math and figure out the 'real' associated-at time.

If that sounds good to you, I'll code that up.

Thanks,
Ben

> 
> johannes
>
Johannes Berg June 14, 2019, 6:46 p.m. UTC | #5 
On Fri, 2019-06-14 at 08:14 -0700, Ben Greear wrote:
> 
> 
> So, maybe I return instead the elapsed time in the netlink API instead of a
> timestamp.  I think that will give me the value that I am looking for,
> and I can still print out the 'real' time in iw so any tools reading that
> output and do some simple math and figure out the 'real' associated-at time.

I don't think that's good. There's a delay between filling the message
and then processing it in userspace. We had this in the scan code and
learned that was full of races.

The better thing is to use CLOCK_BOOTTIME nanoseconds, and then
userspace can use clock_gettime() to get the current time etc. and then
do whatever calculations it needs. If it wants to print the realtime
timestamp, it could even calculate that (with some jitter) by doing
clock_gettime() on both realtime and boottime clocks...

johannes
diff mbox series

Patch

diff --git a/include/net/cfg80211.h b/include/net/cfg80211.h
index f49eb1464b7a..a3ad78b9d9f4 100644
--- a/include/net/cfg80211.h
+++ b/include/net/cfg80211.h
@@ -1268,6 +1268,7 @@  struct cfg80211_tid_stats {
  *	indicate the relevant values in this struct for them
  * @connected_time: time(in secs) since a station is last connected
  * @inactive_time: time since last station activity (tx/rx) in milliseconds
+ * @assoc_at_ms: time in ms of the last association
  * @rx_bytes: bytes (size of MPDUs) received from this station
  * @tx_bytes: bytes (size of MPDUs) transmitted to this station
  * @llid: mesh local link id
@@ -1324,6 +1325,7 @@  struct station_info {
 	u64 filled;
 	u32 connected_time;
 	u32 inactive_time;
+	u64 assoc_at_ms;
 	u64 rx_bytes;
 	u64 tx_bytes;
 	u16 llid;
diff --git a/include/uapi/linux/nl80211.h b/include/uapi/linux/nl80211.h
index 0f03cfcda965..0fcedde890e7 100644
--- a/include/uapi/linux/nl80211.h
+++ b/include/uapi/linux/nl80211.h
@@ -3079,6 +3079,7 @@  enum nl80211_sta_bss_param {
  * @NL80211_STA_INFO_TX_DURATION: aggregate PPDU duration for all frames
  *	sent to the station (u64, usec)
  * @NL80211_STA_INFO_AIRTIME_WEIGHT: current airtime weight for station (u16)
+ * @NL80211_STA_INFO_ASSOC_AT_MS: Timestamp of last association
  * @__NL80211_STA_INFO_AFTER_LAST: internal
  * @NL80211_STA_INFO_MAX: highest possible station info attribute
  */
@@ -3124,6 +3125,7 @@  enum nl80211_sta_info {
 	NL80211_STA_INFO_CONNECTED_TO_GATE,
 	NL80211_STA_INFO_TX_DURATION,
 	NL80211_STA_INFO_AIRTIME_WEIGHT,
+	NL80211_STA_INFO_ASSOC_AT_MS,
 
 	/* keep last */
 	__NL80211_STA_INFO_AFTER_LAST,
diff --git a/net/wireless/nl80211.c b/net/wireless/nl80211.c
index bcb432815c64..cb5acf026900 100644
--- a/net/wireless/nl80211.c
+++ b/net/wireless/nl80211.c
@@ -4763,6 +4763,7 @@  static int nl80211_send_station(struct sk_buff *msg, u32 cmd, u32 portid,
 
 	PUT_SINFO(CONNECTED_TIME, connected_time, u32);
 	PUT_SINFO(INACTIVE_TIME, inactive_time, u32);
+	PUT_SINFO_U64(ASSOC_AT_MS, assoc_at_ms);
 
 	if (sinfo->filled & (BIT_ULL(NL80211_STA_INFO_RX_BYTES) |
 			     BIT_ULL(NL80211_STA_INFO_RX_BYTES64)) &&