RE: [PATCH 2/4] mac80211/cfg: mesh: fix healing time when a mesh peer is disconnecting

[Machani, Yaniv]

On Tue, Jun 28, 2016 at 15:27:47, Bob Copeland wrote:
> linux- wirel...@vger.kernel.org; netdev@vger.kernel.org; Hahn, Maital
> Subject: Re: [PATCH 2/4] mac80211/cfg: mesh: fix healing time when a 
> mesh peer is disconnecting
> 
> On Tue, Jun 28, 2016 at 02:13:05PM +0300, Yaniv Machani wrote:
> > From: Maital Hahn <mait...@ti.com>
> >
> > Once receiving a CLOSE action frame from the disconnecting peer, 
> > flush all entries in the path table which has this peer as the next hop.
> 
> Please address the user-visible behavior in your commit messages.
> Does it crash?  Does it send frames to an invalid peer?  Do frames get 
> dropped?
> 

Hi Bob,
It was a crash, apparently already fixed by your patches some time ago.
I'll remove that part and resend the 2nd part (with some more 'why', and less 
typos..)

> > In addition, upon receiving a packet, if next hop is not found, 
> > trigger PERQ immidiatly, instead of just putting it in the queue.
> 
> "PREQ"
> 
> Please split this into a separate patch that we can review separately 
> (and also give the "why" in the commit log).
> 
> > @@ -1011,6 +1011,7 @@ static void sta_apply_mesh_params(struct
> ieee80211_local *local,
> > if (sta->mesh->plink_state == NL80211_PLINK_ESTAB)
> > changed =
> mesh_plink_dec_estab_count(sdata);
> > sta->mesh->plink_state = params->plink_state;
> > +   mesh_path_flush_by_nexthop(sta);
> 
> This isn't necessary, caller should already be doing
> mesh_path_flush_by_nexthop() in every case I could see.  Besides it 
> cannot be done under plink lock.
> 

I believe this was fixed in your patch "mac80211: mesh: flush paths outside of 
plink lock"
There is probably no need in that on the latest as well.

Thanks,
Yaniv
 

Re: [PATCH 2/4] mac80211/cfg: mesh: fix healing time when a mesh peer is disconnecting

[Bob Copeland]

On Tue, Jun 28, 2016 at 02:13:05PM +0300, Yaniv Machani wrote:
> From: Maital Hahn 
> 
> Once receiving a CLOSE action frame from the disconnecting peer,
> flush all entries in the path table which has this peer as the
> next hop.

Please address the user-visible behavior in your commit messages.
Does it crash?  Does it send frames to an invalid peer?  Do
frames get dropped?

> In addition, upon receiving a packet, if next hop is not found,
> trigger PERQ immidiatly, instead of just putting it in the queue.

"PREQ"

Please split this into a separate patch that we can review
separately (and also give the "why" in the commit log).

> Signed-off-by: Maital Hahn 
> Acked-by: Yaniv Machani 
> ---
>  net/mac80211/cfg.c   |  1 +
>  net/mac80211/mesh.c  |  3 ++-
>  net/mac80211/mesh_hwmp.c | 42 +-
>  3 files changed, 28 insertions(+), 18 deletions(-)
> 
> diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
> index 0c12e40..f876ef7 100644
> --- a/net/mac80211/cfg.c
> +++ b/net/mac80211/cfg.c
> @@ -1011,6 +1011,7 @@ static void sta_apply_mesh_params(struct 
> ieee80211_local *local,
>   if (sta->mesh->plink_state == NL80211_PLINK_ESTAB)
>   changed = mesh_plink_dec_estab_count(sdata);
>   sta->mesh->plink_state = params->plink_state;
> + mesh_path_flush_by_nexthop(sta);

This isn't necessary, caller should already be doing
mesh_path_flush_by_nexthop() in every case I could see.  Besides it
cannot be done under plink lock.

> +++ b/net/mac80211/mesh.c
> @@ -159,7 +159,8 @@ void mesh_sta_cleanup(struct sta_info *sta)
>   if (!sdata->u.mesh.user_mpm) {
>   changed |= mesh_plink_deactivate(sta);
>   del_timer_sync(>mesh->plink_timer);
> - }
> + } else
> + mesh_path_flush_by_nexthop(sta);

And this is already fixed in mac80211-next.

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

[PATCH 2/4] mac80211/cfg: mesh: fix healing time when a mesh peer is disconnecting

[Yaniv Machani]

From: Maital Hahn 

Once receiving a CLOSE action frame from the disconnecting peer,
flush all entries in the path table which has this peer as the
next hop.

In addition, upon receiving a packet, if next hop is not found,
trigger PERQ immidiatly, instead of just putting it in the queue.

Signed-off-by: Maital Hahn 
Acked-by: Yaniv Machani 
---
 net/mac80211/cfg.c   |  1 +
 net/mac80211/mesh.c  |  3 ++-
 net/mac80211/mesh_hwmp.c | 42 +-
 3 files changed, 28 insertions(+), 18 deletions(-)

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 0c12e40..f876ef7 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -1011,6 +1011,7 @@ static void sta_apply_mesh_params(struct ieee80211_local 
*local,
if (sta->mesh->plink_state == NL80211_PLINK_ESTAB)
changed = mesh_plink_dec_estab_count(sdata);
sta->mesh->plink_state = params->plink_state;
+   mesh_path_flush_by_nexthop(sta);
 
ieee80211_mps_sta_status_update(sta);
changed |= ieee80211_mps_set_sta_local_pm(sta,
diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
index 9214bc1..1f5be54 100644
--- a/net/mac80211/mesh.c
+++ b/net/mac80211/mesh.c
@@ -159,7 +159,8 @@ void mesh_sta_cleanup(struct sta_info *sta)
if (!sdata->u.mesh.user_mpm) {
changed |= mesh_plink_deactivate(sta);
del_timer_sync(>mesh->plink_timer);
-   }
+   } else
+   mesh_path_flush_by_nexthop(sta);
 
/* make sure no readers can access nexthop sta from here on */
mesh_path_flush_by_nexthop(sta);
diff --git a/net/mac80211/mesh_hwmp.c b/net/mac80211/mesh_hwmp.c
index 8f9c3bd..9783d49 100644
--- a/net/mac80211/mesh_hwmp.c
+++ b/net/mac80211/mesh_hwmp.c
@@ -19,7 +19,7 @@
 
 #define MAX_PREQ_QUEUE_LEN 64
 
-static void mesh_queue_preq(struct mesh_path *, u8);
+static void mesh_queue_preq(struct mesh_path *, u8, bool);
 
 static inline u32 u32_field_get(const u8 *preq_elem, int offset, bool ae)
 {
@@ -830,7 +830,8 @@ static void hwmp_rann_frame_process(struct 
ieee80211_sub_if_data *sdata,
mhwmp_dbg(sdata,
  "time to refresh root mpath %pM\n",
  orig_addr);
-   mesh_queue_preq(mpath, PREQ_Q_F_START | PREQ_Q_F_REFRESH);
+   mesh_queue_preq(mpath, PREQ_Q_F_START | PREQ_Q_F_REFRESH,
+   false);
mpath->last_preq_to_root = jiffies;
}
 
@@ -925,7 +926,7 @@ void mesh_rx_path_sel_frame(struct ieee80211_sub_if_data 
*sdata,
  * Locking: the function must be called from within a rcu read lock block.
  *
  */
-static void mesh_queue_preq(struct mesh_path *mpath, u8 flags)
+static void mesh_queue_preq(struct mesh_path *mpath, u8 flags, bool immediate)
 {
struct ieee80211_sub_if_data *sdata = mpath->sdata;
struct ieee80211_if_mesh *ifmsh = >u.mesh;
@@ -964,18 +965,24 @@ static void mesh_queue_preq(struct mesh_path *mpath, u8 
flags)
++ifmsh->preq_queue_len;
spin_unlock_bh(>mesh_preq_queue_lock);
 
-   if (time_after(jiffies, ifmsh->last_preq + min_preq_int_jiff(sdata)))
+   if (immediate) {
ieee80211_queue_work(>local->hw, >work);
+   } else {
+   if (time_after(jiffies,
+  ifmsh->last_preq + min_preq_int_jiff(sdata))) {
+   ieee80211_queue_work(>local->hw, >work);
 
-   else if (time_before(jiffies, ifmsh->last_preq)) {
-   /* avoid long wait if did not send preqs for a long time
-* and jiffies wrapped around
-*/
-   ifmsh->last_preq = jiffies - min_preq_int_jiff(sdata) - 1;
-   ieee80211_queue_work(>local->hw, >work);
-   } else
-   mod_timer(>mesh_path_timer, ifmsh->last_preq +
-   min_preq_int_jiff(sdata));
+   } else if (time_before(jiffies, ifmsh->last_preq)) {
+   /* avoid long wait if did not send preqs for a long time
+* and jiffies wrapped around
+*/
+   ifmsh->last_preq = jiffies -
+  min_preq_int_jiff(sdata) - 1;
+   ieee80211_queue_work(>local->hw, >work);
+   } else
+   mod_timer(>mesh_path_timer, ifmsh->last_preq +
+ min_preq_int_jiff(sdata));
+   }
 }
 
 /**
@@ -1110,7 +1117,7 @@ int mesh_nexthop_resolve(struct ieee80211_sub_if_data 
*sdata,
}
 
if (!(mpath->flags & MESH_PATH_RESOLVING))
-   mesh_queue_preq(mpath, PREQ_Q_F_START);
+   mesh_queue_preq(mpath, PREQ_Q_F_START, true);
 
if (skb_queue_len(>frame_queue)