Re: [PATCH v2] mac80211: fix the RANN propagation issues

Re: [PATCH v2] mac80211: fix the RANN propagation issues

[Javier Cardona]

On Thu, Mar 22, 2012 at 5:56 AM, Chun-Yeow Yeoh <yeohchunyeow@gmail.com> wrote:
> This patch is intended to solve the follwing issues in RANN propagation:
> [1] The interval in propagated RANN should be based on the interval of received RANN.
> [2] The aggregated path metric for propagated RANN is as received plus own link metric
>    towards the transmitting mesh STA (not root mesh STA).
> [3] The comparison of path metric for RANN with same sequence number should be done
>    before deciding whether to propagate or not.
>
> Signed-off-by: Chun-Yeow Yeoh <yeohchunyeow@gmail.com>
Acked-by: Javier Cardona <javier@cozybit.com>


> ---
> v2:
> revert back to plain conditionals (Javier and Thomas)
>
>  net/mac80211/mesh.h      |    2 ++
>  net/mac80211/mesh_hwmp.c |   20 ++++++++++++++++----
>  2 files changed, 18 insertions(+), 4 deletions(-)
>
> diff --git a/net/mac80211/mesh.h b/net/mac80211/mesh.h
> index 8d53b71..adf1be3 100644
> --- a/net/mac80211/mesh.h
> +++ b/net/mac80211/mesh.h
> @@ -86,6 +86,7 @@ enum mesh_deferred_task_flags {
>  * mpath itself.  No need to take this lock when adding or removing
>  * an mpath to a hash bucket on a path table.
>  * @rann_snd_addr: the RANN sender address
> + * @rann_metric: the aggregated path metric towards the root node
>  * @is_root: the destination station of this path is a root node
>  * @is_gate: the destination station of this path is a mesh gate
>  *
> @@ -112,6 +113,7 @@ struct mesh_path {
>        enum mesh_path_flags flags;
>        spinlock_t state_lock;
>        u8 rann_snd_addr[ETH_ALEN];
> +       u32 rann_metric;
>        bool is_root;
>        bool is_gate;
>  };
> diff --git a/net/mac80211/mesh_hwmp.c b/net/mac80211/mesh_hwmp.c
> index f80a9e3..14bb1bd 100644
> --- a/net/mac80211/mesh_hwmp.c
> +++ b/net/mac80211/mesh_hwmp.c
> @@ -732,11 +732,12 @@ static void hwmp_rann_frame_process(struct ieee80211_sub_if_data *sdata,
>                                struct ieee80211_rann_ie *rann)
>  {
>        struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
> +       struct ieee80211_local *local = sdata->local;
> +       struct sta_info *sta;
>        struct mesh_path *mpath;
>        u8 ttl, flags, hopcount;
>        u8 *orig_addr;
> -       u32 orig_sn, metric;
> -       u32 interval = ifmsh->mshcfg.dot11MeshHWMPRannInterval;
> +       u32 orig_sn, metric, metric_txsta, interval;
>        bool root_is_gate;
>
>        ttl = rann->rann_ttl;
> @@ -749,6 +750,7 @@ static void hwmp_rann_frame_process(struct ieee80211_sub_if_data *sdata,
>        root_is_gate = !!(flags & RANN_FLAG_IS_GATE);
>        orig_addr = rann->rann_addr;
>        orig_sn = le32_to_cpu(rann->rann_seq);
> +       interval = le32_to_cpu(rann->rann_interval);
>        hopcount = rann->rann_hopcount;
>        hopcount++;
>        metric = le32_to_cpu(rann->rann_metric);
> @@ -761,6 +763,14 @@ static void hwmp_rann_frame_process(struct ieee80211_sub_if_data *sdata,
>                        orig_addr, mgmt->sa, root_is_gate);
>
>        rcu_read_lock();
> +       sta = sta_info_get(sdata, mgmt->sa);
> +       if (!sta) {
> +               rcu_read_unlock();
> +               return;
> +       }
> +
> +       metric_txsta = airtime_link_metric_get(local, sta);
> +
>        mpath = mesh_path_lookup(orig_addr, sdata);
>        if (!mpath) {
>                mesh_path_add(orig_addr, sdata);
> @@ -780,14 +790,16 @@ static void hwmp_rann_frame_process(struct ieee80211_sub_if_data *sdata,
>                mesh_queue_preq(mpath, PREQ_Q_F_START | PREQ_Q_F_REFRESH);
>        }
>
> -       if (mpath->sn < orig_sn && ifmsh->mshcfg.dot11MeshForwarding) {
> +       if ((mpath->sn < orig_sn || (mpath->sn == orig_sn &&
> +          metric < mpath->rann_metric)) && ifmsh->mshcfg.dot11MeshForwarding) {
>                mesh_path_sel_frame_tx(MPATH_RANN, flags, orig_addr,
>                                       cpu_to_le32(orig_sn),
>                                       0, NULL, 0, broadcast_addr,
>                                       hopcount, ttl, cpu_to_le32(interval),
> -                                      cpu_to_le32(metric + mpath->metric),
> +                                      cpu_to_le32(metric + metric_txsta),
>                                       0, sdata);
>                mpath->sn = orig_sn;
> +               mpath->rann_metric = metric + metric_txsta;
>        }
>
>        /* Using individually addressed PREQ for root node */
> --
> 1.7.0.4
>



-- 
Javier Cardona
cozybit Inc.
http://www.cozybit.com

Re: [PATCH v2] mac80211: fix the RANN propagation issues

[Marek Lindner]

Hi,

I am not familiar with the 802.11s code and could be totally wrong here but ..


On Thursday, March 22, 2012 13:56:08 Chun-Yeow Yeoh wrote:
> +       if ((mpath->sn < orig_sn || (mpath->sn == orig_sn &&

.. this sequence number handling looks broken to me. What happens when the 
sequence number wraps around ? One could be smaller than the other and still 
be newer.
I know this isn't really what your patch changes.


>  #define SN_GT(x, y) ((long) (y) - (long) (x) < 0)
>  #define SN_LT(x, y) ((long) (x) - (long) (y) < 0)

Your macros tried to address the problem but casting your sequence number to 
long also breaks the wrap around.

Regards,
Marek

[PATCH v2] mac80211: fix the RANN propagation issues

[Chun-Yeow Yeoh]

This patch is intended to solve the follwing issues in RANN propagation:
[1] The interval in propagated RANN should be based on the interval of received RANN.
[2] The aggregated path metric for propagated RANN is as received plus own link metric
    towards the transmitting mesh STA (not root mesh STA).
[3] The comparison of path metric for RANN with same sequence number should be done
    before deciding whether to propagate or not.

Signed-off-by: Chun-Yeow Yeoh <yeohchunyeow@gmail.com>
---
v2:
revert back to plain conditionals (Javier and Thomas)

 net/mac80211/mesh.h      |    2 ++
 net/mac80211/mesh_hwmp.c |   20 ++++++++++++++++----
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/net/mac80211/mesh.h b/net/mac80211/mesh.h
index 8d53b71..adf1be3 100644
--- a/net/mac80211/mesh.h
+++ b/net/mac80211/mesh.h
@@ -86,6 +86,7 @@ enum mesh_deferred_task_flags {
  * mpath itself.  No need to take this lock when adding or removing
  * an mpath to a hash bucket on a path table.
  * @rann_snd_addr: the RANN sender address
+ * @rann_metric: the aggregated path metric towards the root node
  * @is_root: the destination station of this path is a root node
  * @is_gate: the destination station of this path is a mesh gate
  *
@@ -112,6 +113,7 @@ struct mesh_path {
 	enum mesh_path_flags flags;
 	spinlock_t state_lock;
 	u8 rann_snd_addr[ETH_ALEN];
+	u32 rann_metric;
 	bool is_root;
 	bool is_gate;
 };
diff --git a/net/mac80211/mesh_hwmp.c b/net/mac80211/mesh_hwmp.c
index f80a9e3..14bb1bd 100644
--- a/net/mac80211/mesh_hwmp.c
+++ b/net/mac80211/mesh_hwmp.c
@@ -732,11 +732,12 @@ static void hwmp_rann_frame_process(struct ieee80211_sub_if_data *sdata,
 				struct ieee80211_rann_ie *rann)
 {
 	struct ieee80211_if_mesh *ifmsh = &sdata->u.mesh;
+	struct ieee80211_local *local = sdata->local;
+	struct sta_info *sta;
 	struct mesh_path *mpath;
 	u8 ttl, flags, hopcount;
 	u8 *orig_addr;
-	u32 orig_sn, metric;
-	u32 interval = ifmsh->mshcfg.dot11MeshHWMPRannInterval;
+	u32 orig_sn, metric, metric_txsta, interval;
 	bool root_is_gate;
 
 	ttl = rann->rann_ttl;
@@ -749,6 +750,7 @@ static void hwmp_rann_frame_process(struct ieee80211_sub_if_data *sdata,
 	root_is_gate = !!(flags & RANN_FLAG_IS_GATE);
 	orig_addr = rann->rann_addr;
 	orig_sn = le32_to_cpu(rann->rann_seq);
+	interval = le32_to_cpu(rann->rann_interval);
 	hopcount = rann->rann_hopcount;
 	hopcount++;
 	metric = le32_to_cpu(rann->rann_metric);
@@ -761,6 +763,14 @@ static void hwmp_rann_frame_process(struct ieee80211_sub_if_data *sdata,
 			orig_addr, mgmt->sa, root_is_gate);
 
 	rcu_read_lock();
+	sta = sta_info_get(sdata, mgmt->sa);
+	if (!sta) {
+		rcu_read_unlock();
+		return;
+	}
+
+	metric_txsta = airtime_link_metric_get(local, sta);
+
 	mpath = mesh_path_lookup(orig_addr, sdata);
 	if (!mpath) {
 		mesh_path_add(orig_addr, sdata);
@@ -780,14 +790,16 @@ static void hwmp_rann_frame_process(struct ieee80211_sub_if_data *sdata,
 		mesh_queue_preq(mpath, PREQ_Q_F_START | PREQ_Q_F_REFRESH);
 	}
 
-	if (mpath->sn < orig_sn && ifmsh->mshcfg.dot11MeshForwarding) {
+	if ((mpath->sn < orig_sn || (mpath->sn == orig_sn &&
+	   metric < mpath->rann_metric)) && ifmsh->mshcfg.dot11MeshForwarding) {
 		mesh_path_sel_frame_tx(MPATH_RANN, flags, orig_addr,
 				       cpu_to_le32(orig_sn),
 				       0, NULL, 0, broadcast_addr,
 				       hopcount, ttl, cpu_to_le32(interval),
-				       cpu_to_le32(metric + mpath->metric),
+				       cpu_to_le32(metric + metric_txsta),
 				       0, sdata);
 		mpath->sn = orig_sn;
+		mpath->rann_metric = metric + metric_txsta;
 	}
 
 	/* Using individually addressed PREQ for root node */
-- 
1.7.0.4

Re: [PATCH v2] mac80211: fix the RANN propagation issues

[Javier Cardona]

Hi Marek,

On Thu, Mar 22, 2012 at 3:33 AM, Marek Lindner <lindner_marek@yahoo.de> wrote:
>
> Hi,
>
> I am not familiar with the 802.11s code and could be totally wrong here but ..
>
>
> On Thursday, March 22, 2012 13:56:08 Chun-Yeow Yeoh wrote:
>> +       if ((mpath->sn < orig_sn || (mpath->sn == orig_sn &&
>
> .. this sequence number handling looks broken to me. What happens when the
> sequence number wraps around ? One could be smaller than the other and still
> be newer.
> I know this isn't really what your patch changes.
>
>
>>  #define SN_GT(x, y) ((long) (y) - (long) (x) < 0)
>>  #define SN_LT(x, y) ((long) (x) - (long) (y) < 0)
>
> Your macros tried to address the problem but casting your sequence number to
> long also breaks the wrap around.

Ah, thanks for your help.  I guess we do need those macros after all
but they'd have to be re-written as

#define SN_LT(x, y) ((s32)(x - y) < 0)
#define SN_GT(x, y) ((s32)(x - y) > 0)

And since this is just to handle wrap arounds, there is no need for
the new SN_EQ that was suggested earlier.

Cheers,

Javier

Re: [PATCH v2] mac80211: fix the RANN propagation issues

[Marek Lindner]

On Thursday, March 22, 2012 17:31:33 Javier Cardona wrote:
> >>  #define SN_GT(x, y) ((long) (y) - (long) (x) < 0)
> >>  #define SN_LT(x, y) ((long) (x) - (long) (y) < 0)
> > 
> > Your macros tried to address the problem but casting your sequence number
> > to long also breaks the wrap around.
> 
> Ah, thanks for your help.  I guess we do need those macros after all
> but they'd have to be re-written as
> 
> #define SN_LT(x, y) ((s32)(x - y) < 0)
> #define SN_GT(x, y) ((s32)(x - y) > 0)

No, you need unsigned values for this arithmetic to work (which is why long 
also fails).


> And since this is just to handle wrap arounds, there is no need for
> the new SN_EQ that was suggested earlier.

Sounds reasonable to me.

Regards,
Marek

Re: [PATCH v2] mac80211: fix the RANN propagation issues

[Javier Cardona]

On Thu, Mar 22, 2012 at 11:58 AM, Marek Lindner <lindner_marek@yahoo.de> wrote:
> On Thursday, March 22, 2012 17:31:33 Javier Cardona wrote:
>> >>  #define SN_GT(x, y) ((long) (y) - (long) (x) < 0)
>> >>  #define SN_LT(x, y) ((long) (x) - (long) (y) < 0)
>> >
>> > Your macros tried to address the problem but casting your sequence number
>> > to long also breaks the wrap around.
>>
>> Ah, thanks for your help.  I guess we do need those macros after all
>> but they'd have to be re-written as
>>
>> #define SN_LT(x, y) ((s32)(x - y) < 0)
>> #define SN_GT(x, y) ((s32)(x - y) > 0)
>
> No, you need unsigned values for this arithmetic to work (which is why long
> also fails).

The macro is always used on u32 values, so the arithmetic would be
unsigned in this case.

Cheers,

Javier

Re: [PATCH v2] mac80211: fix the RANN propagation issues

[Marek Lindner]

On Thursday, March 22, 2012 20:13:28 Javier Cardona wrote:
> On Thu, Mar 22, 2012 at 11:58 AM, Marek Lindner <lindner_marek@yahoo.de> 
wrote:
> > On Thursday, March 22, 2012 17:31:33 Javier Cardona wrote:
> >> >>  #define SN_GT(x, y) ((long) (y) - (long) (x) < 0)
> >> >>  #define SN_LT(x, y) ((long) (x) - (long) (y) < 0)
> >> > 
> >> > Your macros tried to address the problem but casting your sequence
> >> > number to long also breaks the wrap around.
> >> 
> >> Ah, thanks for your help.  I guess we do need those macros after all
> >> but they'd have to be re-written as
> >> 
> >> #define SN_LT(x, y) ((s32)(x - y) < 0)
> >> #define SN_GT(x, y) ((s32)(x - y) > 0)
> > 
> > No, you need unsigned values for this arithmetic to work (which is why
> > long also fails).
> 
> The macro is always used on u32 values, so the arithmetic would be
> unsigned in this case.

Ok - then you should be on the safe side.

Regards,
Marek

Re: [PATCH v2] mac80211: fix the RANN propagation issues

[Yeoh Chun-Yeow]

Hi, Marek

Thanks. Appreciate your comments. HWMP sequence number is 32bits or 4
octets. That's why it is defined as u32.

Hi, Javier

Thanks for your explanation and clarification.

I will resubmit the patch for the changes.

"In the section 11C.9.8.3 HWMP sequence numbering, Comparing HWMP
sequence numbers is done using a circular modulo 232 comparison."

Regards,
Chun-Yeow

Re: [PATCH v2] mac80211: fix the RANN propagation issues

[Javier Cardona]

On Thu, Mar 22, 2012 at 5:47 PM, Yeoh Chun-Yeow <yeohchunyeow@gmail.com> wrote:
> "In the section 11C.9.8.3 HWMP sequence numbering, Comparing HWMP
> sequence numbers is done using a circular modulo 232 comparison."

That would be section 13.10.8.3 in the ratified version of the
standard.  And it's modulo 2³² (2^32), not 232 :)

Cheers,

Javier