diff mbox series

mac80211: mesh: use average bitrate for link metric calculation

Message ID 20190115233156.225963-1-julanhsu@google.com (mailing list archive)
State Accepted
Delegated to: Johannes Berg
Headers show
Series mac80211: mesh: use average bitrate for link metric calculation | expand

Commit Message

julanhsu@google.com Jan. 15, 2019, 11:31 p.m. UTC
From: Julan Hsu <julanhsu@google.com>

Use bitrate moving average to smooth out link metric and stablize path
selection.

Signed-off-by: Julan Hsu <julanhsu@google.com>
---
 net/mac80211/cfg.c       | 5 +++++
 net/mac80211/mesh_hwmp.c | 9 ++++++---
 net/mac80211/sta_info.h  | 4 ++++
 3 files changed, 15 insertions(+), 3 deletions(-)

Comments

Benjamin Beichler Jan. 22, 2019, 10:18 a.m. UTC | #1
Hi,
Am 16.01.2019 um 00:31 schrieb julanhsu@google.com:
> From: Julan Hsu <julanhsu@google.com>
>
> Use bitrate moving average to smooth out link metric and stablize path
> selection.
We had some investigations on the same field, but I didn't had time to
publish anything of that. The general idea is good and we implemented it
similar.
One Issue is the different rate of multi cast and management frames in
contrast to uni cast data frames. AFAIK, the rate for management frames,
especially the beacon frames of mesh, are always sent with a low
mandatory rates (although they are configurable). If the link is less
used, the beacon frames may slowly degrade the average.
But anyhow, this problem also exist in the current implementation. The
other problem is, as you may expected already, the actual used
information for the metrics are highly driver dependent.
So e.g. the ath9k driver had some issue in the past, were the update
metric function was not called for some runtime modes (no-SKB vs SKB
ect.). Or the update_metric function is not called for all kind of frames.

But in general I think it is a first good step for better metrics.
Benjamin Beichler Jan. 28, 2019, 4:47 p.m. UTC | #2
Altough the patch is already accepted, I have a question regarding the
current semantics of the whole function airtime_link_metric_get:

My Impression was, that

rate = DIV_ROUND_UP(sta_get_expected_throughput(sta), 100);

will get the expected throughput from the rate control, so e.g. for
ath9k it will receive the already averaged rate from minstrel(_ht).
I don't know which hardware you use, but we have only mesh hardware in
our lab, which use minstrel, so this code wouldn't be triggered.

Unfortunately, we are on a quite old kernel in the lab, where this code
is not present, so I'm not able to proof my assumption.

Nevertheless, my former comment is still valid, as the averaged rate
could be calculated over several kinds of frames (management/multicast)
which have a inherently lower rate.
This could have the inverse effect to slowly change the path metric,
while this patch should stabilize it, isn't it ?
diff mbox series

Patch

diff --git a/net/mac80211/cfg.c b/net/mac80211/cfg.c
index 818aa0060349..31a47d3a122d 100644
--- a/net/mac80211/cfg.c
+++ b/net/mac80211/cfg.c
@@ -1231,6 +1231,11 @@  static void sta_apply_mesh_params(struct ieee80211_local *local,
 			ieee80211_mps_sta_status_update(sta);
 			changed |= ieee80211_mps_set_sta_local_pm(sta,
 				      sdata->u.mesh.mshcfg.power_mode);
+
+			ewma_mesh_tx_rate_avg_init(&sta->mesh->tx_rate_avg);
+			/* init at low value */
+			ewma_mesh_tx_rate_avg_add(&sta->mesh->tx_rate_avg, 10);
+
 			break;
 		case NL80211_PLINK_LISTEN:
 		case NL80211_PLINK_BLOCKED:
diff --git a/net/mac80211/mesh_hwmp.c b/net/mac80211/mesh_hwmp.c
index 6950cd0bf594..34f11be04e64 100644
--- a/net/mac80211/mesh_hwmp.c
+++ b/net/mac80211/mesh_hwmp.c
@@ -300,6 +300,7 @@  void ieee80211s_update_metric(struct ieee80211_local *local,
 {
 	struct ieee80211_tx_info *txinfo = st->info;
 	int failed;
+	struct rate_info rinfo;
 
 	failed = !(txinfo->flags & IEEE80211_TX_STAT_ACK);
 
@@ -310,12 +311,15 @@  void ieee80211s_update_metric(struct ieee80211_local *local,
 	if (ewma_mesh_fail_avg_read(&sta->mesh->fail_avg) >
 			LINK_FAIL_THRESH)
 		mesh_plink_broken(sta);
+
+	sta_set_rate_info_tx(sta, &sta->tx_stats.last_rate, &rinfo);
+	ewma_mesh_tx_rate_avg_add(&sta->mesh->tx_rate_avg,
+				  cfg80211_calculate_bitrate(&rinfo));
 }
 
 static u32 airtime_link_metric_get(struct ieee80211_local *local,
 				   struct sta_info *sta)
 {
-	struct rate_info rinfo;
 	/* This should be adjusted for each device */
 	int device_constant = 1 << ARITH_SHIFT;
 	int test_frame_len = TEST_FRAME_LEN << ARITH_SHIFT;
@@ -339,8 +343,7 @@  static u32 airtime_link_metric_get(struct ieee80211_local *local,
 		if (fail_avg > LINK_FAIL_THRESH)
 			return MAX_METRIC;
 
-		sta_set_rate_info_tx(sta, &sta->tx_stats.last_rate, &rinfo);
-		rate = cfg80211_calculate_bitrate(&rinfo);
+		rate = ewma_mesh_tx_rate_avg_read(&sta->mesh->tx_rate_avg);
 		if (WARN_ON(!rate))
 			return MAX_METRIC;
 
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 9a04327d71d1..b94aeb680d2d 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -343,6 +343,7 @@  struct ieee80211_fast_rx {
 
 /* we use only values in the range 0-100, so pick a large precision */
 DECLARE_EWMA(mesh_fail_avg, 20, 8)
+DECLARE_EWMA(mesh_tx_rate_avg, 8, 16)
 
 /**
  * struct mesh_sta - mesh STA information
@@ -365,6 +366,7 @@  DECLARE_EWMA(mesh_fail_avg, 20, 8)
  * @processed_beacon: set to true after peer rates and capabilities are
  *	processed
  * @fail_avg: moving percentage of failed MSDUs
+ * @tx_rate_avg: moving average of tx bitrate
  */
 struct mesh_sta {
 	struct timer_list plink_timer;
@@ -392,6 +394,8 @@  struct mesh_sta {
 
 	/* moving percentage of failed MSDUs */
 	struct ewma_mesh_fail_avg fail_avg;
+	/* moving average of tx bitrate */
+	struct ewma_mesh_tx_rate_avg tx_rate_avg;
 };
 
 DECLARE_EWMA(signal, 10, 8)