Re: [PATCH v2 2/3] mac80211: mesh: improve path resolving time

[Johannes Berg]

On Tue, 2016-07-19 at 08:36 -0400, Bob Copeland wrote:
> On Wed, Jul 13, 2016 at 02:45:25PM +0300, Yaniv Machani wrote:
> > 
> > When a packet is received for transmission,
> > a PREQ frame is sent to resolve the appropriate path to the desired
> > destination.
> > After path was established, any sequential PREQ will be sent only
> > after
> > dot11MeshHWMPpreqMinInterval, which usually set to few seconds.
> > 
> > This implementation has an impact in cases where we would like to
> > resolve the path quickly.
> > A clear example is when a peer was disconnected from us,
> > while he acted as a hop to our destination.
> > Although the path table will be cleared, the next PREQ frame will
> > be sent only after reaching the MinInterval.
> > This will cause unwanted delay, possibly of few seconds until the
> > traffic will resume.
> > 
> >     if (!(mpath->flags & MESH_PATH_RESOLVING))
> > -   mesh_queue_preq(mpath, PREQ_Q_F_START);
> > +   mesh_queue_preq(mpath, PREQ_Q_F_START, true);
> 
> What about something like this here instead:
> 
> if (!(mpath->flags & MESH_PATH_RESOLVING)) {
> /* force next preq to be sent without delay */
> ifmsh->last_preq = jiffies - min_preq_int_jiff(sdata) - 1;
> mesh_queue_preq(mpath, PREQ_Q_F_START);
> }
> 

Yaniv, did you disagree with this for some strong reason, or were you
going to resend?

Having a smaller patch seems nicer too.

johannes

Re: [PATCH v2 2/3] mac80211: mesh: improve path resolving time

[Johannes Berg]

On Tue, 2016-07-19 at 08:36 -0400, Bob Copeland wrote:
> On Wed, Jul 13, 2016 at 02:45:25PM +0300, Yaniv Machani wrote:
> > 
> > When a packet is received for transmission,
> > a PREQ frame is sent to resolve the appropriate path to the desired
> > destination.
> > After path was established, any sequential PREQ will be sent only
> > after
> > dot11MeshHWMPpreqMinInterval, which usually set to few seconds.
> > 
> > This implementation has an impact in cases where we would like to
> > resolve the path quickly.
> > A clear example is when a peer was disconnected from us,
> > while he acted as a hop to our destination.
> > Although the path table will be cleared, the next PREQ frame will
> > be sent only after reaching the MinInterval.
> > This will cause unwanted delay, possibly of few seconds until the
> > traffic will resume.
> > 
> >     if (!(mpath->flags & MESH_PATH_RESOLVING))
> > -   mesh_queue_preq(mpath, PREQ_Q_F_START);
> > +   mesh_queue_preq(mpath, PREQ_Q_F_START, true);
> 
> What about something like this here instead:
> 
> if (!(mpath->flags & MESH_PATH_RESOLVING)) {
> /* force next preq to be sent without delay */
> ifmsh->last_preq = jiffies - min_preq_int_jiff(sdata) - 1;
> mesh_queue_preq(mpath, PREQ_Q_F_START);
> }
> 

Yaniv, did you disagree with this for some strong reason, or were you
going to resend?

Having a smaller patch seems nicer too.

johannes

Re: [PATCH v2 2/3] mac80211: mesh: improve path resolving time

[Bob Copeland]

On Wed, Jul 13, 2016 at 02:45:25PM +0300, Yaniv Machani wrote:
> When a packet is received for transmission,
> a PREQ frame is sent to resolve the appropriate path to the desired 
> destination.
> After path was established, any sequential PREQ will be sent only after
> dot11MeshHWMPpreqMinInterval, which usually set to few seconds.
> 
> This implementation has an impact in cases where we would like to
> resolve the path quickly.
> A clear example is when a peer was disconnected from us,
> while he acted as a hop to our destination.
> Although the path table will be cleared, the next PREQ frame will be sent 
> only after reaching the MinInterval.
> This will cause unwanted delay, possibly of few seconds until the traffic 
> will resume.
> 
>   if (!(mpath->flags & MESH_PATH_RESOLVING))
> - mesh_queue_preq(mpath, PREQ_Q_F_START);
> + mesh_queue_preq(mpath, PREQ_Q_F_START, true);

What about something like this here instead:

if (!(mpath->flags & MESH_PATH_RESOLVING)) {
/* force next preq to be sent without delay */
ifmsh->last_preq = jiffies - min_preq_int_jiff(sdata) - 1;
mesh_queue_preq(mpath, PREQ_Q_F_START);
}

Maybe a little more magic, but it has a comment explaining it and doesn't
add a bool parameter everywhere.  Or, maybe even just do it inside
mesh_queue_preq() based on having PREQ_Q_F_START && !PREQ_Q_F_REFRESH (if
those are the only cases where "true" is passed).

Generally I try to avoid bool parameters where possible because when you
look at a callsite, you don't know immediately what "true" and "false"
mean, and also each one you add doubles the code paths through a given
function.

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

Re: [PATCH v2 2/3] mac80211: mesh: improve path resolving time

[Bob Copeland]

