[MERGED] osmo-msc[master]: fix paging: add timeout to discard unsuccessful paging

[Harald Welte]

Harald Welte has submitted this change and it was merged.

Change subject: fix paging: add timeout to discard unsuccessful paging
..


fix paging: add timeout to discard unsuccessful paging

Currently, if there is no reply from the BSS / RNC, a subscriber will remain as
"already paged" forever, and is never going to be paged again. Even on IMSI
Detach, the pending request will keep a ref count on the vlr_subscr.

Add a paging timeout, as gsm_network->paging_timeout and in the VTY on the
'msc' node as 'paging timeout (default|<1-65535>'. (There is a 'network' /
'T3113' in OsmoBSC, but to not confuse the two, give this a different name.)

Add test_ms_timeout_paging() test to verify the timeout works.

I hit this while testing Paging across multiple hNodeB, when a UE lost
connection to the hNodeB. I noticed that no matter how long I wait, no Paging
is sent out anymore, and found this embarrassing issue. Good grief...

The choice of 10 seconds is taken from https://osmocom.org/issues/2756

Change-Id: I2db6f1e2ad341cf9c2cc7a21ec2fca0bae5b2db5
---
M include/osmocom/msc/gsm_data.h
M include/osmocom/msc/vlr.h
M src/libcommon-cs/common_cs.c
M src/libmsc/gsm_subscriber.c
M src/libmsc/msc_vty.c
M tests/msc_vlr/msc_vlr_test_ms_timeout.c
M tests/msc_vlr/msc_vlr_test_ms_timeout.err
7 files changed, 298 insertions(+), 1 deletion(-)

Approvals:
  Harald Welte: Looks good to me, approved
  Jenkins Builder: Verified



diff --git a/include/osmocom/msc/gsm_data.h b/include/osmocom/msc/gsm_data.h
index 6349fe0..1b0bff9 100644
--- a/include/osmocom/msc/gsm_data.h
+++ b/include/osmocom/msc/gsm_data.h
@@ -344,6 +344,7 @@
GSM_AUTH_POLICY_REGEXP, /* accept IMSIs matching given regexp */
 };
 
+#define MSC_PAGING_RESPONSE_TIMER_DEFAULT 10
 
 struct gsm_tz {
int override; /* if 0, use system's time zone instead. */
@@ -408,6 +409,7 @@
unsigned int num_bts;
struct llist_head bts_list;
 
+   unsigned int paging_response_timer;
 
/* timer to expire old location updates */
struct osmo_timer_list subscr_expire_timer;
diff --git a/include/osmocom/msc/vlr.h b/include/osmocom/msc/vlr.h
index b625608..1b365a9 100644
--- a/include/osmocom/msc/vlr.h
+++ b/include/osmocom/msc/vlr.h
@@ -157,6 +157,7 @@
struct {
/* pending requests */
bool is_paging;
+   struct osmo_timer_list paging_response_timer;
/* list of struct subscr_request */
struct llist_head requests;
uint8_t lac;
diff --git a/src/libcommon-cs/common_cs.c b/src/libcommon-cs/common_cs.c
index bad8262..4748865 100644
--- a/src/libcommon-cs/common_cs.c
+++ b/src/libcommon-cs/common_cs.c
@@ -60,6 +60,8 @@
/* Use 30 min periodic update interval as sane default */
net->t3212 = 5;
 
+   net->paging_response_timer = MSC_PAGING_RESPONSE_TIMER_DEFAULT;
+
INIT_LLIST_HEAD(>trans_list);
INIT_LLIST_HEAD(>upqueue);
INIT_LLIST_HEAD(>subscr_conns);
diff --git a/src/libmsc/gsm_subscriber.c b/src/libmsc/gsm_subscriber.c
index a013e0e..b3d38d1 100644
--- a/src/libmsc/gsm_subscriber.c
+++ b/src/libmsc/gsm_subscriber.c
@@ -76,7 +76,10 @@
return -EINVAL;
}
 
-   if (event == GSM_PAGING_SUCCEEDED)
+   osmo_timer_del(>cs.paging_response_timer);
+
+   if (event == GSM_PAGING_SUCCEEDED
+   || event == GSM_PAGING_EXPIRED)
msc_stop_paging(vsub);
 
/* Inform parts of the system we don't know */
@@ -126,6 +129,12 @@
return -EINVAL;
 }
 
