keith has uploaded this change for review. (
https://gerrit.osmocom.org/c/osmo-msc/+/28338 )
Change subject: Refactor smsq_take_next_sms()
......................................................................
Refactor smsq_take_next_sms()
The logic in sms_submit_pending() was:
Loop up to 1,000 times around smsq_take_next_sms():
In smsq_take_next_sms(), loop up to 100 times around a DB query
where this query will get one SMS record:
We then allocate memory for the SMS, calling sqlite3_column_XXX()
on each column and also check if the destination addr is attached in
the VLR. If not, we discard (free) this SMS and continue the loop.
A problem with the above occurs when there are more than 100 SMS in the
database for subscribers that are not attached.
In this case smsq_take_next_sms() reaches the "sanity" limit of 100
iterations and returns NULL.
sms_submit_pending() consequently breaks out of its loop of 1000
iterations and no SMS is added to the pending (in-memory) queue.
However, other activity will very likely soon trigger sms_submit_pending()
again and we will repepat the above, pointlessly.
It appears that these 100 iterations over a sqlite3 SELECT + subsequent
sqlite3_column_XXXX() processing is causing osmo-msc to spend a significant
of time in libsqlite3.
So, rather than getting many SMS from the DB only to then discard them
as the subscriber is not attached, let's add a WHERE clause to the query in
order to only select SMS for attached subscribers.
This way we should never have more than one iteration in smsq_take_next_sms()
and we do not do any needless processing in libsqlite3 only to immediately
free() the result.
Change-Id: I06c418d4919dd8d28c643b7e0a735bc74d66212c
---
M include/osmocom/msc/db.h
M src/libmsc/db.c
M src/libmsc/sms_queue.c
M tests/sms_queue/sms_queue_test.c
M tests/sms_queue/sms_queue_test.ok
5 files changed, 66 insertions(+), 39 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/38/28338/1
diff --git a/include/osmocom/msc/db.h b/include/osmocom/msc/db.h
index fc1781b..a67c050 100644
--- a/include/osmocom/msc/db.h
+++ b/include/osmocom/msc/db.h
@@ -36,6 +36,7 @@
int db_fini(void);
/* SMS store-and-forward */
+int db_save_to_temp_vlr(char *msisdn);
int db_sms_store(struct gsm_sms *sms);
struct gsm_sms *db_sms_get(struct gsm_network *net, unsigned long long id);
struct gsm_sms *db_sms_get_next_unsent(struct gsm_network *net,
diff --git a/src/libmsc/db.c b/src/libmsc/db.c
index 07081d5..68cc038 100644
--- a/src/libmsc/db.c
+++ b/src/libmsc/db.c
@@ -46,6 +46,7 @@
enum stmt_idx {
DB_STMT_SMS_STORE,
DB_STMT_SMS_GET,
+ DB_STMT_SMS_GET_NEXT_UNSENT_RR_FOR_ATTACHED,
DB_STMT_SMS_GET_NEXT_UNSENT,
DB_STMT_SMS_GET_UNSENT_FOR_SUBSCR,
DB_STMT_SMS_GET_NEXT_UNSENT_RR_MSISDN,
@@ -56,6 +57,7 @@
DB_STMT_SMS_DEL_EXPIRED,
DB_STMT_SMS_GET_VALID_UNTIL_BY_ID,
DB_STMT_SMS_GET_OLDEST_EXPIRED,
+ DB_STMT_VLR_INSERT,
_NUM_DB_STMT
};
@@ -88,6 +90,7 @@
SCHEMA_RATE,
SCHEMA_AUTHKEY,
SCHEMA_AUTHLAST,
+ SCHEMA_TEMP_VLR,
};
static const char *create_stmts[] = {
@@ -203,6 +206,9 @@
"sres BLOB NOT NULL, "
"kc BLOB NOT NULL "
")",
+ [SCHEMA_TEMP_VLR] = "CREATE TEMPORARY TABLE IF NOT EXISTS t_vlr ("
+ "msisdn TEXT NOT NULL"
+ ")"
};
/***********************************************************************
@@ -270,6 +276,12 @@
"$msg_ref, $protocol_id, $data_coding_scheme, $ud_hdr_ind,
$user_data, $text, "
"$dest_addr, $dest_ton, $dest_npi, $src_addr, $src_ton,
$src_npi)",
[DB_STMT_SMS_GET] = "SELECT " SEL_COLUMNS " FROM SMS WHERE SMS.id =
$id",
+ [DB_STMT_SMS_GET_NEXT_UNSENT_RR_FOR_ATTACHED] =
+ "SELECT " SEL_COLUMNS " FROM SMS "
+ "WHERE sent IS NULL "
+ "AND dest_addr in (SELECT msisdn FROM t_vlr WHERE msisdn >
$dest_addr) "
+ " AND deliver_attempts <= $attempts"
+ " ORDER BY dest_addr, id LIMIT 1",
[DB_STMT_SMS_GET_NEXT_UNSENT] =
"SELECT " SEL_COLUMNS " FROM SMS"
" WHERE sent IS NULL"
@@ -306,6 +318,8 @@
"SELECT strftime('%s', valid_until) FROM SMS WHERE id = $id",
[DB_STMT_SMS_GET_OLDEST_EXPIRED] =
"SELECT id, strftime('%s', valid_until) FROM SMS ORDER BY
valid_until LIMIT 1",
+ [DB_STMT_VLR_INSERT] =
+ "REPLACE INTO t_vlr (msisdn) VALUES ($msisdn)",
};
/***********************************************************************
@@ -568,6 +582,12 @@
sqlite3_free(err_msg);
/* non-fatal */
}
+ rc = sqlite3_exec(g_dbc->db, "PRAGMA temp_store=MEMORY;", 0, 0,
&err_msg);
+ if (rc != SQLITE_OK) {
+ LOGP(DDB, LOGL_ERROR, "Unable to set TEMP_STORE: %s\n",
err_msg);
+ sqlite3_free(err_msg);
+ /* non-fatal */
+ }
return 0;
}
@@ -648,6 +668,22 @@
return 0;
}
+/* Add a connected subscriber MSISDN to the temporary table */
+int db_save_to_temp_vlr(char *msisdn)
+{
+ OSMO_ASSERT(g_dbc);
+ sqlite3_stmt *stmt = g_dbc->stmt[DB_STMT_VLR_INSERT];
+ int rc;
+
+ db_bind_text(stmt, "$msisdn", (char *)msisdn);
+ rc = sqlite3_step(stmt);
+ db_remove_reset(stmt);
+ if (rc != SQLITE_DONE) {
+ LOGP(DDB, LOGL_ERROR, "Cannot add to VLR: SQL error: (%d)
%s\n", rc, sqlite3_errmsg(g_dbc->db));
+ return -EIO;
+ }
+ return 0;
+}
/* store an [unsent] SMS to the database */
int db_sms_store(struct gsm_sms *sms)
{
@@ -858,7 +894,7 @@
int max_failed)
{
OSMO_ASSERT(g_dbc);
- sqlite3_stmt *stmt = g_dbc->stmt[DB_STMT_SMS_GET_NEXT_UNSENT_RR_MSISDN];
+ sqlite3_stmt *stmt =
g_dbc->stmt[DB_STMT_SMS_GET_NEXT_UNSENT_RR_FOR_ATTACHED];
struct gsm_sms *sms;
int rc;
diff --git a/src/libmsc/sms_queue.c b/src/libmsc/sms_queue.c
index 9f18f4f..a1cc465 100644
--- a/src/libmsc/sms_queue.c
+++ b/src/libmsc/sms_queue.c
@@ -294,10 +294,26 @@
struct gsm_sms *sms;
int wrapped = 0;
int sanity = 100;
+ unsigned int count = 0;
char started_with_msisdn[last_msisdn_buflen];
+ struct vlr_subscr *vsub;
OSMO_STRLCPY_ARRAY(started_with_msisdn, last_msisdn);
+ if (net != NULL) { /* db_sms_test passes NULL, so we need to be
tolerant */
+ llist_for_each_entry(vsub, &net->vlr->subscribers, list) {
+ if (vsub->msisdn[0] != '\0') {
+ count++;
+ DEBUGP(DLSMS, "SMS queue: %s is attached.\n",
vsub->msisdn);
+ db_save_to_temp_vlr(vsub->msisdn);
+ }
+ }
+ if (count == 0) {
+ DEBUGP(DLSMS, "SMS queue: no subscribers attached.\n");
+ return NULL;
+ }
+ }
+
while (wrapped < 2 && (--sanity)) {
/* If we wrapped around and passed the first msisdn, we're
* through the entire SMS DB; end it. */
@@ -305,27 +321,16 @@
break;
sms = db_sms_get_next_unsent_rr_msisdn(net, last_msisdn, 9);
+
if (!sms) {
last_msisdn[0] = '\0';
wrapped++;
continue;
}
- /* Whatever happens, next time around service another recipient
- */
+ /* Whatever happens, next time around service another recipient
*/
osmo_strlcpy(last_msisdn, sms->dst.addr, last_msisdn_buflen);
- /* Is the subscriber attached? If not, go to next SMS */
- if (!sms->receiver || !sms->receiver->lu_complete) {
- LOGP(DLSMS, LOGL_DEBUG,
- "Subscriber %s%s is not attached, skipping SMS
%llu\n",
- sms->receiver ? "" : "MSISDN-",
- sms->receiver ?
vlr_subscr_msisdn_or_name(sms->receiver)
- : sms->dst.addr, sms->id);
- sms_free(sms);
- continue;
- }
-
return sms;
}
diff --git a/tests/sms_queue/sms_queue_test.c b/tests/sms_queue/sms_queue_test.c
index f681657..20c31eb 100644
--- a/tests/sms_queue/sms_queue_test.c
+++ b/tests/sms_queue/sms_queue_test.c
@@ -132,6 +132,10 @@
osmo_strlcpy(sms->text, fake_sms_db[i].msisdn,
sizeof(sms->text));
if (fake_sms_db[i].vsub_attached)
fake_sms_db[i].nr_of_sms--;
+ if (!sms->receiver) {
+ sms_free(sms);
+ return NULL;
+ }
return sms;
}
diff --git a/tests/sms_queue/sms_queue_test.ok
b/tests/sms_queue/sms_queue_test.ok
index 146400d..6ccc95f 100644
--- a/tests/sms_queue/sms_queue_test.ok
+++ b/tests/sms_queue/sms_queue_test.ok
@@ -12,21 +12,17 @@
hitting database: looking for MSISDN > '2222', failed_attempts <= 9
#1: sending SMS to 3333 (last_msisdn='3333')
hitting database: looking for MSISDN > '3333', failed_attempts <= 9
- hitting database: looking for MSISDN > '5555', failed_attempts <= 9
hitting database: looking for MSISDN > '', failed_attempts <= 9
#2: sending SMS to 2222 (last_msisdn='2222')
hitting database: looking for MSISDN > '2222', failed_attempts <= 9
#3: sending SMS to 3333 (last_msisdn='3333')
hitting database: looking for MSISDN > '3333', failed_attempts <= 9
- hitting database: looking for MSISDN > '5555', failed_attempts <= 9
hitting database: looking for MSISDN > '', failed_attempts <= 9
-#4: no SMS to send (last_msisdn='5555')
- hitting database: looking for MSISDN > '5555', failed_attempts <= 9
+#4: no SMS to send (last_msisdn='')
hitting database: looking for MSISDN > '', failed_attempts <= 9
-#5: no SMS to send (last_msisdn='5555')
- hitting database: looking for MSISDN > '5555', failed_attempts <= 9
+#5: no SMS to send (last_msisdn='')
hitting database: looking for MSISDN > '', failed_attempts <= 9
-#6: no SMS to send (last_msisdn='5555')
+#6: no SMS to send (last_msisdn='')
- SMS are pending at various nr failed attempts (cutoff at >= 10)
1111 has 1 SMS pending, 0 failed attempts
@@ -60,27 +56,12 @@
5555 (NOT attached) has 1 SMS pending, 0 failed attempts
-->
hitting database: looking for MSISDN > '2345', failed_attempts <= 9
- hitting database: looking for MSISDN > '3333', failed_attempts <= 9
- hitting database: looking for MSISDN > '4444', failed_attempts <= 9
- hitting database: looking for MSISDN > '5555', failed_attempts <= 9
hitting database: looking for MSISDN > '', failed_attempts <= 9
- hitting database: looking for MSISDN > '1111', failed_attempts <= 9
- hitting database: looking for MSISDN > '2222', failed_attempts <= 9
-#0: no SMS to send (last_msisdn='3333')
- hitting database: looking for MSISDN > '3333', failed_attempts <= 9
- hitting database: looking for MSISDN > '4444', failed_attempts <= 9
- hitting database: looking for MSISDN > '5555', failed_attempts <= 9
+#0: no SMS to send (last_msisdn='')
hitting database: looking for MSISDN > '', failed_attempts <= 9
- hitting database: looking for MSISDN > '1111', failed_attempts <= 9
- hitting database: looking for MSISDN > '2222', failed_attempts <= 9
-#1: no SMS to send (last_msisdn='3333')
- hitting database: looking for MSISDN > '3333', failed_attempts <= 9
- hitting database: looking for MSISDN > '4444', failed_attempts <= 9
- hitting database: looking for MSISDN > '5555', failed_attempts <= 9
+#1: no SMS to send (last_msisdn='')
hitting database: looking for MSISDN > '', failed_attempts <= 9
- hitting database: looking for MSISDN > '1111', failed_attempts <= 9
- hitting database: looking for MSISDN > '2222', failed_attempts <= 9
-#2: no SMS to send (last_msisdn='3333')
+#2: no SMS to send (last_msisdn='')
- there are no SMS in the DB
1111 has 0 SMS pending, 0 failed attempts
--
To view, visit https://gerrit.osmocom.org/c/osmo-msc/+/28338
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-Change-Id: I06c418d4919dd8d28c643b7e0a735bc74d66212c
Gerrit-Change-Number: 28338
Gerrit-PatchSet: 1
Gerrit-Owner: keith <[email protected]>
Gerrit-MessageType: newchange
