| Message ID | 1480545889-3690-1-git-send-email-masashi.honma@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Johannes Berg |
| Headers | show |
On Wed, Nov 30, 2016 at 2:44 PM, Masashi Honma <masashi.honma@gmail.com> 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.
On Fri, Dec 02, 2016 at 12:07:18PM -0800, Thomas Pedersen wrote: > On Wed, Nov 30, 2016 at 2:44 PM, Masashi Honma <masashi.honma@gmail.com> 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: [snip] > so, no? I think we need to indicate a TSF adjustment is taking place. I think so too. I must ask, what is the prompt for removing it, are you (Masashi) trying to implement MBCA and this is interfering?
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.
On Fri, Dec 2, 2016 at 10:59 PM, Masashi Honma <masashi.honma@gmail.com> 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.
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
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.
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?
On 2016年12月07日 22:33, Bob Copeland wrote: > This mesh_sync_offset_adjust_tbtt is definitely doing offset > synchronization, so probably "tbtt" should be renamed "tsf" here. Right. I will send a patch for this. > 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? Right, TOFFSET_KNOWN flag looks not need to be cleared when TBTT_ADJUSTING flag is on. Because the t_offset is received when TBTT_ADJUSTING flag is off. The t_offset is expected to have valid value. I will remove the code by next patch. Masashi Honma.
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 = &sdata->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(&ifmsh->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, &ifmsh->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(&ifmsh->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[] = {
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 <masashi.honma@gmail.com> --- net/mac80211/mesh_sync.c | 10 ---------- 1 file changed, 10 deletions(-)