+static void paging_response_timer_cb(void *data)
+{
+   struct vlr_subscr *vsub = data;
+   subscr_paging_dispatch(GSM_HOOK_RR_PAGING, GSM_PAGING_EXPIRED, NULL, 
NULL, vsub);
+}
+
 /*! \brief Start a paging request for vsub, call cbfn(param) when done.
  * \param vsub  subscriber to page.
  * \param cbfn  function to call when the conn is established.
@@ -138,6 +147,7 @@
 {
int rc;
struct subscr_request *request;
+   struct gsm_network *net = vsub->vlr->user_ctx;
 
/* Start paging.. we know it is async so we can do it before */
if (!vsub->cs.is_paging) {
@@ -152,6 +162,8 @@
/* reduced on the first paging callback */
vlr_subscr_get(vsub);
vsub->cs.is_paging = true;
+   osmo_timer_setup(>cs.paging_response_timer, 
paging_response_timer_cb, vsub);
+   osmo_timer_schedule(>cs.paging_response_timer, 
net->paging_response_timer, 0);
} else {
LOGP(DMM, LOGL_DEBUG, "Subscriber %s already paged.\n",
vlr_subscr_name(vsub));
diff --git a/src/libmsc/msc_vty.c b/src/libmsc/msc_vty.c
index 14ad19e..c1c9f6b 100644
--- a/src/libmsc/msc_vty.c
+++ b/src/libmsc/msc_vty.c
@@ -109,6 +109,22 @@
return CMD_SUCCESS;
 }
 
