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
-
keith