Re: [PATCH] mac80211: Remove invalid flag operations in mesh TSF synchronization

[Bob Copeland]

On Wed, Dec 07, 2016 at 09:55:41PM +0900, Masashi Honma wrote:
> >It's called mesh_sync_offset_adjust_tbtt() which matches more closely
> >"TBTT adjustment" than "neighbor offset synchronization"?
> 
> I think so. Because there is not any code creating "TBTT Adjustment Request
> frame" even though the frame is required by "TBTT adjustment".

This mesh_sync_offset_adjust_tbtt is definitely doing offset
synchronization, so probably "tbtt" should be renamed "tsf" here.

> >The code
> >looks more like offset synchronization though. Perhaps there's some
> >confusing and it's kinda doing both?
> 
> In theory, updating the flag with 1) looks not correct because it is not
> clearly defined in spec.
> 
> In practice, I could consider extending the meaning of the flag over the
> spec to use it to avoid referring the updating TSF value by peer as Thomas
> said. I have took the statistics how many TSF drift
> (ifmsh->sync_offset_clockdrift_max) happens. The attached file shows the
> stats. The horizontal axis shows TSF drift time(usec) and vertical axis
> shows how many time the drift occurred. The graph shows almost drifts are
> under 20usec. In contrast, 2) could causes more than 1000usec drift. So 1)
> looks not so large enough to protect with the flag.

Yes, offset synchronization is (given decent clocks) supposed to be only
for small tweaks.  We will do it up to .8 ms drift though -- above .8 ms, we
just reset drift to zero and adopt the new timing offset.  You can see this
kind of large "drift" by restarting a station.

Actually, looking at the code now it doesn't make a lot of sense to set
this flag for offset sync because TOFFSET_KNOWN flag is completely cleared
whenever that is set, so we have to be forgetting the current t_offset all
the time?

-- 
Bob Copeland %% http://bobcopeland.com/

Re: [PATCH] mac80211: Remove invalid flag operations in mesh TSF synchronization

[Masashi Honma]

On 2016年12月07日 18:24, Johannes Berg wrote:

And the flag is refered by 1) as you said.


The purpose of the flag is to prevent 1) while 2) is ongoing.

In other words, 1) has only read access authority to the flag.
However,
previous code updated the flag in 1). In addition, there is no code
for
2). So I just remove the invalid accessing codes.


I don't think 1) has read only access to that flag. A TSF adjust will
by definition move the TBTT as well.


It seems that the wording in the spec disagrees with that - it says
(twice) to set the bit only while the TBTT adjustment procedure is
ongoing, which isn't the case here?

Then again, what exactly *is* this code doing?




It's called mesh_sync_offset_adjust_tbtt() which matches more closely
"TBTT adjustment" than "neighbor offset synchronization"?


I think so. Because there is not any code creating "TBTT Adjustment 
Request frame" even though the frame is required by "TBTT adjustment".



The code
looks more like offset synchronization though. Perhaps there's some
confusing and it's kinda doing both?


In theory, updating the flag with 1) looks not correct because it is not 
clearly defined in spec.


In practice, I could consider extending the meaning of the flag over the 
spec to use it to avoid referring the updating TSF value by peer as 
Thomas said. I have took the statistics how many TSF drift 
(ifmsh->sync_offset_clockdrift_max) happens. The attached file shows the 
stats. The horizontal axis shows TSF drift time(usec) and vertical axis 
shows how many time the drift occurred. The graph shows almost drifts 
are under 20usec. In contrast, 2) could causes more than 1000usec drift. 
So 1) looks not so large enough to protect with the flag.


Masashi Honma.

Re: [PATCH] mac80211: Remove invalid flag operations in mesh TSF synchronization

[Johannes Berg]

Hi,

I'm not sure what to do with this now :)

> > There are two functionalities.
> > 
> > 1) 13.13.2.2 Neighbor offset synchronization method
> > 2) 13.13.4.4 TBTT adjustment

Yes, this much is obvious.

> > The flag is updated by 2).

Yes, the definition of the flag says:

"The TBTT Adjusting subfield is set to 1 while the TBTT adjustment
procedure is ongoing, and is set to 0 otherwise. (See 13.13.4.4.3.)"