+DEFUN(cfg_msc_paging_response_timer, cfg_msc_paging_response_timer_cmd,
+  

osmo-msc[master]: fix paging: add timeout to discard unsuccessful paging

[Harald Welte]

Patch Set 4: Code-Review+2

-- 
To view, visit https://gerrit.osmocom.org/5463
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2db6f1e2ad341cf9c2cc7a21ec2fca0bae5b2db5
Gerrit-PatchSet: 4
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: No

[PATCH] osmo-msc[master]: fix paging: add timeout to discard unsuccessful paging

[Neels Hofmeyr]

Hello Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/5463

to look at the new patch set (#4).

fix paging: add timeout to discard unsuccessful paging

Currently, if there is no reply from the BSS / RNC, a subscriber will remain as
"already paged" forever, and is never going to be paged again. Even on IMSI
Detach, the pending request will keep a ref count on the vlr_subscr.

Add a paging timeout, as gsm_network->paging_timeout and in the VTY on the
'msc' node as 'paging timeout (default|<1-65535>'. (There is a 'network' /
'T3113' in OsmoBSC, but to not confuse the two, give this a different name.)

Add test_ms_timeout_paging() test to verify the timeout works.

I hit this while testing Paging across multiple hNodeB, when a UE lost
connection to the hNodeB. I noticed that no matter how long I wait, no Paging
is sent out anymore, and found this embarrassing issue. Good grief...

The choice of 10 seconds is taken from https://osmocom.org/issues/2756

Change-Id: I2db6f1e2ad341cf9c2cc7a21ec2fca0bae5b2db5
---
M include/osmocom/msc/gsm_data.h
M include/osmocom/msc/vlr.h
M src/libcommon-cs/common_cs.c
M src/libmsc/gsm_subscriber.c
M src/libmsc/msc_vty.c
M tests/msc_vlr/msc_vlr_test_ms_timeout.c
M tests/msc_vlr/msc_vlr_test_ms_timeout.err
7 files changed, 298 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/63/5463/4

diff --git a/include/osmocom/msc/gsm_data.h b/include/osmocom/msc/gsm_data.h
index 6349fe0..1b0bff9 100644
--- a/include/osmocom/msc/gsm_data.h
+++ b/include/osmocom/msc/gsm_data.h
@@ -344,6 +344,7 @@
GSM_AUTH_POLICY_REGEXP, /* accept IMSIs matching given regexp */
 };
 
+#define MSC_PAGING_RESPONSE_TIMER_DEFAULT 10
 
 struct gsm_tz {
int override; /* if 0, use system's time zone instead. */
@@ -408,6 +409,7 @@
unsigned int num_bts;
struct llist_head bts_list;
 
+   unsigned int paging_response_timer;
 
/* timer to expire old location updates */
struct osmo_timer_list subscr_expire_timer;
diff --git a/include/osmocom/msc/vlr.h b/include/osmocom/msc/vlr.h
index b625608..1b365a9 100644
--- a/include/osmocom/msc/vlr.h
+++ b/include/osmocom/msc/vlr.h
@@ -157,6 +157,7 @@
struct {
/* pending requests */
bool is_paging;
+   struct osmo_timer_list paging_response_timer;
/* list of struct subscr_request */
struct llist_head requests;
uint8_t lac;
diff --git a/src/libcommon-cs/common_cs.c b/src/libcommon-cs/common_cs.c
index bad8262..4748865 100644
--- a/src/libcommon-cs/common_cs.c
+++ b/src/libcommon-cs/common_cs.c
@@ -60,6 +60,8 @@
/* Use 30 min periodic update interval as sane default */
net->t3212 = 5;
 
+   net->paging_response_timer = MSC_PAGING_RESPONSE_TIMER_DEFAULT;
+
INIT_LLIST_HEAD(>trans_list);
INIT_LLIST_HEAD(>upqueue);
INIT_LLIST_HEAD(>subscr_conns);
diff --git a/src/libmsc/gsm_subscriber.c b/src/libmsc/gsm_subscriber.c
index a013e0e..b3d38d1 100644
--- a/src/libmsc/gsm_subscriber.c
+++ b/src/libmsc/gsm_subscriber.c
@@ -76,7 +76,10 @@
return -EINVAL;
}
 
-   if (event == GSM_PAGING_SUCCEEDED)
+   osmo_timer_del(>cs.paging_response_timer);
+
+   if (event == GSM_PAGING_SUCCEEDED
+   || event == GSM_PAGING_EXPIRED)
msc_stop_paging(vsub);
 
/* Inform parts of the system we don't know */
@@ -126,6 +129,12 @@
return -EINVAL;
 }
 
+static void paging_response_timer_cb(void *data)
+{
+   struct vlr_subscr *vsub = data;
+   subscr_paging_dispatch(GSM_HOOK_RR_PAGING, GSM_PAGING_EXPIRED, NULL, 
NULL, vsub);
+}
+
 /*! \brief Start a paging request for vsub, call cbfn(param) when done.
  * \param vsub  subscriber to page.
  * \param cbfn  function to call when the conn is established.
@@ -138,6 +147,7 @@
 {
int rc;
struct subscr_request *request;
+   struct gsm_network *net = vsub->vlr->user_ctx;
 
/* Start paging.. we know it is async so we can do it before */
if (!vsub->cs.is_paging) {
@@ -152,6 +162,8 @@
/* reduced on the first paging callback */
vlr_subscr_get(vsub);
vsub->cs.is_paging = true;
+   osmo_timer_setup(>cs.paging_response_timer, 
paging_response_timer_cb, vsub);
+   osmo_timer_schedule(>cs.paging_response_timer, 
net->paging_response_timer, 0);
} else {
LOGP(DMM, LOGL_DEBUG, "Subscriber %s already paged.\n",
vlr_subscr_name(vsub));
diff --git a/src/libmsc/msc_vty.c b/src/libmsc/msc_vty.c
index 14ad19e..c1c9f6b 100644
--- a/src/libmsc/msc_vty.c
+++ b/src/libmsc/msc_vty.c
@@ -109,6 +109,22 @@
return CMD_SUCCESS;
 }
 
+DEFUN(cfg_msc_paging_response_timer, cfg_msc_paging_response_timer_cmd,
+  "paging response-timer (default|<1-65535>)",
+  "Configure 

osmo-msc[master]: fix paging: add timeout to discard unsuccessful paging

[Harald Welte]

Patch Set 3:

(1 comment)

https://gerrit.osmocom.org/#/c/5463/3/src/libmsc/msc_vty.c
File src/libmsc/msc_vty.c:

Line 113:   "paging timeout (default|<1-65535>)",
> the drawback of aliases is that when writing the config back to file, possi
why would there be any aliases involved?  We don't have any existing code that 
we need to interoperate with, right?


-- 
To view, visit https://gerrit.osmocom.org/5463
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2db6f1e2ad341cf9c2cc7a21ec2fca0bae5b2db5
Gerrit-PatchSet: 3
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: Yes

osmo-msc[master]: fix paging: add timeout to discard unsuccessful paging

[Neels Hofmeyr]

Patch Set 3:

(1 comment)

https://gerrit.osmocom.org/#/c/5463/3/src/libmsc/msc_vty.c
File src/libmsc/msc_vty.c:

Line 113:   "paging timeout (default|<1-65535>)",
> let's alsocall it 'paging response-timer' to align with the spec language.
the drawback of aliases is that when writing the config back to file, possibly 
one will be changed into the other. It's not a hard requirement to write back 
the same alias as was read in, but to spare us that confusion I decided to only 
use 'paging response timer' in all the naming. Close enough to 'timeout' anyway.


-- 
To view, visit https://gerrit.osmocom.org/5463
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2db6f1e2ad341cf9c2cc7a21ec2fca0bae5b2db5
Gerrit-PatchSet: 3
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: Yes

osmo-msc[master]: fix paging: add timeout to discard unsuccessful paging

[Harald Welte]

Patch Set 3:

(1 comment)

https://gerrit.osmocom.org/#/c/5463/3/src/libmsc/msc_vty.c
File src/libmsc/msc_vty.c:

Line 113:   "paging timeout (default|<1-65535>)",
let's alsocall it 'paging response-timer' to align with the spec language.


-- 
To view, visit https://gerrit.osmocom.org/5463
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2db6f1e2ad341cf9c2cc7a21ec2fca0bae5b2db5
Gerrit-PatchSet: 3
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: Yes

osmo-msc[master]: fix paging: add timeout to discard unsuccessful paging

[Harald Welte]

Patch Set 3:

It seems there is no number, 23.018 and other related specs simply call it the 
"paging response timer".

-- 
To view, visit https://gerrit.osmocom.org/5463
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2db6f1e2ad341cf9c2cc7a21ec2fca0bae5b2db5
Gerrit-PatchSet: 3
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: No

osmo-msc[master]: fix paging: add timeout to discard unsuccessful paging

[Harald Welte]

Patch Set 3:

> > > are we sure there's no GSM timer number/name for this?
 > >
 > > not sure. I can try to have a look...
 > 
 > So far all "Paging" chapters I have found talk merely of initiating
 > a Paging; closest match would be the 48.008 "3.1.10 Paging", which
 > doesn't mention any timeouts. 

I think the general assumption is that the MSC once sends a PAGING command
to the BSC[s] and doesn't do any re-transmissions but rather relies on the
BSC to take care of that.

The question is how long the BSC will wait for any response from the BSC. I'll 
have a look.

 > The T3113 timer is mentioned for the
 > RR layer in 44.018 "3.3.2.1 Paging initiation by the network", but
 > I'm looking at the MSC, i.e. BSSMAP and IuCS. I feel like I'm
 > searching the needle in the hay stack.

Sorry for that.

 > On another note, I personally find "paging timeout" much easier to
 > understand than "T1234"...

The "problem" is that the existing literature on network 
configuration/optimziation will always refer to the timer numbers. Us inventing 
our own names is fine, as long
was we also use the official number somewhere so people can find it easily.

-- 
To view, visit https://gerrit.osmocom.org/5463
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2db6f1e2ad341cf9c2cc7a21ec2fca0bae5b2db5
Gerrit-PatchSet: 3
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: No

osmo-msc[master]: fix paging: add timeout to discard unsuccessful paging

[Neels Hofmeyr]

Patch Set 3:

> > are we sure there's no GSM timer number/name for this?
 > 
 > not sure. I can try to have a look...

So far all "Paging" chapters I have found talk merely of initiating a Paging; 
closest match would be the 48.008 "3.1.10 Paging", which doesn't mention any 
timeouts. The T3113 timer is mentioned for the RR layer in 44.018 "3.3.2.1 
Paging initiation by the network", but I'm looking at the MSC, i.e. BSSMAP and 
IuCS. I feel like I'm searching the needle in the hay stack.

On another note, I personally find "paging timeout" much easier to understand 
than "T1234"...

-- 
To view, visit https://gerrit.osmocom.org/5463
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2db6f1e2ad341cf9c2cc7a21ec2fca0bae5b2db5
Gerrit-PatchSet: 3
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: No

osmo-msc[master]: fix paging: add timeout to discard unsuccessful paging

[Neels Hofmeyr]

Patch Set 3:

> are we sure there's no GSM timer number/name for this?

not sure. I can try to have a look...

-- 
To view, visit https://gerrit.osmocom.org/5463
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2db6f1e2ad341cf9c2cc7a21ec2fca0bae5b2db5
Gerrit-PatchSet: 3
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: No

osmo-msc[master]: fix paging: add timeout to discard unsuccessful paging

[Harald Welte]

Patch Set 3:

are we sure there's no GSM timer number/name for this?

-- 
To view, visit https://gerrit.osmocom.org/5463
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2db6f1e2ad341cf9c2cc7a21ec2fca0bae5b2db5
Gerrit-PatchSet: 3
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Harald Welte 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: No

[PATCH] osmo-msc[master]: fix paging: add timeout to discard unsuccessful paging

[Neels Hofmeyr]

Hello Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/5463

to look at the new patch set (#3).

fix paging: add timeout to discard unsuccessful paging

Currently, if there is no reply from the BSS / RNC, a subscriber will remain as
"already paged" forever, and is never going to be paged again. Even on IMSI
Detach, the pending request will keep a ref count on the vlr_subscr.

Add a paging timeout, as gsm_network->paging_timeout and in the VTY on the
'msc' node as 'paging timeout (default|<1-65535>'. (There is a 'network' /
'T3113' in OsmoBSC, but to not confuse the two, give this a different name.)

Add test_ms_timeout_paging() test to verify the timeout works.

I hit this while testing Paging across multiple hNodeB, when a UE lost
connection to the hNodeB. I noticed that no matter how long I wait, no Paging
is sent out anymore, and found this embarrassing issue. Good grief...

The choice of 10 seconds is taken from https://osmocom.org/issues/2756

Change-Id: I2db6f1e2ad341cf9c2cc7a21ec2fca0bae5b2db5
---
M include/osmocom/msc/gsm_data.h
M include/osmocom/msc/vlr.h
M src/libcommon-cs/common_cs.c
M src/libmsc/gsm_subscriber.c
M src/libmsc/msc_vty.c
M tests/msc_vlr/msc_vlr_test_ms_timeout.c
M tests/msc_vlr/msc_vlr_test_ms_timeout.err
7 files changed, 298 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/63/5463/3

diff --git a/include/osmocom/msc/gsm_data.h b/include/osmocom/msc/gsm_data.h
index 6349fe0..1540524 100644
--- a/include/osmocom/msc/gsm_data.h
+++ b/include/osmocom/msc/gsm_data.h
@@ -344,6 +344,7 @@
GSM_AUTH_POLICY_REGEXP, /* accept IMSIs matching given regexp */
 };
 
+#define MSC_PAGING_TIMEOUT_DEFAULT 10
 
 struct gsm_tz {
int override; /* if 0, use system's time zone instead. */
@@ -408,6 +409,7 @@
unsigned int num_bts;
struct llist_head bts_list;
 
+   unsigned int paging_timeout;
 
/* timer to expire old location updates */
struct osmo_timer_list subscr_expire_timer;
diff --git a/include/osmocom/msc/vlr.h b/include/osmocom/msc/vlr.h
index b625608..76bb36c 100644
--- a/include/osmocom/msc/vlr.h
+++ b/include/osmocom/msc/vlr.h
@@ -157,6 +157,7 @@
struct {
/* pending requests */
bool is_paging;
+   struct osmo_timer_list paging_timeout;
/* list of struct subscr_request */
struct llist_head requests;
uint8_t lac;
diff --git a/src/libcommon-cs/common_cs.c b/src/libcommon-cs/common_cs.c
index bad8262..ef30aba 100644
--- a/src/libcommon-cs/common_cs.c
+++ b/src/libcommon-cs/common_cs.c
@@ -60,6 +60,8 @@
/* Use 30 min periodic update interval as sane default */
net->t3212 = 5;
 
+   net->paging_timeout = MSC_PAGING_TIMEOUT_DEFAULT;
+
INIT_LLIST_HEAD(>trans_list);
INIT_LLIST_HEAD(>upqueue);
INIT_LLIST_HEAD(>subscr_conns);
diff --git a/src/libmsc/gsm_subscriber.c b/src/libmsc/gsm_subscriber.c
index a013e0e..b534b3f 100644
--- a/src/libmsc/gsm_subscriber.c
+++ b/src/libmsc/gsm_subscriber.c
@@ -76,7 +76,10 @@
return -EINVAL;
}
 
-   if (event == GSM_PAGING_SUCCEEDED)
+   osmo_timer_del(>cs.paging_timeout);
+
+   if (event == GSM_PAGING_SUCCEEDED
+   || event == GSM_PAGING_EXPIRED)
msc_stop_paging(vsub);
 
/* Inform parts of the system we don't know */
@@ -126,6 +129,12 @@
return -EINVAL;
 }
 
+static void paging_timeout_cb(void *data)
+{
+   struct vlr_subscr *vsub = data;
+   subscr_paging_dispatch(GSM_HOOK_RR_PAGING, GSM_PAGING_EXPIRED, NULL, 
NULL, vsub);
+}
+
 /*! \brief Start a paging request for vsub, call cbfn(param) when done.
  * \param vsub  subscriber to page.
  * \param cbfn  function to call when the conn is established.
@@ -138,6 +147,7 @@
 {
int rc;
struct subscr_request *request;
+   struct gsm_network *net = vsub->vlr->user_ctx;
 
/* Start paging.. we know it is async so we can do it before */
if (!vsub->cs.is_paging) {
@@ -152,6 +162,8 @@
/* reduced on the first paging callback */
vlr_subscr_get(vsub);
vsub->cs.is_paging = true;
+   osmo_timer_setup(>cs.paging_timeout, paging_timeout_cb, 
vsub);
+   osmo_timer_schedule(>cs.paging_timeout, 
net->paging_timeout, 0);
} else {
LOGP(DMM, LOGL_DEBUG, "Subscriber %s already paged.\n",
vlr_subscr_name(vsub));
diff --git a/src/libmsc/msc_vty.c b/src/libmsc/msc_vty.c
index 14ad19e..a4d4a39 100644
--- a/src/libmsc/msc_vty.c
+++ b/src/libmsc/msc_vty.c
@@ -109,6 +109,22 @@
return CMD_SUCCESS;
 }
 
+DEFUN(cfg_msc_paging_timeout, cfg_msc_paging_timeout_cmd,
+  "paging timeout (default|<1-65535>)",
+  "Configure Paging\n"
+  "Set Paging timeout, the minimum time to pass between (unsuccessful) 
Pagings sent 

[PATCH] osmo-msc[master]: fix paging: add timeout to discard unsuccessful paging

[Neels Hofmeyr]

Hello Jenkins Builder,

I'd like you to reexamine a change.  Please visit

https://gerrit.osmocom.org/5463

to look at the new patch set (#2).

fix paging: add timeout to discard unsuccessful paging

Currently, if there is no reply from the BSS / RNC, a subscriber will remain as
"already paged" forever, and is never going to be paged again. Even on IMSI
Detach, the pending request will keep a ref count on the vlr_subscr.

Add a paging timeout, as gsm_network->paging_timeout and in the VTY on the
'msc' node as 'paging timeout (default|<1-65535>'. (There is a 'network' /
'T3113' in OsmoBSC, but to not confuse the two, give this a different name.)

Add test_ms_timeout_paging() test to verify the timeout works.

I hit this while testing Paging across multiple hNodeB, when a UE lost
connection to the hNodeB. I noticed that no matter how long I wait, no Paging
is sent out anymore, and found this embarrassing issue. Good grief...

The choice of 10 seconds is taken from https://osmocom.org/issues/2756

Change-Id: I2db6f1e2ad341cf9c2cc7a21ec2fca0bae5b2db5
---
M include/osmocom/msc/gsm_data.h
M include/osmocom/msc/vlr.h
M src/libcommon-cs/common_cs.c
M src/libmsc/gsm_subscriber.c
M src/libmsc/msc_vty.c
M tests/msc_vlr/msc_vlr_test_ms_timeout.c
M tests/msc_vlr/msc_vlr_test_ms_timeout.err
7 files changed, 298 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/63/5463/2

diff --git a/include/osmocom/msc/gsm_data.h b/include/osmocom/msc/gsm_data.h
index 6349fe0..1540524 100644
--- a/include/osmocom/msc/gsm_data.h
+++ b/include/osmocom/msc/gsm_data.h
@@ -344,6 +344,7 @@
GSM_AUTH_POLICY_REGEXP, /* accept IMSIs matching given regexp */
 };
 
+#define MSC_PAGING_TIMEOUT_DEFAULT 10
 
 struct gsm_tz {
int override; /* if 0, use system's time zone instead. */
@@ -408,6 +409,7 @@
unsigned int num_bts;
struct llist_head bts_list;
 
+   unsigned int paging_timeout;
 
/* timer to expire old location updates */
struct osmo_timer_list subscr_expire_timer;
diff --git a/include/osmocom/msc/vlr.h b/include/osmocom/msc/vlr.h
index b625608..76bb36c 100644
--- a/include/osmocom/msc/vlr.h
+++ b/include/osmocom/msc/vlr.h
@@ -157,6 +157,7 @@
struct {
/* pending requests */
bool is_paging;
+   struct osmo_timer_list paging_timeout;
/* list of struct subscr_request */
struct llist_head requests;
uint8_t lac;
diff --git a/src/libcommon-cs/common_cs.c b/src/libcommon-cs/common_cs.c
index bad8262..ef30aba 100644
--- a/src/libcommon-cs/common_cs.c
+++ b/src/libcommon-cs/common_cs.c
@@ -60,6 +60,8 @@
/* Use 30 min periodic update interval as sane default */
net->t3212 = 5;
 
+   net->paging_timeout = MSC_PAGING_TIMEOUT_DEFAULT;
+
INIT_LLIST_HEAD(>trans_list);
INIT_LLIST_HEAD(>upqueue);
INIT_LLIST_HEAD(>subscr_conns);
diff --git a/src/libmsc/gsm_subscriber.c b/src/libmsc/gsm_subscriber.c
index a013e0e..b534b3f 100644
--- a/src/libmsc/gsm_subscriber.c
+++ b/src/libmsc/gsm_subscriber.c
@@ -76,7 +76,10 @@
return -EINVAL;
}
 
-   if (event == GSM_PAGING_SUCCEEDED)
+   osmo_timer_del(>cs.paging_timeout);
+
+   if (event == GSM_PAGING_SUCCEEDED
+   || event == GSM_PAGING_EXPIRED)
msc_stop_paging(vsub);
 
/* Inform parts of the system we don't know */
@@ -126,6 +129,12 @@
return -EINVAL;
 }
 
+static void paging_timeout_cb(void *data)
+{
+   struct vlr_subscr *vsub = data;
+   subscr_paging_dispatch(GSM_HOOK_RR_PAGING, GSM_PAGING_EXPIRED, NULL, 
NULL, vsub);
+}
+
 /*! \brief Start a paging request for vsub, call cbfn(param) when done.
  * \param vsub  subscriber to page.
  * \param cbfn  function to call when the conn is established.
@@ -138,6 +147,7 @@
 {
int rc;
struct subscr_request *request;
+   struct gsm_network *net = vsub->vlr->user_ctx;
 
/* Start paging.. we know it is async so we can do it before */
if (!vsub->cs.is_paging) {
@@ -152,6 +162,8 @@
/* reduced on the first paging callback */
vlr_subscr_get(vsub);
vsub->cs.is_paging = true;
+   osmo_timer_setup(>cs.paging_timeout, paging_timeout_cb, 
vsub);
+   osmo_timer_schedule(>cs.paging_timeout, 
net->paging_timeout, 0);
} else {
LOGP(DMM, LOGL_DEBUG, "Subscriber %s already paged.\n",
vlr_subscr_name(vsub));
diff --git a/src/libmsc/msc_vty.c b/src/libmsc/msc_vty.c
index 14ad19e..a4d4a39 100644
--- a/src/libmsc/msc_vty.c
+++ b/src/libmsc/msc_vty.c
@@ -109,6 +109,22 @@
return CMD_SUCCESS;
 }
 
+DEFUN(cfg_msc_paging_timeout, cfg_msc_paging_timeout_cmd,
+  "paging timeout (default|<1-65535>)",
+  "Configure Paging\n"
+  "Set Paging timeout, the minimum time to pass between (unsuccessful) 
Pagings sent 

osmo-msc[master]: fix paging: add timeout to discard unsuccessful paging

[Neels Hofmeyr]

Patch Set 1:

(1 comment)

https://gerrit.osmocom.org/#/c/5463/1/include/osmocom/msc/gsm_data.h
File include/osmocom/msc/gsm_data.h:

Line 347: #define MSC_PAGING_TIMEOUT_DEFAULT 60
when looking at https://osmocom.org/issues/2756 "T3113 default of 60s seems way 
too large" it appears that this should also be 10 instead?


-- 
To view, visit https://gerrit.osmocom.org/5463
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I2db6f1e2ad341cf9c2cc7a21ec2fca0bae5b2db5
Gerrit-PatchSet: 1
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr 
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr 
Gerrit-HasComments: Yes

[PATCH] osmo-msc[master]: fix paging: add timeout to discard unsuccessful paging

[Neels Hofmeyr]

Review at  https://gerrit.osmocom.org/5463

fix paging: add timeout to discard unsuccessful paging

Currently, if there is no reply from the BSS / RNC, a subscriber will remain as
"already paged" forever, and is never going to be paged again. Even on IMSI
Detach, the pending request will keep a ref count on the vlr_subscr.

Add a paging timeout, as gsm_network->paging_timeout and in the VTY on the
'msc' node as 'paging timeout (default|<1-65535>'. (There is a 'network' /
'T3113' in OsmoBSC, but to not confuse the two, give this a different name.)

Add test_ms_timeout_paging() test to verify the timeout works.

I hit this while testing Paging across multiple hNodeB, when a UE lost
connection to the hNodeB. I noticed that no matter how long I wait, no Paging
is sent out anymore, and found this embarrassing issue. Good grief...

Change-Id: I2db6f1e2ad341cf9c2cc7a21ec2fca0bae5b2db5
---
M include/osmocom/msc/gsm_data.h
M include/osmocom/msc/vlr.h
M src/libcommon-cs/common_cs.c
M src/libmsc/gsm_subscriber.c
M src/libmsc/msc_vty.c
M tests/msc_vlr/msc_vlr_test_ms_timeout.c
M tests/msc_vlr/msc_vlr_test_ms_timeout.err
7 files changed, 298 insertions(+), 1 deletion(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/63/5463/1

diff --git a/include/osmocom/msc/gsm_data.h b/include/osmocom/msc/gsm_data.h
index 6349fe0..8a49d9b 100644
--- a/include/osmocom/msc/gsm_data.h
+++ b/include/osmocom/msc/gsm_data.h
@@ -344,6 +344,7 @@
GSM_AUTH_POLICY_REGEXP, /* accept IMSIs matching given regexp */
 };
 
+#define MSC_PAGING_TIMEOUT_DEFAULT 60
 
 struct gsm_tz {
int override; /* if 0, use system's time zone instead. */
@@ -408,6 +409,7 @@
unsigned int num_bts;
struct llist_head bts_list;
 
+   unsigned int paging_timeout;
 
/* timer to expire old location updates */
struct osmo_timer_list subscr_expire_timer;
diff --git a/include/osmocom/msc/vlr.h b/include/osmocom/msc/vlr.h
index 9e6b12c..cd1ef94 100644
--- a/include/osmocom/msc/vlr.h
+++ b/include/osmocom/msc/vlr.h
@@ -156,6 +156,7 @@
struct {
/* pending requests */
bool is_paging;
+   struct osmo_timer_list paging_timeout;
/* list of struct subscr_request */
struct llist_head requests;
uint8_t lac;
diff --git a/src/libcommon-cs/common_cs.c b/src/libcommon-cs/common_cs.c
index bad8262..ef30aba 100644
--- a/src/libcommon-cs/common_cs.c
+++ b/src/libcommon-cs/common_cs.c
@@ -60,6 +60,8 @@
/* Use 30 min periodic update interval as sane default */
net->t3212 = 5;
 
+   net->paging_timeout = MSC_PAGING_TIMEOUT_DEFAULT;
+
INIT_LLIST_HEAD(>trans_list);
INIT_LLIST_HEAD(>upqueue);
INIT_LLIST_HEAD(>subscr_conns);
diff --git a/src/libmsc/gsm_subscriber.c b/src/libmsc/gsm_subscriber.c
index a013e0e..b534b3f 100644
--- a/src/libmsc/gsm_subscriber.c
+++ b/src/libmsc/gsm_subscriber.c
@@ -76,7 +76,10 @@
return -EINVAL;
}
 
-   if (event == GSM_PAGING_SUCCEEDED)
+   osmo_timer_del(>cs.paging_timeout);
+
+   if (event == GSM_PAGING_SUCCEEDED
+   || event == GSM_PAGING_EXPIRED)
msc_stop_paging(vsub);
 
/* Inform parts of the system we don't know */
@@ -126,6 +129,12 @@
return -EINVAL;
 }
 
+static void paging_timeout_cb(void *data)
+{
+   struct vlr_subscr *vsub = data;
+   subscr_paging_dispatch(GSM_HOOK_RR_PAGING, GSM_PAGING_EXPIRED, NULL, 
NULL, vsub);
+}
+
 /*! \brief Start a paging request for vsub, call cbfn(param) when done.
  * \param vsub  subscriber to page.
  * \param cbfn  function to call when the conn is established.
@@ -138,6 +147,7 @@
 {
int rc;
struct subscr_request *request;
+   struct gsm_network *net = vsub->vlr->user_ctx;
 
/* Start paging.. we know it is async so we can do it before */
if (!vsub->cs.is_paging) {
@@ -152,6 +162,8 @@
/* reduced on the first paging callback */
vlr_subscr_get(vsub);
vsub->cs.is_paging = true;
+   osmo_timer_setup(>cs.paging_timeout, paging_timeout_cb, 
vsub);
+   osmo_timer_schedule(>cs.paging_timeout, 
net->paging_timeout, 0);
} else {
LOGP(DMM, LOGL_DEBUG, "Subscriber %s already paged.\n",
vlr_subscr_name(vsub));
diff --git a/src/libmsc/msc_vty.c b/src/libmsc/msc_vty.c
index 14ad19e..a4d4a39 100644
--- a/src/libmsc/msc_vty.c
+++ b/src/libmsc/msc_vty.c
@@ -109,6 +109,22 @@
return CMD_SUCCESS;
 }
 
+DEFUN(cfg_msc_paging_timeout, cfg_msc_paging_timeout_cmd,
+  "paging timeout (default|<1-65535>)",
+  "Configure Paging\n"
+  "Set Paging timeout, the minimum time to pass between (unsuccessful) 
Pagings sent towards"
+  " BSS or RNC\n"
+  "Set to default timeout (" 
OSMO_STRINGIFY_VAL(MSC_PAGING_TIMEOUT_DEFAULT) " seconds)\n"
+  "Set paging timeout in seconds\n")
+{
+