osmocom-net-gprs July 2013

osmocom-net-gprs@lists.osmocom.org

6 participants
5 discussions

by Holger Hans Peter Freyther

Dear Ivan, all, I started to do work on the PCU and didn't compile C++ for a long time. I was surprised how long it takes to compile the code. From my point of view we are taking the hit of g++ without really using any of the features that C++ vould provide (e.g. no stack variables with automatic cleanup..).

10 years, 9 months

[PATCH 1/9] misc: Ignore the two osmo-pcu binaries and files created by vim

by Holger Hans Peter Freyther

From: Holger Hans Peter Freyther <holger(a)moiji-mobile.com> --- .gitignore | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/.gitignore b/.gitignore index 032b9cf..4f418d8 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,7 @@ *.o *.lo *.a +*.sw? Makefile.in Makefile .deps @@ -22,3 +23,7 @@ core core.* osmoappdesc.pyc + +# binaries +src/osmo-pcu +src/osmo-pcu-remote -- 1.8.3.2

10 years, 9 months

[PATCH 1/8] misc: Introduce a struct osmo_pcu and move things into it.

by Holger Freyther

From: Holger Hans Peter Freyther <holger(a)moiji-mobile.com> One of the issues with not properly re-setting everything is that due the global state it is not clear which variables belong together and how long it exists. Begin with creating a osmo_pcu and moving things into this class. Think of an organic cell, this commit is introducing the cell wall around it... and defines what is inside and what is outside of it. --- src/gprs_bssgp_pcu.cpp | 56 ++++++++++++++++++++++++++++---------------------- 1 file changed, 32 insertions(+), 24 deletions(-) diff --git a/src/gprs_bssgp_pcu.cpp b/src/gprs_bssgp_pcu.cpp index 59185e3..e94b5eb 100644 --- a/src/gprs_bssgp_pcu.cpp +++ b/src/gprs_bssgp_pcu.cpp @@ -21,13 +21,21 @@ #include <gprs_bssgp_pcu.h> #include <pcu_l1_if.h> +struct osmo_pcu { + struct gprs_nsvc *nsvc; + + int bvc_sig_reset; + int bvc_reset; + int bvc_unblocked; + int exit_on_destroy; +}; + +static struct osmo_pcu the_pcu = { 0, }; + struct sgsn_instance *sgsn; extern void *tall_pcu_ctx; struct bssgp_bvc_ctx *bctx = NULL; -struct gprs_nsvc *nsvc = NULL; -static int bvc_sig_reset = 0, bvc_reset = 0, bvc_unblocked = 0; extern uint16_t spoof_mcc, spoof_mnc; -static int exit_on_destroy = 0; struct osmo_timer_list bvc_timer; @@ -353,10 +361,10 @@ int gprs_bssgp_pcu_rx_sign(struct msgb *msg, struct tlv_parsed *tp, struct bssgp break; case BSSGP_PDUT_BVC_RESET_ACK: LOGP(DBSSGP, LOGL_DEBUG, "rx BSSGP_PDUT_BVC_RESET_ACK\n"); - if (!bvc_sig_reset) - bvc_sig_reset = 1; + if (!the_pcu.bvc_sig_reset) + the_pcu.bvc_sig_reset = 1; else - bvc_reset = 1; + the_pcu.bvc_reset = 1; bvc_timeout(NULL); break; case BSSGP_PDUT_PAGING_PS: @@ -380,7 +388,7 @@ int gprs_bssgp_pcu_rx_sign(struct msgb *msg, struct tlv_parsed *tp, struct bssgp break; case BSSGP_PDUT_BVC_UNBLOCK_ACK: LOGP(DBSSGP, LOGL_DEBUG, "rx BSSGP_PDUT_BVC_UNBLOCK_ACK\n"); - bvc_unblocked = 1; + the_pcu.bvc_unblocked = 1; bvc_timeout(NULL); break; case BSSGP_PDUT_SGSN_INVOKE_TRACE: @@ -496,7 +504,7 @@ static int nsvc_signal_cb(unsigned int subsys, unsigned int signal, return -EINVAL; nssd = (struct ns_signal_data *)signal_data; - if (nssd->nsvc != nsvc) { + if (nssd->nsvc != the_pcu.nsvc) { LOGP(DPCU, LOGL_ERROR, "Signal received of unknown NSVC\n"); return -EINVAL; } @@ -506,10 +514,10 @@ static int nsvc_signal_cb(unsigned int subsys, unsigned int signal, if (!nsvc_unblocked) { nsvc_unblocked = 1; LOGP(DPCU, LOGL_NOTICE, "NS-VC %d is unblocked.\n", - nsvc->nsvci); - bvc_sig_reset = 0; - bvc_reset = 0; - bvc_unblocked = 0; + the_pcu.nsvc->nsvci); + the_pcu.bvc_sig_reset = 0; + the_pcu.bvc_reset = 0; + the_pcu.bvc_unblocked = 0; bvc_timeout(NULL); } break; @@ -517,9 +525,9 @@ static int nsvc_signal_cb(unsigned int subsys, unsigned int signal, if (nsvc_unblocked) { nsvc_unblocked = 0; osmo_timer_del(&bvc_timer); - bvc_sig_reset = 0; - bvc_reset = 0; - bvc_unblocked = 0; + the_pcu.bvc_sig_reset = 0; + the_pcu.bvc_reset = 0; + the_pcu.bvc_unblocked = 0; LOGP(DPCU, LOGL_NOTICE, "NS-VC is blocked.\n"); } break; @@ -545,14 +553,14 @@ static void bvc_timeout(void *_priv) { struct gprs_rlcmac_bts *bts = gprs_rlcmac_bts; - if (!bvc_sig_reset) { + if (!the_pcu.bvc_sig_reset) { LOGP(DBSSGP, LOGL_INFO, "Sending reset on BVCI 0\n"); bssgp_tx_bvc_reset(bctx, 0, BSSGP_CAUSE_OML_INTERV); osmo_timer_schedule(&bvc_timer, 1, 0); return; } - if (!bvc_reset) { + if (!the_pcu.bvc_reset) { LOGP(DBSSGP, LOGL_INFO, "Sending reset on BVCI %d\n", bctx->bvci); bssgp_tx_bvc_reset(bctx, bctx->bvci, BSSGP_CAUSE_OML_INTERV); @@ -560,7 +568,7 @@ static void bvc_timeout(void *_priv) return; } - if (!bvc_unblocked) { + if (!the_pcu.bvc_unblocked) { LOGP(DBSSGP, LOGL_INFO, "Sending unblock on BVCI %d\n", bctx->bvci); bssgp_tx_bvc_unblock(bctx); @@ -609,8 +617,8 @@ int gprs_bssgp_create(uint16_t local_port, uint32_t sgsn_ip, dest.sin_port = htons(sgsn_port); dest.sin_addr.s_addr = htonl(sgsn_ip); - nsvc = gprs_ns_nsip_connect(bssgp_nsi, &dest, nsei, nsvci); - if (!nsvc) { + the_pcu.nsvc = gprs_ns_nsip_connect(bssgp_nsi, &dest, nsei, nsvci); + if (!the_pcu.nsvc) { LOGP(DBSSGP, LOGL_ERROR, "Failed to create NSVCt\n"); gprs_ns_destroy(bssgp_nsi); bssgp_nsi = NULL; @@ -620,7 +628,7 @@ int gprs_bssgp_create(uint16_t local_port, uint32_t sgsn_ip, bctx = btsctx_alloc(bvci, nsei); if (!bctx) { LOGP(DBSSGP, LOGL_ERROR, "Failed to create BSSGP context\n"); - nsvc = NULL; + the_pcu.nsvc = NULL; gprs_ns_destroy(bssgp_nsi); bssgp_nsi = NULL; return -EINVAL; @@ -643,7 +651,7 @@ int gprs_bssgp_create(uint16_t local_port, uint32_t sgsn_ip, void gprs_bssgp_destroy_or_exit(void) { - if (exit_on_destroy) { + if (the_pcu.exit_on_destroy) { LOGP(DBSSGP, LOGL_NOTICE, "Exiting on BSSGP destruction.\n"); exit(0); } @@ -655,7 +663,7 @@ void gprs_bssgp_destroy_or_exit(void) osmo_signal_unregister_handler(SS_L_NS, nsvc_signal_cb, NULL); - nsvc = NULL; + the_pcu.nsvc = NULL; /* FIXME: move this to libgb: btsctx_free() */ llist_del(&bctx->list); @@ -671,5 +679,5 @@ void gprs_bssgp_destroy_or_exit(void) void gprs_bssgp_exit_on_destroy(void) { LOGP(DBSSGP, LOGL_NOTICE, "Going to quit on BSSGP destruction\n"); - exit_on_destroy = 1; + the_pcu.exit_on_destroy = 1; } -- 1.8.3.2

