From 91a202d4885d1e3aff6cb090ec688d695c2cd26b Mon Sep 17 00:00:00 2001 From: Neels Hofmeyr Date: Sat, 13 Jul 2019 01:12:53 +0200 Subject: neighbor config: allow re-using ARFCN+BSIC pairs Fix neighbor config to match OsmoBSC manual: implement the plan for neighbor configuration that was so far only described in the manual without actually being in operation. This first allows re-using ARFCN+BSIC pairs in and across BSS. So far the handover_start() code always looked for handover target cells across *all* local cells, even if they were not listed as neighbors to a source cell. Imply all cells as neighbors only as long as there are no explicit neighbors configured. As soon as the first 'neighbor' line appears in a 'bts' config, only the listed neighbors are regarded as handover target cells. (The 'neighbor-list' commands are not related to this, only the relatively new 'neighbor (bts|lac|cgi|...)' commands affect actual handover procedures.) TTCN3 tests TC_ho_neighbor_config_1 thru _7 play through the various aspects of neighbor configuration: both the legacy implicit all-cells-are-neighbors as well as allowing only explicit neighbors by config. Related: OS#4056 Related: osmo-ttcn3-hacks Ia4ba0e75abd3d45a3422b2525e5f938cdc5a04cc Change-Id: I29bca59ab232eddc74e0d4698efb9c9992443983 --- include/osmocom/bsc/handover.h | 18 ++- include/osmocom/bsc/handover_fsm.h | 12 -- include/osmocom/bsc/neighbor_ident.h | 2 + src/osmo-bsc/handover_decision_2.c | 14 +-- src/osmo-bsc/handover_fsm.c | 32 ++++-- src/osmo-bsc/handover_logic.c | 211 +++++++++++++++++++++++++++++++---- src/osmo-bsc/neighbor_ident_vty.c | 9 ++ tests/bsc/bsc_test.c | 2 + 8 files changed, 240 insertions(+), 60 deletions(-) diff --git a/include/osmocom/bsc/handover.h b/include/osmocom/bsc/handover.h index 322913da4..b00ee60f8 100644 --- a/include/osmocom/bsc/handover.h +++ b/include/osmocom/bsc/handover.h @@ -10,6 +10,15 @@ #include #include +#define LOG_HO(conn, level, fmt, args...) do { \ + if (conn->ho.fi) \ + LOGPFSML(conn->ho.fi, level, "%s: " fmt, \ + handover_status(conn), ## args); \ + else \ + LOGP(DHODEC, level, "%s: " fmt, \ + handover_status(conn), ## args); \ + } while(0) + struct gsm_network; struct gsm_lchan; struct gsm_bts; @@ -25,6 +34,8 @@ enum handover_result { HO_RESULT_ERROR, }; +const char *handover_status(struct gsm_subscriber_connection *conn); + extern const struct value_string handover_result_names[]; inline static const char *handover_result_name(enum handover_result val) { return get_value_string(handover_result_names, val); } @@ -70,8 +81,11 @@ enum handover_result bsc_tx_bssmap_ho_complete(struct gsm_subscriber_connection struct gsm_lchan *lchan); void bsc_tx_bssmap_ho_failure(struct gsm_subscriber_connection *conn); -struct gsm_bts *bts_by_neighbor_ident(const struct gsm_network *net, - const struct neighbor_ident_key *search_for); +int find_handover_target_cell(struct gsm_bts **local_target_cell_p, + const struct gsm0808_cell_id_list2 **remote_target_cell_p, + struct gsm_subscriber_connection *conn, const struct neighbor_ident_key *search_for, + bool log_errors); + struct neighbor_ident_key *bts_ident_key(const struct gsm_bts *bts); void handover_parse_inter_bsc_mt(struct gsm_subscriber_connection *conn, diff --git a/include/osmocom/bsc/handover_fsm.h b/include/osmocom/bsc/handover_fsm.h index 7c2145e84..1628d8fd9 100644 --- a/include/osmocom/bsc/handover_fsm.h +++ b/include/osmocom/bsc/handover_fsm.h @@ -4,18 +4,6 @@ #include #include -const char *handover_status(struct gsm_subscriber_connection *conn); - -/* This macro automatically includes a final \n, if omitted. */ -#define LOG_HO(conn, level, fmt, args...) do { \ - if (conn->ho.fi) \ - LOGPFSML(conn->ho.fi, level, "%s: " fmt, \ - handover_status(conn), ## args); \ - else \ - LOGP(DHODEC, level, "%s: " fmt, \ - handover_status(conn), ## args); \ - } while(0) - /* Terminology: * Intra-Cell: stays within one BTS, this should actually be an Assignment. * Intra-BSC: stays within one BSC, but moves between BTSes. diff --git a/include/osmocom/bsc/neighbor_ident.h b/include/osmocom/bsc/neighbor_ident.h index 17bffbc14..aa3827635 100644 --- a/include/osmocom/bsc/neighbor_ident.h +++ b/include/osmocom/bsc/neighbor_ident.h @@ -47,6 +47,8 @@ void neighbor_ident_iter(const struct neighbor_ident_list *nil, void neighbor_ident_vty_init(struct gsm_network *net, struct neighbor_ident_list *nil); void neighbor_ident_vty_write(struct vty *vty, const char *indent, struct gsm_bts *bts); +bool neighbor_ident_bts_entry_exists(uint8_t from_bts); + #define NEIGHBOR_IDENT_VTY_KEY_PARAMS "arfcn <0-1023> bsic (<0-63>|any)" #define NEIGHBOR_IDENT_VTY_KEY_DOC \ "ARFCN of neighbor cell\n" "ARFCN value\n" \ diff --git a/src/osmo-bsc/handover_decision_2.c b/src/osmo-bsc/handover_decision_2.c index a8fff6319..0e24c0d47 100644 --- a/src/osmo-bsc/handover_decision_2.c +++ b/src/osmo-bsc/handover_decision_2.c @@ -900,18 +900,8 @@ static void collect_handover_candidate(struct gsm_lchan *lchan, struct neigh_mea return; } - neighbor_bts = bts_by_neighbor_ident(bts->network, &ni); - - neighbor_cil = neighbor_ident_get(bts->network->neighbor_bss_cells, &ni); - - if (neighbor_bts && neighbor_cil) { - LOGPHOBTS(bts, LOGL_ERROR, "Configuration error: %s exists as both local" - " neighbor (bts %u) and remote-BSS neighbor (%s). Will consider only" - " the local-BSS neighbor.\n", - neighbor_ident_key_name(&ni), - neighbor_bts->nr, gsm0808_cell_id_list_name(neighbor_cil)); - neighbor_cil = NULL; - } + find_handover_target_cell(&neighbor_bts, &neighbor_cil, + lchan->conn, &ni, false); if (!neighbor_bts && !neighbor_cil) { LOGPHOBTS(bts, LOGL_DEBUG, "no neighbor ARFCN %u BSIC %u configured for this cell\n", diff --git a/src/osmo-bsc/handover_fsm.c b/src/osmo-bsc/handover_fsm.c index 6d0c2d47e..d1593470b 100644 --- a/src/osmo-bsc/handover_fsm.c +++ b/src/osmo-bsc/handover_fsm.c @@ -31,6 +31,7 @@ #include #include +#include #include #include #include @@ -200,6 +201,9 @@ void handover_request(struct handover_out_req *req) conn = req->old_lchan->conn; OSMO_ASSERT(conn && conn->fi); + /* Make sure the handover target neighbor_ident_key contains the correct source bts nr */ + req->target_nik.from_bts = req->old_lchan->ts->trx->bts->nr; + /* To make sure we're allowed to start a handover, go through a gscon event dispatch. If that is accepted, the * same req is passed to handover_start(). */ osmo_fsm_inst_dispatch(conn->fi, GSCON_EV_HANDOVER_START, req); @@ -285,9 +289,10 @@ void handover_start(struct handover_out_req *req) OSMO_ASSERT(req && req->old_lchan && req->old_lchan->conn); struct gsm_subscriber_connection *conn = req->old_lchan->conn; + const struct neighbor_ident_key *search_for = &req->target_nik; struct handover *ho = &conn->ho; - struct gsm_bts *bts; - const struct gsm0808_cell_id_list2 *cil; + struct gsm_bts *local_target_cell = NULL; + const struct gsm0808_cell_id_list2 *remote_target_cell = NULL; if (conn->ho.fi) { LOG_HO(conn, LOGL_ERROR, "Handover requested while another handover is ongoing; Ignore\n"); @@ -295,6 +300,9 @@ void handover_start(struct handover_out_req *req) } handover_reset(conn); + /* When handover_start() is invoked by the gscon, it expects a HANDOVER_END event. The best way to ensure this + * is to always create a handover_fsm instance, even if the target cell is not resolved yet. Any failure should + * then call handover_end(), which ensures that the conn snaps back to a valid state. */ handover_fsm_alloc(conn); ho->from_hodec_id = req->from_hodec_id; @@ -302,21 +310,25 @@ void handover_start(struct handover_out_req *req) req->old_lchan->type : req->new_lchan_type; ho->target_cell = req->target_nik; - bts = bts_by_neighbor_ident(conn->network, &req->target_nik); - if (bts) { - ho->new_bts = bts; + if (find_handover_target_cell(&local_target_cell, &remote_target_cell, + conn, search_for, true)) + goto no_handover; + + if (local_target_cell) { + ho->new_bts = local_target_cell; handover_start_intra_bsc(conn); return; } - cil = neighbor_ident_get(conn->network->neighbor_bss_cells, &req->target_nik); - if (cil) { - handover_start_inter_bsc_out(conn, cil); + if (remote_target_cell) { + handover_start_inter_bsc_out(conn, remote_target_cell); return; } - LOG_HO(conn, LOGL_ERROR, "Cannot handover %s: neighbor unknown\n", - neighbor_ident_key_name(&req->target_nik)); + /* should never reach this, because find_handover_target_cell() would have returned error. */ + OSMO_ASSERT(false); + +no_handover: handover_end(conn, HO_RESULT_FAIL_NO_CHANNEL); } diff --git a/src/osmo-bsc/handover_logic.c b/src/osmo-bsc/handover_logic.c index 5725213ef..5be838340 100644 --- a/src/osmo-bsc/handover_logic.c +++ b/src/osmo-bsc/handover_logic.c @@ -125,42 +125,205 @@ int bts_handover_count(struct gsm_bts *bts, int ho_scopes) return count; } -struct gsm_bts *bts_by_neighbor_ident(const struct gsm_network *net, - const struct neighbor_ident_key *search_for) +/* Find out a handover target cell for the given neighbor_ident_key, + * and make sure there are no ambiguous matches. + * Given a source BTS and a target ARFCN+BSIC, find which cell is the right handover target. + * ARFCN+BSIC may be re-used within and/or across BSS, so make sure that only those cells that are explicitly + * listed as neighbor of the source cell are viable handover targets. + * The (legacy) default configuration is that, when no explicit neighbors are listed, that all local cells are + * neighbors, in which case each ARFCN+BSIC must exist at most once. + * If there is more than one viable handover target cell found for the given ARFCN+BSIC, that constitutes a + * configuration error and should not result in handover, so that the system's misconfiguration is more likely + * to be found. + */ +int find_handover_target_cell(struct gsm_bts **local_target_cell_p, + const struct gsm0808_cell_id_list2 **remote_target_cell_p, + struct gsm_subscriber_connection *conn, const struct neighbor_ident_key *search_for, + bool log_errors) { - struct gsm_bts *found = NULL; - struct gsm_bts *bts; - struct gsm_bts *wildcard_match = NULL; - - llist_for_each_entry(bts, &net->bts_list, list) { - struct neighbor_ident_key entry = { - .from_bts = NEIGHBOR_IDENT_KEY_ANY_BTS, - .arfcn = bts->c0->arfcn, - .bsic = bts->bsic, - }; - if (neighbor_ident_key_match(&entry, search_for, true)) { - if (found) { - LOGP(DHO, LOGL_ERROR, "CONFIG ERROR: Multiple BTS match %s: %d and %d\n", - neighbor_ident_key_name(search_for), - found->nr, bts->nr); - return found; + struct gsm_network *net = conn->network; + struct gsm_bts *from_bts; + struct gsm_bts *local_target_cell = NULL; + const struct gsm0808_cell_id_list2 *remote_target_cell = NULL; + struct gsm_bts_ref *neigh; + bool ho_active; + bool as_active; + + if (local_target_cell_p) + *local_target_cell_p = NULL; + if (remote_target_cell_p) + *remote_target_cell_p = NULL; + + if (!search_for) { + if (log_errors) + LOG_HO(conn, LOGL_ERROR, "Handover without target cell\n"); + return -EINVAL; + } + + from_bts = gsm_bts_num(net, search_for->from_bts); + if (!from_bts) { + if (log_errors) + LOG_HO(conn, LOGL_ERROR, "Handover without source cell\n"); + return -EINVAL; + } + + ho_active = ho_get_ho_active(from_bts->ho); + as_active = (ho_get_algorithm(from_bts->ho) == 2) + && ho_get_hodec2_as_active(from_bts->ho); + if (!ho_active && !as_active) { + if (log_errors) + LOG_HO(conn, LOGL_ERROR, "Cannot start Handover: Handover and Assignment disabled for this source cell (%s)\n", + neighbor_ident_key_name(search_for)); + return -EINVAL; + } + + if (llist_empty(&from_bts->local_neighbors) + && !neighbor_ident_bts_entry_exists(from_bts->nr)) { + /* No explicit neighbor entries exist for this BTS. Hence apply the legacy default behavior that all + * local cells are neighbors. */ + struct gsm_bts *bts; + struct gsm_bts *wildcard_match = NULL; + + LOG_HO(conn, LOGL_DEBUG, "No explicit neighbors, regarding all local cells as neighbors\n"); + + llist_for_each_entry(bts, &net->bts_list, list) { + struct neighbor_ident_key bts_key = *bts_ident_key(bts); + if (neighbor_ident_key_match(&bts_key, search_for, true)) { + if (local_target_cell) { + if (log_errors) + LOG_HO(conn, LOGL_ERROR, + "NEIGHBOR CONFIGURATION ERROR: Multiple local cells match %s" + " (BTS %d and BTS %d)." + " Aborting Handover because of ambiguous network topology.\n", + neighbor_ident_key_name(search_for), + local_target_cell->nr, bts->nr); + return -EINVAL; + } + local_target_cell = bts; } - found = bts; + if (neighbor_ident_key_match(&bts_key, search_for, false)) + wildcard_match = bts; + } + + if (!local_target_cell) + local_target_cell = wildcard_match; + + if (!local_target_cell) { + if (log_errors) + LOG_HO(conn, LOGL_ERROR, "Cannot Handover, no cell matches %s\n", + neighbor_ident_key_name(search_for)); + return -EINVAL; + } + + if (local_target_cell == from_bts && !as_active) { + if (log_errors) + LOG_HO(conn, LOGL_ERROR, + "Cannot start re-assignment, Assignment disabled for this cell (%s)\n", + neighbor_ident_key_name(search_for)); + return -EINVAL; + } + if (local_target_cell != from_bts && !ho_active) { + if (log_errors) + LOG_HO(conn, LOGL_ERROR, + "Cannot start Handover, Handover disabled for this cell (%s)\n", + neighbor_ident_key_name(search_for)); + return -EINVAL; + } + + if (local_target_cell_p) + *local_target_cell_p = local_target_cell; + return 0; + } + + /* One or more local- or remote-BSS cell neighbors are configured. Find a match among those, but also detect + * ambiguous matches (if multiple cells match, it is a configuration error). */ + + LOG_HO(conn, LOGL_DEBUG, "There are explicit neighbors configured for this cell\n"); + + /* Iterate explicit local neighbor cells */ + llist_for_each_entry(neigh, &from_bts->local_neighbors, entry) { + struct gsm_bts *neigh_bts = neigh->bts; + struct neighbor_ident_key neigh_bts_key = *bts_ident_key(neigh_bts); + neigh_bts_key.from_bts = from_bts->nr; + + LOG_HO(conn, LOGL_DEBUG, "Local neighbor %s\n", neighbor_ident_key_name(&neigh_bts_key)); + + if (!neighbor_ident_key_match(&neigh_bts_key, search_for, true)) { + LOG_HO(conn, LOGL_DEBUG, "Doesn't match %s\n", neighbor_ident_key_name(search_for)); + continue; } - if (neighbor_ident_key_match(&entry, search_for, false)) - wildcard_match = bts; + + if (local_target_cell) { + if (log_errors) + LOG_HO(conn, LOGL_ERROR, + "NEIGHBOR CONFIGURATION ERROR: Multiple BTS match %s (BTS %d and BTS %d)." + " Aborting Handover because of ambiguous network topology.\n", + neighbor_ident_key_name(search_for), local_target_cell->nr, neigh_bts->nr); + return -EINVAL; + } + + local_target_cell = neigh_bts; + } + + /* Any matching remote-BSS neighbor cell? */ + remote_target_cell = neighbor_ident_get(net->neighbor_bss_cells, search_for); + + if (remote_target_cell) + LOG_HO(conn, LOGL_DEBUG, "Found remote target cell %s\n", + gsm0808_cell_id_list_name(remote_target_cell)); + + if (local_target_cell && remote_target_cell) { + if (log_errors) + LOG_HO(conn, LOGL_ERROR, "NEIGHBOR CONFIGURATION ERROR: Both a local and a remote-BSS cell match %s" + " (BTS %d and remote %s)." + " Aborting Handover because of ambiguous network topology.\n", + neighbor_ident_key_name(search_for), local_target_cell->nr, + gsm0808_cell_id_list_name(remote_target_cell)); + return -EINVAL; + } + + if (local_target_cell == from_bts && !as_active) { + if (log_errors) + LOG_HO(conn, LOGL_ERROR, + "Cannot start re-assignment, Assignment disabled for this cell (%s)\n", + neighbor_ident_key_name(search_for)); + return -EINVAL; + } + + if (((local_target_cell && local_target_cell != from_bts) + || remote_target_cell) + && !ho_active) { + if (log_errors) + LOG_HO(conn, LOGL_ERROR, + "Cannot start Handover, Handover disabled for this cell (%s)\n", + neighbor_ident_key_name(search_for)); + return -EINVAL; + } + + if (local_target_cell) { + if (local_target_cell_p) + *local_target_cell_p = local_target_cell; + return 0; + } + + if (remote_target_cell) { + if (remote_target_cell_p) + *remote_target_cell_p = remote_target_cell; + return 0; } - if (found) - return found; + if (log_errors) + LOG_HO(conn, LOGL_ERROR, "Cannot handover %s: neighbor unknown\n", + neighbor_ident_key_name(search_for)); - return wildcard_match; + return -ENODEV; } struct neighbor_ident_key *bts_ident_key(const struct gsm_bts *bts) { static struct neighbor_ident_key key; key = (struct neighbor_ident_key){ + .from_bts = NEIGHBOR_IDENT_KEY_ANY_BTS, .arfcn = bts->c0->arfcn, .bsic = bts->bsic, }; diff --git a/src/osmo-bsc/neighbor_ident_vty.c b/src/osmo-bsc/neighbor_ident_vty.c index 2b8cd7e2b..f158e7f4e 100644 --- a/src/osmo-bsc/neighbor_ident_vty.c +++ b/src/osmo-bsc/neighbor_ident_vty.c @@ -389,6 +389,15 @@ static bool nil_match_bts(const struct neighbor_ident_key *key, return true; } +bool neighbor_ident_bts_entry_exists(uint8_t from_bts) +{ + struct nil_match_bts_data d = { + .bts_nr = from_bts, + }; + neighbor_ident_iter(g_neighbor_cells, nil_match_bts, &d); + return (bool)d.found; +} + static int neighbor_del_all(struct vty *vty) { int rc; diff --git a/tests/bsc/bsc_test.c b/tests/bsc/bsc_test.c index 492f0c5a1..103e0bbb8 100644 --- a/tests/bsc/bsc_test.c +++ b/tests/bsc/bsc_test.c @@ -250,3 +250,5 @@ void gscon_submit_rsl_dtap(struct gsm_subscriber_connection *conn, struct msgb *msg, int link_id, int allow_sacch) {} void ts_fsm_alloc(struct gsm_bts_trx_ts *ts) {} void lchan_activate(struct gsm_lchan *lchan, void *info) {} +bool neighbor_ident_bts_entry_exists(uint8_t from_bts) { return false; } +const char *handover_status(struct gsm_subscriber_connection *conn) { return "x"; } -- cgit v1.2.3