pespin has submitted this change. ( https://gerrit.osmocom.org/c/osmo-bsc/+/31940 ) Change subject: Fix Lb/A SCCP conn lookup after recent regression in optimization patch ...................................................................... Fix Lb/A SCCP conn lookup after recent regression in optimization patch In osmo-bsc, there's currently 0..1 Lb links and 0..N A links, where N is the number of MSC, but links can be shared in the underlaying stack (struct osmo_sccp_instance), hence range 0..N of different osmo_sccp_instance (identified by PC). Even more, the Lb and A link can share the same underlaying stack, so osmo-bsc can end up with only 1 struct osmo_sccp_instance shared by all the above mentioned links in case all are configured under the same PC. Total range A+Lb is 0..(1+N). A struct gsm_subscriber_conn stores 2 struct sccp_instance*, one for Lb (conn->lcs.lb.*)and one for A (conn->sccp.*). They can actually point to the same sccp_instance or to different ones, as explained above, depending on the configured setup. In any case, a gsm_subscriber_conn needs 2 rb_nodes since it can hold any of the 2 conn_ids independently (A or Lb). The previous patch forgot to add that 2nd rb_node as well as some initialization and release code for the Lb conn. This patch addresses that. When the 2nd rb_node, a problem when iterating the rbtree appears: how to find out the "conn" pointer from the rb_node pointer, since the rb_node pointer can be any of the 2 rb_nodes inside the struct at a different offsets. In order to solve that problem, a new struct bscp_sccp_conn_node is added, which holds all the relevant information used by the rbtree lookup code in a generic way (rb_node and conn_id), plus a backpointer to the struct bsc_gsm_subcriber it relates too. Fixes: 85062ccad31e9fb73e0256a5ee556c6ae0a16449 Change-Id: If42d93adee71d646766929a09bc01ae92b734ef3 --- M include/osmocom/bsc/gsm_data.h M src/osmo-bsc/bsc_sccp.c M src/osmo-bsc/bsc_subscr_conn_fsm.c M src/osmo-bsc/bsc_vty.c M src/osmo-bsc/handover_fsm.c M src/osmo-bsc/lb.c M src/osmo-bsc/lchan_rtp_fsm.c M src/osmo-bsc/osmo_bsc_sigtran.c M tests/bsc/bsc_test.c 9 files changed, 125 insertions(+), 58 deletions(-) Approvals: laforge: Looks good to me, but someone else must approve pespin: Looks good to me, approved Jenkins Builder: Verified diff --git a/include/osmocom/bsc/gsm_data.h b/include/osmocom/bsc/gsm_data.h index dc7ee3f..25e634a 100644 --- a/include/osmocom/bsc/gsm_data.h +++ b/include/osmocom/bsc/gsm_data.h @@ -281,6 +281,21 @@ struct osmo_mgcpc_ep_ci *created_ci_for_msc; }; +struct gsm_subscriber_connection; + +struct bscp_sccp_conn_node { + /* entry in (struct bsc_sccp_inst)->connections */ + struct rb_node node; + /* Sigtran connection ID: + * if set: Range (0..SCCP_CONN_ID_MAX) (24 bit) + * if unset: SCCP_CONN_ID_UNSET (-1) if unset */ + uint32_t conn_id; + /* backpointer where this sccp conn belongs to: */ + struct gsm_subscriber_connection *gscon; +}; + +void bscp_sccp_conn_node_init(struct bscp_sccp_conn_node *sccp_conn, struct gsm_subscriber_connection *gscon); + /* active radio connection of a mobile subscriber */ struct gsm_subscriber_connection { /* global linked list of subscriber_connections */ @@ -332,15 +347,8 @@ struct { /* SCCP connection related */ struct bsc_msc_data *msc; - - /* entry in (struct bsc_sccp_inst)->connections */ - struct rb_node node; - - /* Sigtran connection ID: - * if set: Range (0..SCCP_CONN_ID_MAX) (24 bit) - * if unset: SCCP_CONN_ID_UNSET (-1) if unset */ - uint32_t conn_id; enum subscr_sccp_state state; + struct bscp_sccp_conn_node conn; } sccp; /* for audio handling */ @@ -389,11 +397,8 @@ /* Lb interface to the SMLC: BSSMAP-LE/SCCP connection associated with this subscriber */ struct { - /* Sigtran connection ID: - * if set: Range (0..SCCP_CONN_ID_MAX) (24 bit) - * if unset: SCCP_CONN_ID_UNSET (-1) if unset */ - uint32_t conn_id; enum subscr_sccp_state state; + struct bscp_sccp_conn_node conn; } lb; } lcs; @@ -885,8 +890,8 @@ struct bsc_sccp_inst *bsc_sccp_inst_alloc(void *ctx); uint32_t bsc_sccp_inst_next_conn_id(struct bsc_sccp_inst *bsc_sccp); -int bsc_sccp_inst_register_gscon(struct bsc_sccp_inst *bsc_sccp, struct gsm_subscriber_connection *conn); -void bsc_sccp_inst_unregister_gscon(struct bsc_sccp_inst *bsc_sccp, struct gsm_subscriber_connection *conn); +int bsc_sccp_inst_register_gscon(struct bsc_sccp_inst *bsc_sccp, struct bscp_sccp_conn_node *sccp_conn); +void bsc_sccp_inst_unregister_gscon(struct bsc_sccp_inst *bsc_sccp, struct bscp_sccp_conn_node *sccp_conn); struct gsm_subscriber_connection *bsc_sccp_inst_get_gscon_by_conn_id(const struct bsc_sccp_inst *bsc_sccp, uint32_t conn_id); struct gsm_network { diff --git a/src/osmo-bsc/bsc_sccp.c b/src/osmo-bsc/bsc_sccp.c index 6fa1d4b..323d7fd 100644 --- a/src/osmo-bsc/bsc_sccp.c +++ b/src/osmo-bsc/bsc_sccp.c @@ -27,6 +27,12 @@ #include <osmocom/bsc/bsc_msc_data.h> #include <osmocom/bsc/lb.h> +void bscp_sccp_conn_node_init(struct bscp_sccp_conn_node *sccp_conn, struct gsm_subscriber_connection *gscon) +{ + sccp_conn->conn_id = SCCP_CONN_ID_UNSET; + sccp_conn->gscon = gscon; +} + struct bsc_sccp_inst *bsc_sccp_inst_alloc(void *ctx) { struct bsc_sccp_inst *bsc_sccp; @@ -38,23 +44,21 @@ return bsc_sccp; } -int bsc_sccp_inst_register_gscon(struct bsc_sccp_inst *bsc_sccp, struct gsm_subscriber_connection *conn) +int bsc_sccp_inst_register_gscon(struct bsc_sccp_inst *bsc_sccp, struct bscp_sccp_conn_node *sccp_conn) { struct rb_node **n = &(bsc_sccp->connections.rb_node); struct rb_node *parent = NULL; - uint32_t conn_id = conn->sccp.conn_id; + uint32_t conn_id = sccp_conn->conn_id; OSMO_ASSERT(conn_id != SCCP_CONN_ID_UNSET); while (*n) { - struct gsm_subscriber_connection *it; - - it = container_of(*n, struct gsm_subscriber_connection, sccp.node); + struct bscp_sccp_conn_node *it = container_of(*n, struct bscp_sccp_conn_node, node); parent = *n; - if (conn_id < it->sccp.conn_id) { + if (conn_id < it->conn_id) { n = &((*n)->rb_left); - } else if (conn_id > it->sccp.conn_id) { + } else if (conn_id > it->conn_id) { n = &((*n)->rb_right); } else { LOGP(DMSC, LOGL_ERROR, @@ -63,35 +67,34 @@ } } - rb_link_node(&conn->sccp.node, parent, n); - rb_insert_color(&conn->sccp.node, &bsc_sccp->connections); + rb_link_node(&sccp_conn->node, parent, n); + rb_insert_color(&sccp_conn->node, &bsc_sccp->connections); return 0; } -void bsc_sccp_inst_unregister_gscon(struct bsc_sccp_inst *bsc_sccp, struct gsm_subscriber_connection *conn) +void bsc_sccp_inst_unregister_gscon(struct bsc_sccp_inst *bsc_sccp, struct bscp_sccp_conn_node *sccp_conn) { - OSMO_ASSERT(conn->sccp.conn_id != SCCP_CONN_ID_UNSET); - rb_erase(&conn->sccp.node, &bsc_sccp->connections); + OSMO_ASSERT(sccp_conn->conn_id != SCCP_CONN_ID_UNSET); + rb_erase(&sccp_conn->node, &bsc_sccp->connections); } /* Helper function to Check if the given connection id is already assigned */ struct gsm_subscriber_connection *bsc_sccp_inst_get_gscon_by_conn_id(const struct bsc_sccp_inst *bsc_sccp, uint32_t conn_id) { const struct rb_node *node = bsc_sccp->connections.rb_node; - struct gsm_subscriber_connection *conn; OSMO_ASSERT(conn_id != SCCP_CONN_ID_UNSET); /* Range (0..SCCP_CONN_ID_MAX) expected, see bsc_sccp_inst_next_conn_id() */ OSMO_ASSERT(conn_id <= SCCP_CONN_ID_MAX); while (node) { - conn = container_of(node, struct gsm_subscriber_connection, sccp.node); - if (conn_id < conn->sccp.conn_id) + struct bscp_sccp_conn_node *sccp_conn = container_of(node, struct bscp_sccp_conn_node, node); + if (conn_id < sccp_conn->conn_id) node = node->rb_left; - else if (conn_id > conn->sccp.conn_id) + else if (conn_id > sccp_conn->conn_id) node = node->rb_right; else - return conn; + return sccp_conn->gscon; } return NULL; diff --git a/src/osmo-bsc/bsc_subscr_conn_fsm.c b/src/osmo-bsc/bsc_subscr_conn_fsm.c index 8e21992..de9d4d2 100644 --- a/src/osmo-bsc/bsc_subscr_conn_fsm.c +++ b/src/osmo-bsc/bsc_subscr_conn_fsm.c @@ -769,7 +769,7 @@ mgw_info = (struct mgcp_conn_peer){ .port = port, - .call_id = conn->sccp.conn_id, + .call_id = conn->sccp.conn.conn_id, .ptime = 20, .x_osmo_osmux_use = conn->assignment.req.use_osmux, .x_osmo_osmux_cid = conn->assignment.req.osmux_cid, @@ -1108,13 +1108,13 @@ LOGPFSML(fi, LOGL_DEBUG, "Disconnecting SCCP\n"); struct bsc_msc_data *msc = conn->sccp.msc; /* FIXME: include a proper cause value / error message? */ - osmo_sccp_tx_disconn(msc->a.sccp_user, conn->sccp.conn_id, &msc->a.bsc_addr, 0); + osmo_sccp_tx_disconn(msc->a.sccp_user, conn->sccp.conn.conn_id, &msc->a.bsc_addr, 0); conn->sccp.state = SUBSCR_SCCP_ST_NONE; } - if (conn->sccp.conn_id != SCCP_CONN_ID_UNSET && conn->sccp.msc) { + if (conn->sccp.conn.conn_id != SCCP_CONN_ID_UNSET && conn->sccp.msc) { struct bsc_sccp_inst *bsc_sccp = osmo_sccp_get_priv(conn->sccp.msc->a.sccp); - bsc_sccp_inst_unregister_gscon(bsc_sccp, conn); - conn->sccp.conn_id = SCCP_CONN_ID_UNSET; + bsc_sccp_inst_unregister_gscon(bsc_sccp, &conn->sccp.conn); + conn->sccp.conn.conn_id = SCCP_CONN_ID_UNSET; } if (conn->bsub) { @@ -1231,7 +1231,8 @@ conn->network = net; INIT_LLIST_HEAD(&conn->dtap_queue); INIT_LLIST_HEAD(&conn->hodec2.penalty_timers); - conn->sccp.conn_id = SCCP_CONN_ID_UNSET; + bscp_sccp_conn_node_init(&conn->sccp.conn, conn); + bscp_sccp_conn_node_init(&conn->lcs.lb.conn, conn); /* Default clear cause (on RR translates to GSM48_RR_CAUSE_ABNORMAL_UNSPEC) */ conn->clear_cause = GSM0808_CAUSE_EQUIPMENT_FAILURE; @@ -1404,7 +1405,7 @@ { osmo_fsm_inst_update_id_f(conn->fi, "msc%u-conn%u%s%s", conn->sccp.msc ? conn->sccp.msc->nr : UINT_MAX, - conn->sccp.conn_id, + conn->sccp.conn.conn_id, conn->bsub? "_" : "", conn->bsub? bsc_subscr_id(conn->bsub) : ""); } diff --git a/src/osmo-bsc/bsc_vty.c b/src/osmo-bsc/bsc_vty.c index e860888..8eff8de 100644 --- a/src/osmo-bsc/bsc_vty.c +++ b/src/osmo-bsc/bsc_vty.c @@ -727,7 +727,7 @@ static void dump_one_subscr_conn(struct vty *vty, const struct gsm_subscriber_connection *conn) { vty_out(vty, "conn ID=%u, MSC=%u, hodec2_fail=%d, mgw_ep=%s%s", - conn->sccp.conn_id, conn->sccp.msc->nr, conn->hodec2.failures, + conn->sccp.conn.conn_id, conn->sccp.msc->nr, conn->hodec2.failures, osmo_mgcpc_ep_name(conn->user_plane.mgw_endpoint), VTY_NEWLINE); if (conn->lcls.global_call_ref_len) { vty_out(vty, " LCLS GCR: %s%s", diff --git a/src/osmo-bsc/handover_fsm.c b/src/osmo-bsc/handover_fsm.c index ea07f08..24766a5 100644 --- a/src/osmo-bsc/handover_fsm.c +++ b/src/osmo-bsc/handover_fsm.c @@ -269,7 +269,7 @@ if (conn->fi->id) osmo_fsm_inst_update_id_f(fi, "%s_%s", label, conn->fi->id); else - osmo_fsm_inst_update_id_f(fi, "%s_conn%u", label, conn->sccp.conn_id); + osmo_fsm_inst_update_id_f(fi, "%s_conn%u", label, conn->sccp.conn.conn_id); } static void handover_reset(struct gsm_subscriber_connection *conn) diff --git a/src/osmo-bsc/lb.c b/src/osmo-bsc/lb.c index 6c6f599..72905b8 100644 --- a/src/osmo-bsc/lb.c +++ b/src/osmo-bsc/lb.c @@ -199,6 +199,8 @@ scu_prim->u.disconnect.conn_id); rc = -EINVAL; } else { + bsc_sccp_inst_unregister_gscon(bsc_sccp, &conn->lcs.lb.conn); + conn->lcs.lb.conn.conn_id = SCCP_CONN_ID_UNSET; conn->lcs.lb.state = SUBSCR_SCCP_ST_NONE; if (msgb_l2len(oph->msg) > 0) { rc = lcs_loc_req_rx_bssmap_le(conn, oph->msg); @@ -237,7 +239,11 @@ LOGPFSMSL(conn->fi, DLCS, LOGL_ERROR, "Unable to allocate SCCP Connection ID for BSSMAP-LE to SMLC\n"); return -ENOSPC; } - conn->lcs.lb.conn_id = conn_id; + conn->lcs.lb.conn.conn_id = conn_id; + if (bsc_sccp_inst_register_gscon(bsc_sccp, &conn->lcs.lb.conn) < 0) { + LOGP(DMSC, LOGL_ERROR, "Unable to register Lb SCCP connection (id=%u)\n", conn->lcs.lb.conn.conn_id); + return -1; + } ss7 = osmo_ss7_instance_find(bsc_gsmnet->smlc->cs7_instance); OSMO_ASSERT(ss7); LOGPFSMSL(conn->fi, DLCS, LOGL_INFO, "Opening new SCCP connection (id=%u) to SMLC: %s\n", conn_id, @@ -245,22 +251,35 @@ rc = osmo_sccp_tx_conn_req_msg(bsc_gsmnet->smlc->sccp_user, conn_id, &bsc_gsmnet->smlc->bsc_addr, &bsc_gsmnet->smlc->smlc_addr, msg); - if (rc >= 0) + if (rc >= 0) { rate_ctr_inc(rate_ctr_group_get_ctr(bsc_gsmnet->smlc->ctrs, SMLC_CTR_BSSMAP_LE_TX_SUCCESS)); - else - rate_ctr_inc(rate_ctr_group_get_ctr(bsc_gsmnet->smlc->ctrs, SMLC_CTR_BSSMAP_LE_TX_ERR_SEND)); - if (rc >= 0) conn->lcs.lb.state = SUBSCR_SCCP_ST_WAIT_CONN_CONF; + } else { + rate_ctr_inc(rate_ctr_group_get_ctr(bsc_gsmnet->smlc->ctrs, SMLC_CTR_BSSMAP_LE_TX_ERR_SEND)); + goto failed_unregister_conn_id; + } return rc; + +failed_unregister_conn_id: + bsc_sccp_inst_unregister_gscon(bsc_sccp, &conn->lcs.lb.conn); + conn->lcs.lb.conn.conn_id = SCCP_CONN_ID_UNSET; + return rc; } void lb_close_conn(struct gsm_subscriber_connection *conn) { + struct bsc_sccp_inst *bsc_sccp; if (conn->lcs.lb.state == SUBSCR_SCCP_ST_NONE) return; - osmo_sccp_tx_disconn(bsc_gsmnet->smlc->sccp_user, conn->lcs.lb.conn_id, &bsc_gsmnet->smlc->bsc_addr, 0); + OSMO_ASSERT(conn->lcs.lb.conn.conn_id != SCCP_CONN_ID_UNSET); + + osmo_sccp_tx_disconn(bsc_gsmnet->smlc->sccp_user, conn->lcs.lb.conn.conn_id, &bsc_gsmnet->smlc->bsc_addr, 0); + conn->lcs.lb.state = SUBSCR_SCCP_ST_NONE; + bsc_sccp = osmo_sccp_get_priv(bsc_gsmnet->smlc->sccp); + bsc_sccp_inst_unregister_gscon(bsc_sccp, &conn->lcs.lb.conn); + conn->lcs.lb.conn.conn_id = SCCP_CONN_ID_UNSET; } /* Send data to SMLC, take ownership of *msg */ @@ -293,7 +312,7 @@ } LOGPFSMSL(conn->fi, DLCS, LOGL_DEBUG, "Tx %s\n", osmo_bssap_le_pdu_to_str_c(OTC_SELECT, bssap_le)); - rc = osmo_sccp_tx_data_msg(bsc_gsmnet->smlc->sccp_user, conn->lcs.lb.conn_id, msg); + rc = osmo_sccp_tx_data_msg(bsc_gsmnet->smlc->sccp_user, conn->lcs.lb.conn.conn_id, msg); if (rc >= 0) rate_ctr_inc(rate_ctr_group_get_ctr(bsc_gsmnet->smlc->ctrs, SMLC_CTR_BSSMAP_LE_TX_SUCCESS)); else diff --git a/src/osmo-bsc/lchan_rtp_fsm.c b/src/osmo-bsc/lchan_rtp_fsm.c index c568a66..7e9c31f 100644 --- a/src/osmo-bsc/lchan_rtp_fsm.c +++ b/src/osmo-bsc/lchan_rtp_fsm.c @@ -169,7 +169,7 @@ .x_osmo_osmux_cid = -1, /* -1 is wildcard, .x_osmo_osmux_use set below */ }; if (lchan->conn) { - crcx_info.call_id = lchan->conn->sccp.conn_id; + crcx_info.call_id = lchan->conn->sccp.conn.conn_id; if (lchan->conn->sccp.msc) crcx_info.x_osmo_ign = lchan->conn->sccp.msc->x_osmo_ign; } diff --git a/src/osmo-bsc/osmo_bsc_sigtran.c b/src/osmo-bsc/osmo_bsc_sigtran.c index 1642059..8837330 100644 --- a/src/osmo-bsc/osmo_bsc_sigtran.c +++ b/src/osmo-bsc/osmo_bsc_sigtran.c @@ -170,8 +170,8 @@ if (!conn) return -ENOMEM; conn->sccp.msc = msc; - conn->sccp.conn_id = scu_prim->u.connect.conn_id; - if (bsc_sccp_inst_register_gscon(bsc_sccp, conn) < 0) { + conn->sccp.conn.conn_id = scu_prim->u.connect.conn_id; + if (bsc_sccp_inst_register_gscon(bsc_sccp, &conn->sccp.conn) < 0) { LOGP(DMSC, LOGL_NOTICE, "(calling_addr=%s conn_id=%u) N-CONNECT.ind failed registering conn\n", osmo_sccp_addr_dump(&scu_prim->u.connect.calling_addr), scu_prim->u.connect.conn_id); osmo_fsm_inst_term(conn->fi, OSMO_FSM_TERM_REQUEST, NULL); @@ -308,7 +308,7 @@ OSMO_ASSERT(conn); OSMO_ASSERT(msg); OSMO_ASSERT(conn->sccp.msc); - OSMO_ASSERT(conn->sccp.conn_id == SCCP_CONN_ID_UNSET); + OSMO_ASSERT(conn->sccp.conn.conn_id == SCCP_CONN_ID_UNSET); msc = conn->sccp.msc; @@ -318,16 +318,16 @@ } bsc_sccp = osmo_sccp_get_priv(msc->a.sccp); - conn->sccp.conn_id = conn_id = bsc_sccp_inst_next_conn_id(bsc_sccp); - if (conn->sccp.conn_id == SCCP_CONN_ID_UNSET) { + conn->sccp.conn.conn_id = conn_id = bsc_sccp_inst_next_conn_id(bsc_sccp); + if (conn->sccp.conn.conn_id == SCCP_CONN_ID_UNSET) { LOGP(DMSC, LOGL_ERROR, "Unable to allocate SCCP Connection ID\n"); return -1; } - if (bsc_sccp_inst_register_gscon(bsc_sccp, conn) < 0) { - LOGP(DMSC, LOGL_ERROR, "Unable to register SCCP connection (id=%u)\n", conn->sccp.conn_id); + if (bsc_sccp_inst_register_gscon(bsc_sccp, &conn->sccp.conn) < 0) { + LOGP(DMSC, LOGL_ERROR, "Unable to register SCCP connection (id=%u)\n", conn->sccp.conn.conn_id); return -1; } - LOGP(DMSC, LOGL_DEBUG, "Allocated new connection id: %u\n", conn->sccp.conn_id); + LOGP(DMSC, LOGL_DEBUG, "Allocated new connection id: %u\n", conn->sccp.conn.conn_id); ss7 = osmo_ss7_instance_find(msc->a.cs7_instance); OSMO_ASSERT(ss7); LOGP(DMSC, LOGL_INFO, "Opening new SCCP connection (id=%u) to MSC %d: %s\n", conn_id, @@ -390,7 +390,7 @@ return -EINVAL; } - conn_id = conn->sccp.conn_id; + conn_id = conn->sccp.conn.conn_id; ss7 = osmo_ss7_instance_find(msc->a.cs7_instance); OSMO_ASSERT(ss7); diff --git a/tests/bsc/bsc_test.c b/tests/bsc/bsc_test.c index 62757b4..dda10c3 100644 --- a/tests/bsc/bsc_test.c +++ b/tests/bsc/bsc_test.c @@ -212,7 +212,7 @@ conn = bsc_subscr_con_allocate(net); conn->lchan = lchan; conn->assignment.new_lchan = lchan; - conn->sccp.conn_id = 123; + conn->sccp.conn.conn_id = 123; conn->bsub = bsc_subscr_find_or_create_by_tmsi(net->bsc_subscribers, 0x423, "test"); gscon_update_id(conn); -- To view, visit https://gerrit.osmocom.org/c/osmo-bsc/+/31940 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: osmo-bsc Gerrit-Branch: master Gerrit-Change-Id: If42d93adee71d646766929a09bc01ae92b734ef3 Gerrit-Change-Number: 31940 Gerrit-PatchSet: 4 Gerrit-Owner: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel &lt;dwillmann(a)sysmocom.de&gt; Gerrit-Reviewer: laforge &lt;laforge(a)osmocom.org&gt; Gerrit-Reviewer: neels &lt;nhofmeyr(a)sysmocom.de&gt; Gerrit-Reviewer: osmith &lt;osmith(a)sysmocom.de&gt; Gerrit-Reviewer: pespin &lt;pespin(a)sysmocom.de&gt; Gerrit-MessageType: merged