10 years, 9 months

[PATCH 1/2] misc: Remove if (timer_pending) stop_timer idiom from the code

by Holger Hans Peter Freyther

osmo_timer_del is an idempotent operation. There is no requirement to check if it is running. If you don't want a timer to run, delete it. Maybe one should have called the method _unschedule, _cancel to make this more clear. --- src/gprs_bssgp_pcu.cpp | 6 ++---- src/sysmo_sock.cpp | 3 +-- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/src/gprs_bssgp_pcu.cpp b/src/gprs_bssgp_pcu.cpp index 54753b8..c791913 100644 --- a/src/gprs_bssgp_pcu.cpp +++ b/src/gprs_bssgp_pcu.cpp @@ -515,8 +515,7 @@ static int nsvc_signal_cb(unsigned int subsys, unsigned int signal, case S_NS_BLOCK: if (nsvc_unblocked) { nsvc_unblocked = 0; - if (osmo_timer_pending(&bvc_timer)) - osmo_timer_del(&bvc_timer); + osmo_timer_del(&bvc_timer); bvc_sig_reset = 0; bvc_reset = 0; bvc_unblocked = 0; @@ -646,8 +645,7 @@ void gprs_bssgp_destroy(void) if (!bssgp_nsi) return; - if (osmo_timer_pending(&bvc_timer)) - osmo_timer_del(&bvc_timer); + osmo_timer_del(&bvc_timer); osmo_signal_unregister_handler(SS_L_NS, nsvc_signal_cb, NULL); diff --git a/src/sysmo_sock.cpp b/src/sysmo_sock.cpp index a4cc6de..d4fb5a6 100644 --- a/src/sysmo_sock.cpp +++ b/src/sysmo_sock.cpp @@ -304,8 +304,7 @@ void pcu_l1if_close(void) if (!state) return; - if (osmo_timer_pending(&state->timer)) - osmo_timer_del(&state->timer); + osmo_timer_del(&state->timer); bfd = &state->conn_bfd; if (bfd->fd > 0) -- 1.8.3.2