(802.11-2012 8.4.2.100.8 Mesh Capability)

> > 13.13.4.4.3 TBTT scanning and adjustment procedures:
> > The mesh STA shall set the TBTT Adjusting field in the Mesh
> > Configuration element to 1 in order to announce that the TBTT
> > adjustment procedure is ongoing.

That's saying the same thing again, I guess :)

> > And the flag is refered by 1) as you said.
> > 
> > 
> > The purpose of the flag is to prevent 1) while 2) is ongoing.
> > 
> > In other words, 1) has only read access authority to the flag.
> > However,
> > previous code updated the flag in 1). In addition, there is no code
> > for
> > 2). So I just remove the invalid accessing codes.
> 
> I don't think 1) has read only access to that flag. A TSF adjust will
> by definition move the TBTT as well.

It seems that the wording in the spec disagrees with that - it says
(twice) to set the bit only while the TBTT adjustment procedure is
ongoing, which isn't the case here?

Then again, what exactly *is* this code doing?

It's called mesh_sync_offset_adjust_tbtt() which matches more closely
"TBTT adjustment" than "neighbor offset synchronization"? The code
looks more like offset synchronization though. Perhaps there's some
confusing and it's kinda doing both?

johannes

Re: [PATCH] mac80211: Remove invalid flag operations in mesh TSF synchronization

[Thomas Pedersen]

On Fri, Dec 2, 2016 at 10:59 PM, Masashi Honma  wrote:
> On 2016/12/03 06:13, Bob Copeland wrote:
>>
>> On Fri, Dec 02, 2016 at 12:07:18PM -0800, Thomas Pedersen wrote:
>
>
> # Rejected by linux wireless ML. This is resubmission.
>
> thomas and Bob, Thanks for comments.
>
>> 802.11-2012 13.13.2.2.3:
>> The mesh STA checks if the transmitter of the Beacon frame or Probe
>> Response frame is in the
>> process of the TBTT adjustment (see 13.13.4.4.3).
>
> There are two functionalities.
>
> 1) 13.13.2.2 Neighbor offset synchronization method
> 2) 13.13.4.4 TBTT adjustment
>
> The ifmsh->adjusting_tbtt flag implements "TBTT Adjusting field" in the
> Mesh Configuration field.
>
> The flag is updated by 2).
> 13.13.4.4.3 TBTT scanning and adjustment procedures:
> The mesh STA shall set the TBTT Adjusting field in the Mesh
> Configuration element to 1 in order to announce that the TBTT
> adjustment procedure is ongoing.
>
> And the flag is refered by 1) as you said.
>
>
> The purpose of the flag is to prevent 1) while 2) is ongoing.
>
> In other words, 1) has only read access authority to the flag. However,
> previous code updated the flag in 1). In addition, there is no code for
> 2). So I just remove the invalid accessing codes.

I don't think 1) has read only access to that flag. A TSF adjust will
by definition move the TBTT as well.

-- 
thomas

Re: [PATCH] mac80211: Remove invalid flag operations in mesh TSF synchronization

[Masashi Honma]

On 2016/12/03 06:13, Bob Copeland wrote:

On Fri, Dec 02, 2016 at 12:07:18PM -0800, Thomas Pedersen wrote:


# Rejected by linux wireless ML. This is resubmission.

thomas and Bob, Thanks for comments.

> 802.11-2012 13.13.2.2.3:
> The mesh STA checks if the transmitter of the Beacon frame or Probe
> Response frame is in the
> process of the TBTT adjustment (see 13.13.4.4.3).

There are two functionalities.

1) 13.13.2.2 Neighbor offset synchronization method
2) 13.13.4.4 TBTT adjustment

The ifmsh->adjusting_tbtt flag implements "TBTT Adjusting field" in the
Mesh Configuration field.

The flag is updated by 2).
13.13.4.4.3 TBTT scanning and adjustment procedures:
The mesh STA shall set the TBTT Adjusting field in the Mesh
Configuration element to 1 in order to announce that the TBTT
adjustment procedure is ongoing.

And the flag is refered by 1) as you said.

The purpose of the flag is to prevent 1) while 2) is ongoing.

