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)
The PCU does not properly re-set the state when the connection to the BTS is lost (and the SGSN potentially is re-started during that). This results in the BSSGP BVCI > 1 remaining blocked and no data will be accepted by the SGSN.
Add the '-e' option and exit the PCU when the BSSGP/NS are getting destroyed. --- src/gprs_bssgp_pcu.cpp | 13 ++++++++++++- src/gprs_bssgp_pcu.h | 3 ++- src/openbts_sock.cpp | 2 +- src/pcu_l1_if.cpp | 2 +- src/pcu_main.cpp | 7 ++++++- src/sysmo_sock.cpp | 2 +- 6 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/src/gprs_bssgp_pcu.cpp b/src/gprs_bssgp_pcu.cpp index c791913..59185e3 100644 --- a/src/gprs_bssgp_pcu.cpp +++ b/src/gprs_bssgp_pcu.cpp @@ -27,6 +27,7 @@ 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;
@@ -640,8 +641,13 @@ int gprs_bssgp_create(uint16_t local_port, uint32_t sgsn_ip, return 0; }
-void gprs_bssgp_destroy(void) +void gprs_bssgp_destroy_or_exit(void) { + if (exit_on_destroy) { + LOGP(DBSSGP, LOGL_NOTICE, "Exiting on BSSGP destruction.\n"); + exit(0); + } + if (!bssgp_nsi) return;
@@ -662,3 +668,8 @@ void gprs_bssgp_destroy(void) bssgp_nsi = NULL; }
+void gprs_bssgp_exit_on_destroy(void) +{ + LOGP(DBSSGP, LOGL_NOTICE, "Going to quit on BSSGP destruction\n"); + exit_on_destroy = 1; +} diff --git a/src/gprs_bssgp_pcu.h b/src/gprs_bssgp_pcu.h index d669c3a..dc57e4f 100644 --- a/src/gprs_bssgp_pcu.h +++ b/src/gprs_bssgp_pcu.h @@ -56,6 +56,7 @@ int gprs_bssgp_create(uint16_t local_port, uint32_t sgsn_ip, uint16_t uint16_t mcc, uint16_t mnc, uint16_t lac, uint16_t rac, uint16_t cell_id);
-void gprs_bssgp_destroy(void); +void gprs_bssgp_exit_on_destroy(void); +void gprs_bssgp_destroy_or_exit(void);
#endif // GPRS_BSSGP_PCU_H diff --git a/src/openbts_sock.cpp b/src/openbts_sock.cpp index 845aa77..a09f834 100644 --- a/src/openbts_sock.cpp +++ b/src/openbts_sock.cpp @@ -179,7 +179,7 @@ int pcu_l1if_open()
void pcu_l1if_close(void) { - gprs_bssgp_destroy(); + gprs_bssgp_destroy_or_exit();
/* FIXME: cleanup l1if */ talloc_free(l1fh->fl1h); diff --git a/src/pcu_l1_if.cpp b/src/pcu_l1_if.cpp index 923070f..037cf60 100644 --- a/src/pcu_l1_if.cpp +++ b/src/pcu_l1_if.cpp @@ -369,7 +369,7 @@ bssgp_failed: trx, ts); } } - gprs_bssgp_destroy(); + gprs_bssgp_destroy_or_exit(); return 0; } LOGP(DL1IF, LOGL_INFO, "BTS available\n"); diff --git a/src/pcu_main.cpp b/src/pcu_main.cpp index 041831f6..754043f 100644 --- a/src/pcu_main.cpp +++ b/src/pcu_main.cpp @@ -60,6 +60,7 @@ static void print_help() "provided by BTS\n" " -r --realtime PRIO Use SCHED_RR with the specified " "priority\n" + " -e --exit Exit the application on disconnect\n" ); }
@@ -75,10 +76,11 @@ static void handle_options(int argc, char **argv) { "mnc", 1, 0, 'n' }, { "version", 0, 0, 'V' }, { "realtime", 1, 0, 'r' }, + { "exit", 0, 0, 'e' }, { 0, 0, 0, 0 } };
- c = getopt_long(argc, argv, "hc:m:n:Vr:", + c = getopt_long(argc, argv, "hc:m:n:Vr:e", long_options, &option_idx); if (c == -1) break; @@ -105,6 +107,9 @@ static void handle_options(int argc, char **argv) case 'r': rt_prio = atoi(optarg); break; + case 'e': + gprs_bssgp_exit_on_destroy(); + break; default: fprintf(stderr, "Unknown option '%c'\n", c); exit(0); diff --git a/src/sysmo_sock.cpp b/src/sysmo_sock.cpp index d4fb5a6..e116e5a 100644 --- a/src/sysmo_sock.cpp +++ b/src/sysmo_sock.cpp @@ -117,7 +117,7 @@ static void pcu_sock_close(struct pcu_sock_state *state, int lost) } }
- gprs_bssgp_destroy(); + gprs_bssgp_destroy_or_exit();
if (lost) { state->timer.cb = pcu_sock_timeout;
Hi Holger,
Sorry for slow response. Actually I think that we should implement more intellectual behavior of PCUfor handling restarts and disconnections of BTS and SGSN. I should try to find the way to solve this problem, but I will commit this patch, because it fixes current problems.
2013/7/13 Holger Hans Peter Freyther holger@moiji-mobile.com
The PCU does not properly re-set the state when the connection to the BTS is lost (and the SGSN potentially is re-started during that). This results in the BSSGP BVCI > 1 remaining blocked and no data will be accepted by the SGSN.
Add the '-e' option and exit the PCU when the BSSGP/NS are getting destroyed.
src/gprs_bssgp_pcu.cpp | 13 ++++++++++++- src/gprs_bssgp_pcu.h | 3 ++- src/openbts_sock.cpp | 2 +- src/pcu_l1_if.cpp | 2 +- src/pcu_main.cpp | 7 ++++++- src/sysmo_sock.cpp | 2 +- 6 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/src/gprs_bssgp_pcu.cpp b/src/gprs_bssgp_pcu.cpp index c791913..59185e3 100644 --- a/src/gprs_bssgp_pcu.cpp +++ b/src/gprs_bssgp_pcu.cpp @@ -27,6 +27,7 @@ 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;
@@ -640,8 +641,13 @@ int gprs_bssgp_create(uint16_t local_port, uint32_t sgsn_ip, return 0; }
-void gprs_bssgp_destroy(void) +void gprs_bssgp_destroy_or_exit(void) {
if (exit_on_destroy) {LOGP(DBSSGP, LOGL_NOTICE, "Exiting on BSSGPdestruction.\n");
exit(0);}if (!bssgp_nsi) return;@@ -662,3 +668,8 @@ void gprs_bssgp_destroy(void) bssgp_nsi = NULL; }
+void gprs_bssgp_exit_on_destroy(void) +{
LOGP(DBSSGP, LOGL_NOTICE, "Going to quit on BSSGP destruction\n");exit_on_destroy = 1;+} diff --git a/src/gprs_bssgp_pcu.h b/src/gprs_bssgp_pcu.h index d669c3a..dc57e4f 100644 --- a/src/gprs_bssgp_pcu.h +++ b/src/gprs_bssgp_pcu.h @@ -56,6 +56,7 @@ int gprs_bssgp_create(uint16_t local_port, uint32_t sgsn_ip, uint16_t uint16_t mcc, uint16_t mnc, uint16_t lac, uint16_t rac, uint16_t cell_id);
-void gprs_bssgp_destroy(void); +void gprs_bssgp_exit_on_destroy(void); +void gprs_bssgp_destroy_or_exit(void);
#endif // GPRS_BSSGP_PCU_H diff --git a/src/openbts_sock.cpp b/src/openbts_sock.cpp index 845aa77..a09f834 100644 --- a/src/openbts_sock.cpp +++ b/src/openbts_sock.cpp @@ -179,7 +179,7 @@ int pcu_l1if_open()
void pcu_l1if_close(void) {
gprs_bssgp_destroy();
gprs_bssgp_destroy_or_exit(); /* FIXME: cleanup l1if */ talloc_free(l1fh->fl1h);diff --git a/src/pcu_l1_if.cpp b/src/pcu_l1_if.cpp index 923070f..037cf60 100644 --- a/src/pcu_l1_if.cpp +++ b/src/pcu_l1_if.cpp @@ -369,7 +369,7 @@ bssgp_failed: trx, ts); } }
gprs_bssgp_destroy();
gprs_bssgp_destroy_or_exit(); return 0; } LOGP(DL1IF, LOGL_INFO, "BTS available\n");diff --git a/src/pcu_main.cpp b/src/pcu_main.cpp index 041831f6..754043f 100644 --- a/src/pcu_main.cpp +++ b/src/pcu_main.cpp @@ -60,6 +60,7 @@ static void print_help() "provided by BTS\n" " -r --realtime PRIO Use SCHED_RR with the specified " "priority\n"
" -e --exit Exit the application ondisconnect\n" ); }
@@ -75,10 +76,11 @@ static void handle_options(int argc, char **argv) { "mnc", 1, 0, 'n' }, { "version", 0, 0, 'V' }, { "realtime", 1, 0, 'r' },
{ "exit", 0, 0, 'e' }, { 0, 0, 0, 0 } };
c = getopt_long(argc, argv, "hc:m:n:Vr:",
c = getopt_long(argc, argv, "hc:m:n:Vr:e", long_options, &option_idx); if (c == -1) break;@@ -105,6 +107,9 @@ static void handle_options(int argc, char **argv) case 'r': rt_prio = atoi(optarg); break;
case 'e':gprs_bssgp_exit_on_destroy();break; default: fprintf(stderr, "Unknown option '%c'\n", c); exit(0);diff --git a/src/sysmo_sock.cpp b/src/sysmo_sock.cpp index d4fb5a6..e116e5a 100644 --- a/src/sysmo_sock.cpp +++ b/src/sysmo_sock.cpp @@ -117,7 +117,7 @@ static void pcu_sock_close(struct pcu_sock_state *state, int lost) } }
gprs_bssgp_destroy();
gprs_bssgp_destroy_or_exit(); if (lost) { state->timer.cb = pcu_sock_timeout;-- 1.8.3.2
On Thu, Jul 25, 2013 at 08:27:04PM +0400, Ivan Kluchnikov wrote:
Dear Ivan,
I should try to find the way to solve this problem, but I will commit this patch, because it fixes current problems.
Well, I have more commits pending for review but I would like to get the first bits in first instead of sending you a patch bomb. The 'root' cause appears to be that the state is in a couple of static fields that are throughout the file. I started to move them into a struct (an organic cell/entity). At that point it became clear of why NS/BSSGP is behaving like it does behave.
Once you merge the two changes, I will rebase and sent my cleanup patchset. And then we can look into properly re-setting the state.
kind regards holger
Hi Ivan,
On Thu, Jul 25, 2013 at 08:27:04PM +0400, Ivan Kluchnikov wrote:
Actually I think that we should implement more intellectual behavior of PCUfor handling restarts and disconnections of BTS and SGSN.
I think there is nothing wrong with a 'fail fast' approach. the SGSN disconnecting should be a rare event. The PCU is a small process. Exiting and re-spawning just to make 100% sure that no bogus state remains is a valid approach from my point of view. osmo-bts also does it the same way.
It's not like we're talking about a 200MByte sized complex process where re-spawning is an expensive operation.
Regards, Harald
osmocom-net-gprs@lists.osmocom.org