10 years, 9 months

Weird usage of gprs_ns_vty_init from gprs_bssgp_create

by Holger Hans Peter Freyther

Dear Andreas, Ivan, when trying to understand the CPU usage of the PCU without having any subscribers I noticed the following backtrace: #0 _int_malloc (av=0x402fe4d4, bytes=52) at malloc.c:3241 #1 0x4023a3b8 in __GI___libc_malloc (bytes=52) at malloc.c:2859 #2 0x4929b6e8 in _talloc_zero () from /usr/lib/libosmocore.so.4 #3 0x49422e44 in vector_init () from /usr/lib/libosmovty.so.0 #4 0x4941f1f8 in install_element () from /usr/lib/libosmovty.so.0 #5 0x492e602c in gprs_ns_vty_init () from /usr/lib/libosmogb.so.2 #6 0x0001b670 in gprs_bssgp_create (local_port=<optimized out>, sgsn_ip=184194049, sgsn_port=<optimized out>, nsei=<optimized out>, nsvci=2, bvci=2, mcc=<optimized out>, mnc=<optimized out>, lac=1, rac=0, cell_id=0) at gprs_bssgp_pcu.cpp:585 #7 0x00016990 in pcu_rx_info_ind (info_ind=0x7) at pcu_l1_if.cpp:431 #8 pcu_rx (msg_type=<optimized out>, pcu_prim=0x3) at pcu_l1_if.cpp:577 #9 0x00017914 in pcu_sock_read (bfd=0x6a3f0) at sysmo_sock.cpp:151 #10 pcu_sock_cb (bfd=0x6a3f0, flags=1) at sysmo_sock.cpp:218 #11 0x49294ff4 in osmo_select_main () from /usr/lib/libosmocore.so.4 #12 0x0000a668 in main (argc=<optimized out>, argv=0xbee40b04) at pcu_main.cpp:219 This means when a info indication is received gprs_bssgp_create will be called which will re-initialize the VTY commands but more important re-set vty_nsi inside the VTY code of libosmogb. But it also means that vty_nsi will be a dangling pointer in case of 1.) gprs_ns_nsip_connect, btsctx_alloc, gprs_ns_nsip_listen fail 2.) Between the sysmobts software exiting and re-starting and bringing OML back up 3.) More cases that I might not understand. The dangling pointer will lead to a crash when the VTY of the PCU is used in such situation/window. does any of you care to fix that race? I think the easiest would be to just close/open/bind/listen/connect on the NS once we get a new config. holger -- - Holger Freyther <hfreyther(a)sysmocom.de> http://www.sysmocom.de/ ======================================================================= * sysmocom - systems for mobile communications GmbH * Schivelbeiner Str. 5 * 10439 Berlin, Germany * Sitz / Registered office: Berlin, HRB 134158 B * Geschaeftsfuehrer / Managing Directors: Holger Freyther, Harald Welte

10 years, 10 months

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

osmocom-net-gprs July 2013