This is merely a historical archive of years 2008-2021, before the migration to mailman3.
A maintained and still updated list archive can be found at https://lists.osmocom.org/hyperkitty/list/gerrit-log@lists.osmocom.org/.
laforge gerrit-no-reply at lists.osmocom.orglaforge has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/21979 )
Change subject: hodec2: fix candidate choices in congestion check
......................................................................
hodec2: fix candidate choices in congestion check
Fix flaws in picking a candidate for congestion resolution, shown in
recently added tests.
- For TCH/H->TCH/F upgrading, do not favor moving to a weaker neighbor
cell.
- When comparing dynamic timeslots on the same cell, favor a dynamic
timeslot that frees an entire dyn TS even though the target rxlev
differs.
Do not separate the passes for inter-cell and intra-cell candidates:
before, the inter-cell pass would already pick a candidate and start
handover, even though the subsequent intra-cell pass would have revealed
a better candidate. Join the intra-cell considerations into
pick_better_lchan_to_move().
The intra-cell pass was separate, because it would find the *weakest*
current rxlev, to give a TCH/H to TCH/F upgrade to the currently weakest
lchan.
Instead of the separate pass for weakest rxlev, in addition to the
target cell's rxlev, also consider the rxlev *change* in
pick_better_lchan_to_move(): For candidates that do not change the rxlev
(usually those that stay in the same cell) and that upgrade to TCH/F,
favor a candidate with weaker current rxlev.
Completely revisit the conditions in pick_better_lchan_to_move() to
yield the desired prioritization of candidate preferences.
In three handover tests, remove the "FAIL" comments and adjust to now
expect the actually desired behavior.
Related: SYS#5032
Change-Id: I2704899c85c35dfd4eba43468452483f40016ca2
---
M src/osmo-bsc/handover_decision_2.c
M tests/handover/test_amr_tch_h_to_f_congestion_two_cells.ho_vty
M tests/handover/test_congestion_intra_vs_inter_cell.ho_vty
M tests/handover/test_dyn_ts_favor_moving_half_used_tch_h.ho_vty
4 files changed, 81 insertions(+), 169 deletions(-)
Approvals:
laforge: Looks good to me, approved
pespin: Looks good to me, but someone else must approve
Jenkins Builder: Verified
diff --git a/src/osmo-bsc/handover_decision_2.c b/src/osmo-bsc/handover_decision_2.c
index eb72e11..1ab92f5 100644
--- a/src/osmo-bsc/handover_decision_2.c
+++ b/src/osmo-bsc/handover_decision_2.c
@@ -1384,11 +1384,6 @@
return count;
}
-static bool is_intra_cell(const struct ho_candidate *c)
-{
- return c->bts && (c->lchan->ts->trx->bts == c->bts);
-}
-
static bool is_upgrade_to_tchf(const struct ho_candidate *c, uint8_t for_requirement)
{
return c->lchan
@@ -1399,46 +1394,78 @@
/* Given two candidates, pick the one that should rather be moved during handover.
* Return the better candidate in out-parameters best_cand and best_avg_db.
*/
-static struct ho_candidate *pick_better_lchan_to_move(bool want_highest_db,
- struct ho_candidate *a,
- struct ho_candidate *b)
+static struct ho_candidate *pick_better_lchan_to_move(struct ho_candidate *a,
+ struct ho_candidate *b,
+ uint8_t for_requirement)
{
- int a_rxlev;
- int b_rxlev;
+ int a_rxlev_change;
+ int b_rxlev_change;
+ struct ho_candidate *ret = a;
if (!a)
return b;
if (!b)
return a;
- a_rxlev = a->rxlev_target + a->rxlev_afs_bias;
- b_rxlev = b->rxlev_target + b->rxlev_afs_bias;
+ a_rxlev_change = a->rxlev_target - a->rxlev_current;
+ b_rxlev_change = b->rxlev_target - b->rxlev_current;
- if (want_highest_db && a_rxlev < b_rxlev)
- return b;
- if (!want_highest_db && a_rxlev > b_rxlev)
+ /* Typically, a congestion related handover reduces RXLEV. If there is a candidate that actually improves RXLEV,
+ * prefer that, because it pre-empts a likely handover due to measurement results later. Also favor unchanged
+ * RXLEV over a loss of RXLEV (favor staying within the same cell over moving to a worse cell). */
+ if (a_rxlev_change >= 0 && a_rxlev_change > b_rxlev_change)
+ return a;
+ if (b_rxlev_change >= 0 && b_rxlev_change > a_rxlev_change)
return b;
- /* The two lchans have identical ratings, prefer picking a dynamic timeslot: free PDCH and allow more timeslot
- * type flexibility for further congestion resolution. */
- if (lchan_is_on_dynamic_ts(b->lchan)) {
- /* If both are dynamic, prefer one that completely (or to a higher degree) frees its timeslot. */
- if (lchan_is_on_dynamic_ts(a->lchan)
- && ts_usage_count(a->lchan->ts) < ts_usage_count(b->lchan->ts))
+ if (a_rxlev_change < 0 && b_rxlev_change < 0) {
+ /* For handover that reduces RXLEV, favor the highest resulting RXLEV, AFS bias applied. */
+ int a_rxlev = a->rxlev_target + a->rxlev_afs_bias;
+ int b_rxlev = b->rxlev_target + b->rxlev_afs_bias;
+
+ if (a_rxlev > b_rxlev)
return a;
- /* If both equally satisfy these preferences, it does not matter which one is picked.
- * Give slight preference to moving later dyn TS, so that a free dyn TS may group with following static
- * PDCH, though this depends on how the user configured the TS -- not harmful to do so anyway. */
- return b;
+ if (b_rxlev > a_rxlev)
+ return b;
+ /* There is no target RXLEV difference between the two candidates. Let other factors influence the
+ * choice. */
}
- return a;
+ /* Prefer picking a dynamic timeslot: free PDCH and allow more timeslot type flexibility for further
+ * congestion resolution. */
+ if (lchan_is_on_dynamic_ts(b->lchan)) {
+ unsigned int ac, bc;
+
+ if (!lchan_is_on_dynamic_ts(a->lchan))
+ return b;
+
+ /* Both are dynamic timeslots. Prefer one that completely (or to a higher degree) frees its
+ * timeslot. */
+ ac = ts_usage_count(a->lchan->ts);
+ bc = ts_usage_count(b->lchan->ts);
+ if (bc < ac)
+ return b;
+ if (ac < bc)
+ return a;
+ /* (If both are dynamic timeslots, favor moving the later dynamic timeslot. That is a vague preference
+ * for later dynamic TS to become PDCH and join up with plain PDCH that follow it -- not actually clear
+ * whether that helps, and depends on user's TS config. No harm done either way.) */
+ ret = b;
+ }
+
+ /* When upgrading TCH/H to TCH/F, favor moving a TCH/H with lower current rxlev, because presumably that
+ * one benefits more from a higher bandwidth. */
+ if (is_upgrade_to_tchf(a, for_requirement) && is_upgrade_to_tchf(b, for_requirement)) {
+ if (b->rxlev_current < a->rxlev_current)
+ return b;
+ if (a->rxlev_current < b->rxlev_current)
+ return a;
+ }
+
+ return ret;
}
static struct ho_candidate *pick_best_candidate(struct ho_candidate *clist, int clist_len,
- bool want_highest_db,
- bool omit_intra_cell_upgrade_to_tchf,
- bool only_intra_cell_upgrade_to_tchf,
uint8_t for_requirement)
{
struct ho_candidate *result = NULL;
@@ -1446,8 +1473,6 @@
for (i = 0; i < clist_len; i++) {
struct ho_candidate *c = &clist[i];
- bool intra_cell;
- bool upgrade_to_tch_f;
/* For multiple passes of congestion resolution, already handovered candidates are marked by lchan =
* NULL. (though at the time of writing, multiple passes of congestion resolution are DISABLED.) */
@@ -1461,23 +1486,13 @@
if (!(c->requirements & for_requirement))
continue;
- intra_cell = is_intra_cell(c);
- upgrade_to_tch_f = is_upgrade_to_tchf(c, for_requirement);
-
- if (only_intra_cell_upgrade_to_tchf
- && !(intra_cell && upgrade_to_tch_f))
- continue;
- if (omit_intra_cell_upgrade_to_tchf
- && (intra_cell && upgrade_to_tch_f))
- continue;
-
/* improve AHS */
- if (upgrade_to_tch_f)
+ if (is_upgrade_to_tchf(c, for_requirement))
c->rxlev_afs_bias = ho_get_hodec2_afs_bias_rxlev(c->bts->ho);
else
c->rxlev_afs_bias = 0;
- result = pick_better_lchan_to_move(want_highest_db, result, c);
+ result = pick_better_lchan_to_move(result, c, for_requirement);
}
return result;
@@ -1643,11 +1658,7 @@
/* TODO: attempt inter-BSC HO if no local cells qualify, and rely on the remote BSS to
* deny receiving the handover if it also considers itself congested. Maybe do that only
* when the cell is absolutely full, i.e. not only min-free-slots. (x) */
- best_cand = pick_best_candidate(clist, candidates,
- /* want_highest_db */ true,
- /* omit_intra_cell_upgrade_to_tchf */ true,
- /* only_intra_cell_upgrade_to_tchf */ false,
- REQUIREMENT_B_MASK);
+ best_cand = pick_best_candidate(clist, candidates, REQUIREMENT_B_MASK);
if (best_cand) {
any_ho = 1;
LOGPHOCAND(best_cand, LOGL_DEBUG, "Best candidate: RX level %d%s\n",
@@ -1676,54 +1687,12 @@
}
#if 0
-next_b2:
-#endif
- /* For upgrading TCH/H to TCH/F within the same cell, we want to pick the *lowest* average rxlev: for staying
- * within the same cell, give the MS with the worst service more bandwidth. When staying within the same cell,
- * the target avg rxlev is identical to the source lchan rxlev, so it is fine to use the same avg rxlev value,
- * but simply pick the lowest one.
- * Upgrading TCH/H channels obviously only applies when TCH/H is actually congested. */
- if (tchh_congestion > 0)
- best_cand = pick_best_candidate(clist, candidates,
- /* want_highest_db */ false,
- /* omit_intra_cell_upgrade_to_tchf */ false,
- /* only_intra_cell_upgrade_to_tchf */ true,
- REQUIREMENT_B_MASK);
- if (best_cand) {
- any_ho = 1;
- LOGPHOCAND(best_cand, LOGL_INFO, "Worst candidate: RX level %d from TCH/H -> TCH/F%s\n",
- rxlev2dbm(best_cand->rxlev_target),
- best_cand->rxlev_afs_bias ? " (applied AHS -> AFS rxlev bias)" : "");
- trigger_ho(best_cand, best_cand->requirements & REQUIREMENT_B_MASK);
-#if 0
- /* if there is still congestion, mark lchan as deleted
- * and redo this process */
- tchh_congestion--;
- if (tchh_congestion > 0) {
- delete_lchan = best_cand->lchan;
- best_cand = NULL;
- goto next_b2;
- }
-#else
- /* must exit here, because triggering handover/assignment
- * will cause change in requirements. more check for this
- * bts is performed in the next iteration.
- */
-#endif
- goto exit;
- }
-
-#if 0
next_c1:
#endif
/* Select best candidate that balances congestion.
* Again no remote BSS.
* Again no TCH/H -> F upgrades within the same cell. */
- best_cand = pick_best_candidate(clist, candidates,
- /* want_highest_db */ true,
- /* omit_intra_cell_upgrade_to_tchf */ true,
- /* only_intra_cell_upgrade_to_tchf */ false,
- REQUIREMENT_C_MASK);
+ best_cand = pick_best_candidate(clist, candidates, REQUIREMENT_C_MASK);
if (best_cand) {
any_ho = 1;
LOGPHOCAND(best_cand, LOGL_INFO, "Best candidate: RX level %d%s\n",
@@ -1754,46 +1723,6 @@
LOGPHOBTS(bts, LOGL_DEBUG, "Did not find a best candidate that fulfills requirement C"
" (omitting change from AHS to AFS)\n");
-#if 0
-next_c2:
-#endif
- /* Look for upgrading TCH/H to TCH/F within the same cell, which balances congestion, again upgrade the TCH/H
- * lchan that has the worst reception: */
- if (tchh_congestion > 0)
- best_cand = pick_best_candidate(clist, candidates,
- /* want_highest_db */ false,
- /* omit_intra_cell_upgrade_to_tchf */ false,
- /* only_intra_cell_upgrade_to_tchf */ true,
- REQUIREMENT_C_MASK);
-
- /* perform handover, if there is a candidate */
- if (best_cand) {
- any_ho = 1;
- LOGPHOCAND(best_cand, LOGL_INFO, "Worst candidate: RX level %d from TCH/H -> TCH/F%s\n",
- rxlev2dbm(best_cand->rxlev_target),
- best_cand->rxlev_afs_bias ? " (applied AHS -> AFS rxlev bias)" : "");
- trigger_ho(best_cand, best_cand->requirements & REQUIREMENT_C_MASK);
-#if 0
- /* if there is still congestion, mark lchan as deleted
- * and redo this process */
- tchh_congestion--;
- if (tchh_congestion > 0) {
- delete_lchan = best_cand->lchan;
- best_cand = NULL;
- goto next_c2;
- }
-#else
- /* must exit here, because triggering handover/assignment
- * will cause change in requirements. more check for this
- * bts is performed in the next iteration.
- */
-#endif
- goto exit;
- }
- LOGPHOBTS(bts, LOGL_DEBUG, "Did not find a worst candidate that fulfills requirement C,"
- " selecting candidates that change from AHS to AFS only\n");
-
-
exit:
/* free array */
talloc_free(clist);
diff --git a/tests/handover/test_amr_tch_h_to_f_congestion_two_cells.ho_vty b/tests/handover/test_amr_tch_h_to_f_congestion_two_cells.ho_vty
index eaaeabd..7cbcd4b 100644
--- a/tests/handover/test_amr_tch_h_to_f_congestion_two_cells.ho_vty
+++ b/tests/handover/test_amr_tch_h_to_f_congestion_two_cells.ho_vty
@@ -12,12 +12,6 @@
meas-rep repeat 10 lchan 1 0 4 0 rxlev 30 rxqual 0 ta 0 neighbors 20
expect-no-chan
congestion-check
-# FAIL: bts 1 has better rxlev, so the call should stay in bts 1. Instead, a handover to bts 0 happens.
-expect-ho from lchan 1 0 4 0 to lchan 0 0 1 0
-expect-ts-use trx 0 0 states * TCH/F - - - - - *
-expect-ts-use trx 1 0 states * - - - - - - *
-# the fail continues: later the better rxqual does *another* ho back to the original cell
-meas-rep lchan 0 0 1 0 rxlev 20 rxqual 0 ta 0 neighbors 30
-expect-ho from lchan 0 0 1 0 to lchan 1 0 1 0
+expect-ho from lchan 1 0 4 0 to lchan 1 0 1 0
expect-ts-use trx 0 0 states * - - - - - - *
expect-ts-use trx 1 0 states * TCH/F - - - - - *
diff --git a/tests/handover/test_congestion_intra_vs_inter_cell.ho_vty b/tests/handover/test_congestion_intra_vs_inter_cell.ho_vty
index f0f8e2a..d93f56c 100644
--- a/tests/handover/test_congestion_intra_vs_inter_cell.ho_vty
+++ b/tests/handover/test_congestion_intra_vs_inter_cell.ho_vty
@@ -12,11 +12,9 @@
expect-no-chan
congestion-check
-# FAIL! moving to a weaker neighbor, should move to TCH/F in current cell instead.
-# FAIL! should favor upgrading the weaker TS 6.
-expect-ho from lchan 0 0 5 0 to lchan 1 0 1 0
-expect-ts-use trx 0 0 states * - - - - - TCH/H- *
-expect-ts-use trx 1 0 states * TCH/F - - - - - *
+expect-ho from lchan 0 0 6 0 to lchan 0 0 1 0
+expect-ts-use trx 0 0 states * TCH/F - - - TCH/H- - *
+expect-ts-use trx 1 0 states * - - - - - - *
# clear measurements for next run
set-ts-use trx 0 0 states * - - - - - - *
@@ -29,10 +27,9 @@
expect-no-chan
congestion-check
-# FAIL! moving to a weaker neighbor, should move to TCH/F in current cell instead.
-expect-ho from lchan 0 0 5 0 to lchan 1 0 1 0
-expect-ts-use trx 0 0 states * - - - - - TCH/H- *
-expect-ts-use trx 1 0 states * TCH/F - - - - - *
+expect-ho from lchan 0 0 5 0 to lchan 0 0 1 0
+expect-ts-use trx 0 0 states * TCH/F - - - - TCH/H- *
+expect-ts-use trx 1 0 states * - - - - - - *
# clear measurements for next run
set-ts-use trx 0 0 states * - - - - - - *
@@ -45,11 +42,9 @@
expect-no-chan
congestion-check
-# FAIL! moving to a weaker neighbor, should move to TCH/F in current cell instead.
-# FAIL! should favor upgrading the weaker TS 5.
-expect-ho from lchan 0 0 6 0 to lchan 1 0 1 0
-expect-ts-use trx 0 0 states * - - - - TCH/H- - *
-expect-ts-use trx 1 0 states * TCH/F - - - - - *
+expect-ho from lchan 0 0 5 0 to lchan 0 0 1 0
+expect-ts-use trx 0 0 states * TCH/F - - - - TCH/H- *
+expect-ts-use trx 1 0 states * - - - - - - *
# clear measurements for next run
set-ts-use trx 0 0 states * - - - - - - *
@@ -62,10 +57,9 @@
expect-no-chan
congestion-check
-# FAIL! moving to a weaker neighbor, should move to TCH/F in current cell instead.
-expect-ho from lchan 0 0 5 0 to lchan 1 0 1 0
-expect-ts-use trx 0 0 states * - - - - - TCH/H- *
-expect-ts-use trx 1 0 states * TCH/F - - - - - *
+expect-ho from lchan 0 0 5 0 to lchan 0 0 1 0
+expect-ts-use trx 0 0 states * TCH/F - - - - TCH/H- *
+expect-ts-use trx 1 0 states * - - - - - - *
# clear measurements for next run
set-ts-use trx 0 0 states * - - - - - - *
@@ -78,11 +72,9 @@
expect-no-chan
congestion-check
-# FAIL! moving to a weaker neighbor, should move to TCH/F in current cell instead.
-# FAIL! should favor upgrading the weaker TS 5.
-expect-ho from lchan 0 0 6 0 to lchan 1 0 1 0
-expect-ts-use trx 0 0 states * - - - - TCH/H- - *
-expect-ts-use trx 1 0 states * TCH/F - - - - - *
+expect-ho from lchan 0 0 5 0 to lchan 0 0 1 0
+expect-ts-use trx 0 0 states * TCH/F - - - - TCH/H- *
+expect-ts-use trx 1 0 states * - - - - - - *
# clear measurements for next run
set-ts-use trx 0 0 states * - - - - - - *
@@ -95,11 +87,9 @@
expect-no-chan
congestion-check
-# FAIL! moving to a weaker neighbor, should move to TCH/F in current cell instead.
-# FAIL! should favor upgrading the weaker TS 6.
-expect-ho from lchan 0 0 5 0 to lchan 1 0 1 0
-expect-ts-use trx 0 0 states * - - - - - TCH/H- *
-expect-ts-use trx 1 0 states * TCH/F - - - - - *
+expect-ho from lchan 0 0 6 0 to lchan 0 0 1 0
+expect-ts-use trx 0 0 states * TCH/F - - - TCH/H- - *
+expect-ts-use trx 1 0 states * - - - - - - *
# clear measurements for next run
set-ts-use trx 0 0 states * - - - - - - *
diff --git a/tests/handover/test_dyn_ts_favor_moving_half_used_tch_h.ho_vty b/tests/handover/test_dyn_ts_favor_moving_half_used_tch_h.ho_vty
index 9d88c86..794bbef 100644
--- a/tests/handover/test_dyn_ts_favor_moving_half_used_tch_h.ho_vty
+++ b/tests/handover/test_dyn_ts_favor_moving_half_used_tch_h.ho_vty
@@ -41,6 +41,5 @@
meas-rep lchan 0 0 4 0 rxlev 32 rxqual 0 ta 0
meas-rep lchan 0 0 4 1 rxlev 31 rxqual 0 ta 0
congestion-check
-# FAIL: should still pick lchan 0 0 3 0!
-expect-ho from lchan 0 0 4 1 to lchan 0 0 1 0
-expect-ts-use trx 0 0 states * TCH/F TCH/HH TCH/H- TCH/H- pdch - -
+expect-ho from lchan 0 0 3 0 to lchan 0 0 1 0
+expect-ts-use trx 0 0 states * TCH/F TCH/HH pdch TCH/HH pdch - -
--
To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/21979
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-bsc
Gerrit-Branch: master
Gerrit-Change-Id: I2704899c85c35dfd4eba43468452483f40016ca2
Gerrit-Change-Number: 21979
Gerrit-PatchSet: 3
Gerrit-Owner: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge at osmocom.org>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-MessageType: merged
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20210111/18caf3e7/attachment.htm>