Message ID | 20160713114528.24835-1-yanivma@ti.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Johannes Berg |
Headers | show |
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.
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 -- To unsubscribe from this list: send the line "unsubscribe linux-wireless" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 = &sdata->u.mesh; @@ -964,18 +965,24 @@ static void mesh_queue_preq(struct mesh_path *mpath, u8 flags) ++ifmsh->preq_queue_len; spin_unlock_bh(&ifmsh->mesh_preq_queue_lock); - if (time_after(jiffies, ifmsh->last_preq + min_preq_int_jiff(sdata))) + if (immediate) { ieee80211_queue_work(&sdata->local->hw, &sdata->work); + } else { + if (time_after(jiffies, + ifmsh->last_preq + min_preq_int_jiff(sdata))) { + ieee80211_queue_work(&sdata->local->hw, &sdata->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(&sdata->local->hw, &sdata->work); - } else - mod_timer(&ifmsh->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(&sdata->local->hw, &sdata->work); + } else + mod_timer(&ifmsh->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(&mpath->frame_queue) >= MESH_FRAME_QUEUE_LEN) skb_to_free = skb_dequeue(&mpath->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); + } next_hop = rcu_dereference(mpath->next_hop); if (next_hop) { @@ -1192,7 +1200,7 @@ void mesh_path_timer(unsigned long data) mpath->discovery_timeout *= 2; mpath->flags &= ~MESH_PATH_REQ_QUEUED; spin_unlock_bh(&mpath->state_lock); - mesh_queue_preq(mpath, 0); + mesh_queue_preq(mpath, 0, false); } else { mpath->flags &= ~(MESH_PATH_RESOLVING | MESH_PATH_RESOLVED |