On Wed, Jul 13, 2016 at 02:45:25PM +0300, Yaniv Machani wrote:
> When a packet is received for transmission,
> a PREQ frame is sent to resolve the appropriate path to the desired 
> destination.
> After path was established, any sequential PREQ will be sent only after
> dot11MeshHWMPpreqMinInterval, which usually set to few seconds.
> 
> This implementation has an impact in cases where we would like to
> resolve the path quickly.
> A clear example is when a peer was disconnected from us,
> while he acted as a hop to our destination.
> Although the path table will be cleared, the next PREQ frame will be sent 
> only after reaching the MinInterval.
> This will cause unwanted delay, possibly of few seconds until the traffic 
> will resume.
> 
>   if (!(mpath->flags & MESH_PATH_RESOLVING))
> - mesh_queue_preq(mpath, PREQ_Q_F_START);
> + mesh_queue_preq(mpath, PREQ_Q_F_START, true);

What about something like this here instead:

if (!(mpath->flags & MESH_PATH_RESOLVING)) {
/* force next preq to be sent without delay */
ifmsh->last_preq = jiffies - min_preq_int_jiff(sdata) - 1;
mesh_queue_preq(mpath, PREQ_Q_F_START);
}

Maybe a little more magic, but it has a comment explaining it and doesn't
add a bool parameter everywhere.  Or, maybe even just do it inside
mesh_queue_preq() based on having PREQ_Q_F_START && !PREQ_Q_F_REFRESH (if
those are the only cases where "true" is passed).

Generally I try to avoid bool parameters where possible because when you
look at a callsite, you don't know immediately what "true" and "false"
mean, and also each one you add doubles the code paths through a given
function.

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

[PATCH v2 2/3] mac80211: mesh: improve path resolving time

[Yaniv Machani]

When a packet is received for transmission,
a PREQ frame is sent to resolve the appropriate path to the desired destination.
After path was established, any sequential PREQ will be sent only after
dot11MeshHWMPpreqMinInterval, which usually set to few seconds.

This implementation has an impact in cases where we would like to
resolve the path quickly.
A clear example is when a peer was disconnected from us,
while he acted as a hop to our destination.
Although the path table will be cleared, the next PREQ frame will be sent only 
after reaching the MinInterval.
This will cause unwanted delay, possibly of few seconds until the traffic will 
resume.

To improve that, added an 'immediate' flag to be used when the path needs to be 
resolved.
Once set, a PREQ frame will be send w/o considering the MinInterval parameter.

Signed-off-by: Maital Hahn 
Signed-off-by: Yaniv Machani 
---
v2 - Updated comment to explain the scenario better.
   - Removed unnccesary changes that was alreay upstreamed.
   
 net/mac80211/mesh_hwmp.c | 42 +-
 1 file changed, 25 insertions(+), 17 deletions(-)

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) >= MESH_FRAME_QUEUE_LEN)
skb_to_free = skb_dequeue(>frame_queue);
@@ -1157,8 +1164,9 @@ int mesh_nexthop_lookup(struct ieee80211_sub_if_data 
*sdata,
   
msecs_to_jiffies(sdata->u.mesh.mshcfg.path_refresh_time)) &&
ether_addr_equal(sdata->vif.addr, hdr->addr4) &&
!(mpath->flags & MESH_PATH_RESOLVING) &&
-   !(mpath->flags & MESH_PATH_FIXED))
-   mesh_queue_preq(mpath, PREQ_Q_F_START | PREQ_Q_F_REFRESH);
+   !(mpath->flags & MESH_PATH_FIXED)) {
+   mesh_queue_preq(mpath, PREQ_Q_F_START | 

[PATCH v2 2/3] mac80211: mesh: improve path resolving time

[Yaniv Machani]

When a packet is received for transmission,
a PREQ frame is sent to resolve the appropriate path to the desired destination.
After path was established, any sequential PREQ will be sent only after
dot11MeshHWMPpreqMinInterval, which usually set to few seconds.

This implementation has an impact in cases where we would like to
resolve the path quickly.
A clear example is when a peer was disconnected from us,
while he acted as a hop to our destination.
Although the path table will be cleared, the next PREQ frame will be sent only 
after reaching the MinInterval.
This will cause unwanted delay, possibly of few seconds until the traffic will 
resume.

To improve that, added an 'immediate' flag to be used when the path needs to be 
resolved.
Once set, a PREQ frame will be send w/o considering the MinInterval parameter.

Signed-off-by: Maital Hahn 
Signed-off-by: Yaniv Machani 
---
v2 - Updated comment to explain the scenario better.
   - Removed unnccesary changes that was alreay upstreamed.
   
 net/mac80211/mesh_hwmp.c | 42 +-
 1 file changed, 25 insertions(+), 17 deletions(-)

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) >= MESH_FRAME_QUEUE_LEN)
skb_to_free = skb_dequeue(>frame_queue);
@@ -1157,8 +1164,9 @@ int mesh_nexthop_lookup(struct ieee80211_sub_if_data 
*sdata,
   
msecs_to_jiffies(sdata->u.mesh.mshcfg.path_refresh_time)) &&
ether_addr_equal(sdata->vif.addr, hdr->addr4) &&
!(mpath->flags & MESH_PATH_RESOLVING) &&
-   !(mpath->flags & MESH_PATH_FIXED))
-   mesh_queue_preq(mpath, PREQ_Q_F_START | PREQ_Q_F_REFRESH);
+   !(mpath->flags & MESH_PATH_FIXED)) {
+   mesh_queue_preq(mpath, PREQ_Q_F_START | PREQ_Q_F_REFRESH, false);
+   }