cellular-infrastructure
/
osmo-bsc
mirror of https://gerrit.osmocom.org/osmo-bsc
9
0
Fork 0
Code Issues Releases Wiki Activity

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
This commit is contained in:
Neels Hofmeyr 2021-03-09 17:10:18 +01:00
parent 3ac1278dcf
commit d88f9a5384
9 changed files with 135 additions and 131 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
include/osmocom/bsc/bts.h
View File

@ -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);

6
include/osmocom/bsc/gsm_data.h
View File

@ -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

47
include/osmocom/bsc/penalty_timers.h
View File

@ -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 <osmocom/gsm/gsm0808_utils.h>
/* 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);

4
src/osmo-bsc/bsc_subscr_conn_fsm.c
View File

@ -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

28
src/osmo-bsc/bts.c
View File

@ -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;

63
src/osmo-bsc/handover_decision_2.c
View File

@ -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 = {

1
src/osmo-bsc/handover_fsm.c
View File

@ -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);

113
src/osmo-bsc/penalty_timers.c
View File

@ -28,16 +28,6 @@
#include <osmocom/bsc/penalty_timers.h>
#include <osmocom/bsc/gsm_data.h>
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;
}

2
tests/handover/handover_test.c
View File

@ -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;