From d88f9a538408c69c3a39b5370197aedc8492c3fc Mon Sep 17 00:00:00 2001 From: Neels Hofmeyr Date: Tue, 9 Mar 2021 17:10:18 +0100 Subject: refactor handover penalty timers So far the list of penalty timers was stored for an opaque target pointer. That was either a gsm_bts pointer for a local BTS, or a cell identifier list pointer for a remote-BSS cell. Reasons to refactor penalty timers: - The cell identifier list pointer came from the neighbor configuration storage, but the way cell neighbor config is stored will change in a subsequent patch. There will be no more cell identifier lists there. - Storing object pointers is inherently unsafe -- if an object gets removed and another gets allocated, the penalty timer could theoretically remain in force for an unrelated object. Rather store penalty timers for specific Cell IDs. Since remote-BSS neighbors can be requested by a cell identifier *list*, use a gsm0808_cell_id_list2 as key in the list of penalty timers. Fix handover_test.c: have different CI for each local BTS. So far it was the same LAC+CI for all BTSes, which now would make the test fail, because any penalty timer would appear to apply to all local cells. Related: OS#5018 Change-Id: I72dd6226a6d69c3f653a3174c6f55bf4eecc6885 --- include/osmocom/bsc/bts.h | 2 + include/osmocom/bsc/gsm_data.h | 6 +- include/osmocom/bsc/penalty_timers.h | 47 +++++---------- src/osmo-bsc/bsc_subscr_conn_fsm.c | 4 +- src/osmo-bsc/bts.c | 28 +++++++++ src/osmo-bsc/handover_decision_2.c | 63 +++---------------- src/osmo-bsc/handover_fsm.c | 1 + src/osmo-bsc/penalty_timers.c | 113 ++++++++++++++++++++++------------- tests/handover/handover_test.c | 2 + 9 files changed, 135 insertions(+), 131 deletions(-) diff --git a/include/osmocom/bsc/bts.h b/include/osmocom/bsc/bts.h index 0c0e46734..efa413411 100644 --- a/include/osmocom/bsc/bts.h +++ b/include/osmocom/bsc/bts.h @@ -622,6 +622,8 @@ char *gsm_bts_name(const struct gsm_bts *bts); bool gsm_bts_matches_lai(const struct gsm_bts *bts, const struct osmo_location_area_id *lai); bool gsm_bts_matches_cell_id(const struct gsm_bts *bts, const struct gsm0808_cell_id *cell_id); +void gsm_bts_cell_id(struct gsm0808_cell_id *cell_id, const struct gsm_bts *bts); +void gsm_bts_cell_id_list(struct gsm0808_cell_id_list2 *cell_id_list, const struct gsm_bts *bts); int gsm_bts_local_neighbor_add(struct gsm_bts *bts, struct gsm_bts *neighbor); int gsm_bts_local_neighbor_del(struct gsm_bts *bts, const struct gsm_bts *neighbor); diff --git a/include/osmocom/bsc/gsm_data.h b/include/osmocom/bsc/gsm_data.h index af1c8e853..1bf21aef1 100644 --- a/include/osmocom/bsc/gsm_data.h +++ b/include/osmocom/bsc/gsm_data.h @@ -207,6 +207,10 @@ struct handover { enum gsm_chan_t new_lchan_type; struct neighbor_ident_key target_cell; + /* For inter-BSC handover, this may reflect more than one Cell ID. Must also be set for intra-BSC handover, + * because it is used as key for penalty timers (e.g. in handover decision 2). */ + struct gsm0808_cell_id_list2 target_cell_ids; + uint8_t ho_ref; struct gsm_bts *new_bts; struct gsm_lchan *new_lchan; @@ -248,7 +252,7 @@ struct gsm_subscriber_connection { struct { int failures; - struct penalty_timers *penalty_timers; + struct llist_head penalty_timers; } hodec2; /* "Codec List (MSC Preferred)" as received by the BSSAP Assignment Request. 3GPP 48.008 diff --git a/include/osmocom/bsc/penalty_timers.h b/include/osmocom/bsc/penalty_timers.h index f5d17786c..d662b3c31 100644 --- a/include/osmocom/bsc/penalty_timers.h +++ b/include/osmocom/bsc/penalty_timers.h @@ -2,40 +2,23 @@ * initially used by handover algorithm 2 to keep per-BTS timers for each subscriber connection. */ #pragma once -/* Opaque struct to manage penalty timers */ -struct penalty_timers; +#include -/* Initialize a list of penalty timers. - * param ctx: talloc context to allocate in. - * returns an empty struct penalty_timers. */ -struct penalty_timers *penalty_timers_init(void *ctx); +struct penalty_timer { + struct llist_head entry; -/* Add a penalty timer for an arbitrary object. - * Note: the ownership of for_object remains with the caller; it is handled as a mere void* value, so - * invalid pointers can be handled without problems, while common sense dictates that invalidated - * pointers (freed objects) should probably be removed from this list. More importantly, the pointer must - * match any pointers used to query penalty timers, so for_object should reference some global/singleton - * object that tends to stay around longer than the penalty timers. - * param pt: penalty timers list as from penalty_timers_init(). - * param for_object: arbitrary pointer reference to store a penalty timer for (passing NULL is possible, - * but note that penalty_timers_clear() will clear all timers if given for_object=NULL). - * param timeout: penalty time in seconds. */ -void penalty_timers_add(struct penalty_timers *pt, const void *for_object, int timeout); + struct gsm0808_cell_id for_target_cell; + unsigned int timeout; +}; -/* Return the amount of penalty time remaining for an object. - * param pt: penalty timers list as from penalty_timers_init(). - * param for_object: arbitrary pointer reference to query penalty timers for. - * returns seconds remaining until all penalty time has expired. */ -unsigned int penalty_timers_remaining(struct penalty_timers *pt, const void *for_object); +void penalty_timers_add(void *ctx, struct llist_head *penalty_timers, + const struct gsm0808_cell_id *for_target_cell, int timeout); +void penalty_timers_add_list(void *ctx, struct llist_head *penalty_timers, + const struct gsm0808_cell_id_list2 *for_target_cells, int timeout); -/* Clear penalty timers for one or all objects. - * param pt: penalty timers list as from penalty_timers_init(). - * param for_object: arbitrary pointer reference to clear penalty time for, - * or NULL to clear all timers. */ -void penalty_timers_clear(struct penalty_timers *pt, const void *for_object); +unsigned int penalty_timers_remaining(struct llist_head *penalty_timers, + const struct gsm0808_cell_id *for_target_cell); +unsigned int penalty_timers_remaining_list(struct llist_head *penalty_timers, + const struct gsm0808_cell_id_list2 *for_target_cells); -/* Free a struct as returned from penalty_timers_init(). - * Clear all timers from the list, deallocate the list and set the pointer to NULL. - * param pt: pointer-to-pointer which references a struct penalty_timers as returned by - * penalty_timers_init(); *pt_p will be set to NULL. */ -void penalty_timers_free(struct penalty_timers **pt_p); +void penalty_timers_clear(struct llist_head *penalty_timers, const struct gsm0808_cell_id *for_target_cell); diff --git a/src/osmo-bsc/bsc_subscr_conn_fsm.c b/src/osmo-bsc/bsc_subscr_conn_fsm.c index ed08e86ad..954c6a5d7 100644 --- a/src/osmo-bsc/bsc_subscr_conn_fsm.c +++ b/src/osmo-bsc/bsc_subscr_conn_fsm.c @@ -933,7 +933,7 @@ static void gscon_pre_term(struct osmo_fsm_inst *fi, enum osmo_fsm_term_cause ca /* drop pending messages */ gscon_dtap_queue_flush(conn, 0); - penalty_timers_free(&conn->hodec2.penalty_timers); + penalty_timers_clear(&conn->hodec2.penalty_timers, NULL); } static int gscon_timer_cb(struct osmo_fsm_inst *fi) @@ -1004,7 +1004,7 @@ struct gsm_subscriber_connection *bsc_subscr_con_allocate(struct gsm_network *ne conn->network = net; INIT_LLIST_HEAD(&conn->dtap_queue); - /* BTW, penalty timers will be initialized on-demand. */ + INIT_LLIST_HEAD(&conn->hodec2.penalty_timers); conn->sccp.conn_id = -1; /* don't allocate from 'conn' context, as gscon_cleanup() will call talloc_free(conn) before diff --git a/src/osmo-bsc/bts.c b/src/osmo-bsc/bts.c index 950af5fb3..5a7229872 100644 --- a/src/osmo-bsc/bts.c +++ b/src/osmo-bsc/bts.c @@ -414,6 +414,34 @@ bool gsm_bts_matches_cell_id(const struct gsm_bts *bts, const struct gsm0808_cel } } +/* Return a LAC+CI cell identity for the given BTS. + * (For matching a BTS within the local BSS, the PLMN code is not important.) */ +void gsm_bts_cell_id(struct gsm0808_cell_id *cell_id, const struct gsm_bts *bts) +{ + *cell_id = (struct gsm0808_cell_id){ + .id_discr = CELL_IDENT_LAC_AND_CI, + .id.lac_and_ci = { + .lac = bts->location_area_code, + .ci = bts->cell_identity, + }, + }; +} + +/* Same as gsm_bts_cell_id(), but return in a single-entry gsm0808_cell_id_list2. Useful for e.g. + * gsm0808_cell_id_list_add() and gsm0808_cell_id_lists_same(). */ +void gsm_bts_cell_id_list(struct gsm0808_cell_id_list2 *cell_id_list, const struct gsm_bts *bts) +{ + struct gsm0808_cell_id cell_id; + struct gsm0808_cell_id_list2 add; + int rc; + gsm_bts_cell_id(&cell_id, bts); + gsm0808_cell_id_to_list(&add, &cell_id); + /* Since the target list is empty, this should always succeed. */ + (*cell_id_list) = (struct gsm0808_cell_id_list2){}; + rc = gsm0808_cell_id_list_add(cell_id_list, &add); + OSMO_ASSERT(rc > 0); +} + static struct gsm_bts_ref *gsm_bts_ref_find(const struct llist_head *list, const struct gsm_bts *bts) { struct gsm_bts_ref *ref; diff --git a/src/osmo-bsc/handover_decision_2.c b/src/osmo-bsc/handover_decision_2.c index 134f50250..0d0391cc2 100644 --- a/src/osmo-bsc/handover_decision_2.c +++ b/src/osmo-bsc/handover_decision_2.c @@ -215,49 +215,6 @@ void hodec2_on_change_congestion_check_interval(struct gsm_network *net, unsigne reinit_congestion_timer(net); } -static void _conn_penalty_time_add(struct gsm_subscriber_connection *conn, - const void *for_object, - int penalty_time) -{ - if (!for_object) { - LOGP(DHODEC, LOGL_ERROR, "%s Unable to set Handover-2 penalty timer:" - " no target cell pointer\n", - bsc_subscr_name(conn->bsub)); - return; - } - - if (!conn->hodec2.penalty_timers) { - conn->hodec2.penalty_timers = penalty_timers_init(conn); - OSMO_ASSERT(conn->hodec2.penalty_timers); - } - - penalty_timers_add(conn->hodec2.penalty_timers, for_object, penalty_time); -} - -static void nik_penalty_time_add(struct gsm_subscriber_connection *conn, - struct neighbor_ident_key *nik, - int penalty_time) -{ - _conn_penalty_time_add(conn, - neighbor_ident_get(conn->network->neighbor_bss_cells, nik), - penalty_time); -} - -static void bts_penalty_time_add(struct gsm_subscriber_connection *conn, - struct gsm_bts *bts, - int penalty_time) -{ - _conn_penalty_time_add(conn, bts, penalty_time); -} - -static unsigned int conn_penalty_time_remaining(struct gsm_subscriber_connection *conn, - const void *for_object) -{ - if (!conn->hodec2.penalty_timers) - return 0; - return penalty_timers_remaining(conn->hodec2.penalty_timers, for_object); -} - /* did we get a RXLEV for a given cell in the given report? Mark matches as MRC_F_PROCESSED. */ static struct gsm_meas_rep_cell *cell_in_rep(struct gsm_meas_rep *mr, uint16_t arfcn, uint8_t bsic) { @@ -491,6 +448,7 @@ static void check_requirements(struct ho_candidate *c) uint8_t requirement = 0; unsigned int penalty_time; int32_t current_overbooked; + struct gsm0808_cell_id target_cell_id; c->requirements = 0; /* Requirement A */ @@ -510,7 +468,8 @@ static void check_requirements(struct ho_candidate *c) } /* the handover penalty timer must not run for this bts */ - penalty_time = conn_penalty_time_remaining(c->current.lchan->conn, c->target.bts); + gsm_bts_cell_id(&target_cell_id, c->target.bts); + penalty_time = penalty_timers_remaining(&c->current.lchan->conn->hodec2.penalty_timers, &target_cell_id); if (penalty_time) { LOGPHOLCHANTOBTS(c->current.lchan, c->target.bts, LOGL_DEBUG, "not a candidate, target BTS still in penalty time" " (%u seconds left)\n", penalty_time); @@ -788,7 +747,7 @@ static void check_requirements_remote_bss(struct ho_candidate *c) /* Requirement A */ /* the handover penalty timer must not run for this bts */ - penalty_time = conn_penalty_time_remaining(c->current.lchan->conn, c->target.cil); + penalty_time = penalty_timers_remaining_list(&c->current.lchan->conn->hodec2.penalty_timers, c->target.cil); if (penalty_time) { LOGPHOLCHANTOREMOTE(c->current.lchan, c->target.cil, LOGL_DEBUG, "not a candidate, target BSS still in penalty time" @@ -1544,6 +1503,7 @@ static void on_measurement_report(struct gsm_meas_rep *mr) /* Max Distance */ if (lchan->meas_rep_count > 0 && lchan->last_ta > ho_get_hodec2_max_distance(bts->ho)) { + struct gsm0808_cell_id bts_id; global_ho_reason = HO_REASON_MAX_DISTANCE; LOGPHOLCHAN(lchan, LOGL_NOTICE, "TA is TOO HIGH: %u > %d\n", lchan->last_ta, ho_get_hodec2_max_distance(bts->ho)); @@ -1551,7 +1511,9 @@ static void on_measurement_report(struct gsm_meas_rep *mr) * early. it must be started before selecting a better cell, * so there is no assignment selected, due to running * penalty timer. */ - bts_penalty_time_add(lchan->conn, bts, ho_get_hodec2_penalty_max_dist(bts->ho)); + gsm_bts_cell_id(&bts_id, bts); + penalty_timers_add(lchan->conn, &lchan->conn->hodec2.penalty_timers, &bts_id, + ho_get_hodec2_penalty_max_dist(bts->ho)); find_alternative_lchan(lchan, true); return; } @@ -1989,7 +1951,6 @@ static void congestion_check_cb(void *arg) static void on_handover_end(struct gsm_subscriber_connection *conn, enum handover_result result) { struct gsm_bts *old_bts = NULL; - struct gsm_bts *new_bts = NULL; int penalty; struct handover *ho = &conn->ho; @@ -1999,8 +1960,6 @@ static void on_handover_end(struct gsm_subscriber_connection *conn, enum handove if (conn->lchan) old_bts = conn->lchan->ts->trx->bts; - if (ho->new_lchan) - new_bts = ho->new_lchan->ts->trx->bts; /* Only interested in handovers within this BSS or going out into another BSS. Incoming handovers * from another BSS are accounted for in the other BSS. */ @@ -2027,11 +1986,7 @@ static void on_handover_end(struct gsm_subscriber_connection *conn, enum handove LOG_HO(conn, LOGL_NOTICE, "Failed, starting penalty timer (%d s)\n", penalty); conn->hodec2.failures = 0; - - if (new_bts) - bts_penalty_time_add(conn, new_bts, penalty); - else - nik_penalty_time_add(conn, &ho->target_cell, penalty); + penalty_timers_add_list(conn, &conn->hodec2.penalty_timers, &ho->target_cell_ids, penalty); } static struct handover_decision_callbacks hodec2_callbacks = { diff --git a/src/osmo-bsc/handover_fsm.c b/src/osmo-bsc/handover_fsm.c index 87359dc74..527015236 100644 --- a/src/osmo-bsc/handover_fsm.c +++ b/src/osmo-bsc/handover_fsm.c @@ -375,6 +375,7 @@ static void handover_start_intra_bsc(struct gsm_subscriber_connection *conn) ho->scope = (ho->new_bts == bts) ? HO_INTRA_CELL : HO_INTRA_BSC; ho->ho_ref = g_next_ho_ref++; ho->async = true; + gsm_bts_cell_id_list(&ho->target_cell_ids, ho->new_bts); ho->new_lchan = lchan_select_by_type(ho->new_bts, ho->new_lchan_type); diff --git a/src/osmo-bsc/penalty_timers.c b/src/osmo-bsc/penalty_timers.c index 02cf2468a..689006114 100644 --- a/src/osmo-bsc/penalty_timers.c +++ b/src/osmo-bsc/penalty_timers.c @@ -28,16 +28,6 @@ #include #include -struct penalty_timers { - struct llist_head timers; -}; - -struct penalty_timer { - struct llist_head entry; - const void *for_object; - unsigned int timeout; -}; - static unsigned int time_now(void) { time_t now; @@ -46,16 +36,14 @@ static unsigned int time_now(void) return (unsigned int)now; } -struct penalty_timers *penalty_timers_init(void *ctx) -{ - struct penalty_timers *pt = talloc_zero(ctx, struct penalty_timers); - if (!pt) - return NULL; - INIT_LLIST_HEAD(&pt->timers); - return pt; -} - -void penalty_timers_add(struct penalty_timers *pt, const void *for_object, int timeout) +/* Add a penalty timer for a target cell ID. + * \param ctx talloc context to allocate new struct penalty_timer from. + * \param penalty_timers llist head to add penalty timer to. + * \param for_target_cell Which handover target to penalize. + * \param timeout Penalty time in seconds. + */ +void penalty_timers_add(void *ctx, struct llist_head *penalty_timers, + const struct gsm0808_cell_id *for_target_cell, int timeout) { struct penalty_timer *timer; unsigned int now; @@ -67,9 +55,9 @@ void penalty_timers_add(struct penalty_timers *pt, const void *for_object, int t then = now + timeout; - /* timer already running for that BTS? */ - llist_for_each_entry(timer, &pt->timers, entry) { - if (timer->for_object != for_object) + /* timer already running for that target cell? */ + llist_for_each_entry(timer, penalty_timers, entry) { + if (!gsm0808_cell_ids_match(&timer->for_target_cell, for_target_cell, true)) continue; /* raise, if running timer will timeout earlier or has timed * out already, otherwise keep later timeout */ @@ -79,24 +67,49 @@ void penalty_timers_add(struct penalty_timers *pt, const void *for_object, int t } /* add new timer */ - timer = talloc_zero(pt, struct penalty_timer); + timer = talloc_zero(ctx, struct penalty_timer); if (!timer) return; - timer->for_object = for_object; + timer->for_target_cell = *for_target_cell; timer->timeout = then; - llist_add_tail(&timer->entry, &pt->timers); + llist_add_tail(&timer->entry, penalty_timers); } -unsigned int penalty_timers_remaining(struct penalty_timers *pt, const void *for_object) +/* Add a penalty timer for each target cell ID in the given list. + * \param ctx talloc context to allocate new struct penalty_timer from. + * \param penalty_timers llist head to add penalty timer to. + * \param for_target_cells Which handover targets to penalize. + * \param timeout Penalty time in seconds. + */ +void penalty_timers_add_list(void *ctx, struct llist_head *penalty_timers, + const struct gsm0808_cell_id_list2 *for_target_cells, int timeout) +{ + int i; + for (i = 0; i < for_target_cells->id_list_len; i++) { + struct gsm0808_cell_id add = { + .id_discr = for_target_cells->id_discr, + .id = for_target_cells->id_list[i], + }; + penalty_timers_add(ctx, penalty_timers, &add, timeout); + } +} + +/* Return the amount of penalty time in seconds remaining for a target cell. + * \param penalty_timers llist head to look up penalty time in. + * \param for_target_cell Which handover target to query. + * \returns seconds remaining until all penalty time has expired. + */ +unsigned int penalty_timers_remaining(struct llist_head *penalty_timers, + const struct gsm0808_cell_id *for_target_cell) { struct penalty_timer *timer; unsigned int now = time_now(); unsigned int max_remaining = 0; - llist_for_each_entry(timer, &pt->timers, entry) { + llist_for_each_entry(timer, penalty_timers, entry) { unsigned int remaining; - if (timer->for_object != for_object) + if (!gsm0808_cell_ids_match(&timer->for_target_cell, for_target_cell, true)) continue; if (now >= timer->timeout) continue; @@ -107,23 +120,39 @@ unsigned int penalty_timers_remaining(struct penalty_timers *pt, const void *for return max_remaining; } -void penalty_timers_clear(struct penalty_timers *pt, const void *for_object) +/* Return the largest amount of penalty time in seconds remaining for any one of the given target cells. + * Call penalty_timers_remaining() for each entry of for_target_cells and return the largest value encountered. + * \param penalty_timers llist head to look up penalty time in. + * \param for_target_cells Which handover targets to query. + * \returns seconds remaining until all penalty time has expired. + */ +unsigned int penalty_timers_remaining_list(struct llist_head *penalty_timers, + const struct gsm0808_cell_id_list2 *for_target_cells) +{ + int i; + unsigned int max_remaining = 0; + for (i = 0; i < for_target_cells->id_list_len; i++) { + unsigned int remaining; + struct gsm0808_cell_id query = { + .id_discr = for_target_cells->id_discr, + .id = for_target_cells->id_list[i], + }; + remaining = penalty_timers_remaining(penalty_timers, &query); + max_remaining = OSMO_MAX(max_remaining, remaining); + } + return max_remaining; +} + +/* Clear penalty timers for one target cell, or completely clear the entire list. + * \param penalty_timers llist head to add penalty timer to. + * \param for_target_cell Which handover target to clear timers for, or NULL to clear all timers. */ +void penalty_timers_clear(struct llist_head *penalty_timers, const struct gsm0808_cell_id *for_target_cell) { struct penalty_timer *timer, *timer2; - llist_for_each_entry_safe(timer, timer2, &pt->timers, entry) { - if (for_object && timer->for_object != for_object) + llist_for_each_entry_safe(timer, timer2, penalty_timers, entry) { + if (for_target_cell && !gsm0808_cell_ids_match(&timer->for_target_cell, for_target_cell, true)) continue; llist_del(&timer->entry); talloc_free(timer); } } - -void penalty_timers_free(struct penalty_timers **pt_p) -{ - struct penalty_timers *pt = *pt_p; - if (!pt) - return; - penalty_timers_clear(pt, NULL); - talloc_free(pt); - *pt_p = NULL; -} diff --git a/tests/handover/handover_test.c b/tests/handover/handover_test.c index dff1390d8..332c94cd1 100644 --- a/tests/handover/handover_test.c +++ b/tests/handover/handover_test.c @@ -205,6 +205,7 @@ const char * const bts_default_ts[] = { static struct gsm_bts *_create_bts(int num_trx, const char * const *ts_args, int ts_args_count) { static int arfcn = 870; + static int ci = 0; struct gsm_bts *bts; struct e1inp_sign_link *rsl_link; int i; @@ -220,6 +221,7 @@ static struct gsm_bts *_create_bts(int num_trx, const char * const *ts_args, int } bts->location_area_code = 23; + bts->cell_identity = ci++; bts->c0->arfcn = arfcn++; bts->codec.efr = 1; -- cgit v1.2.3