In other words, 1) has only read access authority to the flag. However,
previous code updated the flag in 1). In addition, there is no code for
2). So I just remove the invalid accessing codes.

Masashi Honma.

Re: [PATCH] mac80211: Remove invalid flag operations in mesh TSF synchronization

[Thomas Pedersen]

On Wed, Nov 30, 2016 at 2:44 PM, Masashi Honma  wrote:
> mesh_sync_offset_adjust_tbtt() implements Extensible synchronization
> framework ([1] 13.13.2 Extensible synchronization framework). It shall
> not operate the flag "TBTT Adjusting subfield" ([1] 8.4.2.100.8 Mesh
> Capability), since it is used only for MBCA ([1] 13.13.4 Mesh beacon
> collision avoidance, see 13.13.4.4.3 TBTT scanning and adjustment
> procedures for detail). So this patch remove the flag operations.

802.11-2012 13.13.2.2.3:

The mesh STA checks if the transmitter of the Beacon frame or Probe
Response frame is in the
process of the TBTT adjustment (see 13.13.4.4.3). If the received
frame contains the Mesh
Configuration element and the TBTT Adjusting subfield in the Mesh
Configuration field is 1, the
mesh STA shall invalidate the T offset value for this neighbor STA and
shall not perform the
following steps.

so, no? I think we need to indicate a TSF adjustment is taking place.

-- 
thomas

[PATCH] mac80211: Remove invalid flag operations in mesh TSF synchronization

[Masashi Honma]

mesh_sync_offset_adjust_tbtt() implements Extensible synchronization
framework ([1] 13.13.2 Extensible synchronization framework). It shall
not operate the flag "TBTT Adjusting subfield" ([1] 8.4.2.100.8 Mesh
Capability), since it is used only for MBCA ([1] 13.13.4 Mesh beacon
collision avoidance, see 13.13.4.4.3 TBTT scanning and adjustment
procedures for detail). So this patch remove the flag operations.

[1] IEEE Std 802.11 2012

Signed-off-by: Masashi Honma 
---
 net/mac80211/mesh_sync.c | 10 --
 1 file changed, 10 deletions(-)

diff --git a/net/mac80211/mesh_sync.c b/net/mac80211/mesh_sync.c
index faca22c..836d791 100644
--- a/net/mac80211/mesh_sync.c
+++ b/net/mac80211/mesh_sync.c
@@ -172,11 +172,9 @@ static void mesh_sync_offset_adjust_tbtt(struct 
ieee80211_sub_if_data *sdata,
 struct beacon_data *beacon)
 {
struct ieee80211_if_mesh *ifmsh = >u.mesh;
-   u8 cap;
 
WARN_ON(ifmsh->mesh_sp_id != IEEE80211_SYNC_METHOD_NEIGHBOR_OFFSET);
WARN_ON(!rcu_read_lock_held());
-   cap = beacon->meshconf->meshconf_cap;
 
spin_lock_bh(>sync_offset_lock);
 
@@ -190,21 +188,13 @@ static void mesh_sync_offset_adjust_tbtt(struct 
ieee80211_sub_if_data *sdata,
  "TBTT : kicking off TBTT adjustment with 
clockdrift_max=%lld\n",
  ifmsh->sync_offset_clockdrift_max);
set_bit(MESH_WORK_DRIFT_ADJUST, >wrkq_flags);
-
-   ifmsh->adjusting_tbtt = true;
} else {
msync_dbg(sdata,
  "TBTT : max clockdrift=%lld; too small to adjust\n",
  (long long)ifmsh->sync_offset_clockdrift_max);
ifmsh->sync_offset_clockdrift_max = 0;
-
-   ifmsh->adjusting_tbtt = false;
}
spin_unlock_bh(>sync_offset_lock);
-
-   beacon->meshconf->meshconf_cap = ifmsh->adjusting_tbtt ?
-   IEEE80211_MESHCONF_CAPAB_TBTT_ADJUSTING | cap :
-   ~IEEE80211_MESHCONF_CAPAB_TBTT_ADJUSTING & cap;
 }
 
 static const struct sync_method sync_methods[] = {
-- 
2.7.4