Re: [PATCH] Guaranteed throughput smsc-independent
On Fri, Jun 19, 2009 at 05:50:16PM +0200, Alexander Malysh wrote:
> Hi,
>
> ok, here we go. This is relative large patch because too much things must be
> merged from private version into official.
Sorry to take this long to get back to you. I've tested it and the results
looks promising, only some comments inlined below (I left only the parts I
considered relevant for the comments).
> Please test attached patch against CVS head or checkout/follow from/on:
> http://github.com/amalysh/kannel/tree/smpp-throttling
I have to say I backported it to stable and test it there only since I need
this for production and I'm not inclined to put a full cvs version just yet.
Also, I haven't tested it in production yet, only against a dumb smsc based
on Net::SMPP.
> This patch implements following:
> - new main loop logic
> - some inlines for speed
Nice catch :)
> - more error handling
> - async shutdown
I didn't test this.
diff --git a/gw/smsc/smsc_smpp.c b/gw/smsc/smsc_smpp.c
index cf61122..60cd2df 100644
--- a/gw/smsc/smsc_smpp.c
+++ b/gw/smsc/smsc_smpp.c
@@ -79,6 +79,7 @@
#include "dlr.h"
#include "bearerbox.h"
#include "meta_data.h"
+#include "load.h"
#define SMPP_DEFAULT_CHARSET "UTF-8"
@@ -111,7 +112,7 @@
#define SMPP_MAX_PENDING_SUBMITS10
#define SMPP_DEFAULT_VERSION0x34
#define SMPP_DEFAULT_PRIORITY 0
-#define SMPP_THROTTLING_SLEEP_TIME 15
+#define SMPP_THROTTLING_SLEEP_TIME 1
Why change this? I know it's no big deal, but I just don't get why we don't
want to stop sending for a longer while if the smsc tell us to.
#define SMPP_DEFAULT_CONNECTION_TIMEOUT 10 * SMPP_ENQUIRE_LINK_INTERVAL
#define SMPP_DEFAULT_WAITACK60
#define SMPP_DEFAULT_SHUTDOWN_TIMEOUT 30
Shouldn't be SMPP_DEFAULT_SHUTDOWN_TIMEOUT usually >= SMPP_DEFAULT_WAITACK so
we wait for the acks instead of shutting down and not getting them? (I know
this is not exactly your patch, but...)
@@ -912,15 +917,15 @@ static SMPP_PDU *msg_to_pdu(SMPP *smpp, Msg *msg)
* Note: we always send in UTC and just define "Time Difference" as 00 and
* direction '+'.
*/
-validity = msg->sms.validity >= 0 ? msg->sms.validity :
smpp->validityperiod;
-if (validity >= 0) {
+validity = msg->sms.validity != SMS_PARAM_UNDEFINED ? msg->sms.validity :
smpp->validityperiod;
+if (validity != SMS_PARAM_UNDEFINED) {
Could this still be SMS_PARAM_UNDEFINED after the line above? (not sure if
msg->sms.validity or smpp->validityperiod can be SMS_PARAM_UNDEFINED)
struct tm tm = gw_gmtime(time(NULL) + validity * 60);
pdu->u.submit_sm.validity_period =
octstr_format("%02d%02d%02d%02d%02d%02d000+",
tm.tm_year % 100, tm.tm_mon + 1, tm.tm_mday,
tm.tm_hour, tm.tm_min, tm.tm_sec);
}
-if (msg->sms.deferred >= 0) {
+if (msg->sms.deferred != SMS_PARAM_UNDEFINED) {
struct tm tm = gw_gmtime(time(NULL) + msg->sms.deferred * 60);
pdu->u.submit_sm.schedule_delivery_time =
octstr_format("%02d%02d%02d%02d%02d%02d000+",
tm.tm_year % 100, tm.tm_mon + 1, tm.tm_mday,
@@ -1860,94 +1897,125 @@ static void io_thread(void *arg)
conn = open_transceiver(smpp);
else
conn = open_receiver(smpp);
-
-last_enquire_sent = last_cleanup = last_response =
date_universal_now();
+
pending_submits = -1;
len = 0;
-smpp->throttling_err_time = 0;
-for (;conn != NULL;) {
-timeout = last_enquire_sent + smpp->enquire_link_interval
-- date_universal_now();
-
-if (conn_wait(conn, timeout) == -1)
+last_response = last_cleanup = last_enquire_sent = time(NULL);
+while(conn != NULL) {
+ret = read_pdu(smpp, conn, &len, &pdu);
+if (ret == -1) { /* connection broken */
+error(0, "SMPP[%s]: I/O error or other error. Re-connecting.",
+ octstr_get_cstr(smpp->conn->id));
break;
-
-/* unbind
- * Read so long as unbind_resp received or timeout passed.
Otherwise we have
- * double delivered messages.
- */
-if (smpp->quitting) {
-send_unbind(smpp, conn);
-last_response = time(NULL);
-while(conn_wait(conn, 1.00) != -1 &&
- difftime(time(NULL), last_response) <
SMPP_DEFAULT_SHUTDOWN_TIMEOUT &&
- smpp->conn->status != SMSCCONN_DISCONNECTED) {
-if (read_pdu(smpp, conn, &len, &pdu) == 1) {
-dump_pdu("Got PDU:", smpp->conn->id, pdu);
-handle_pdu(smpp, conn, pdu, &pending_submits);
-smpp_pdu_destroy(pdu);
-}
+} else if (ret == -2) {
+/* wrong pdu length , send gnack */
+len = 0;
+if (send_gn
Re: [PATCH] Guaranteed throughput smsc-independent
Alex: On Wed, Jun 10, 2009 at 11:52:31PM +0200, Alexander Malysh wrote: > This patch was only to show how it could be done. It's not complete > for SMPP. For SMPP the main loop should be reordered. I have local > version that works as expected but I need some time to extract it > because of many differences. > > I will try to extract it this weekend... Did you have any chance to do it? Could I help you somehow? Thanks, Damián Viano(Des).
Re: [PATCH] Guaranteed throughput smsc-independent
On Wed, Jun 10, 2009 at 05:28:04PM +0200, Alexander Malysh wrote:
>
> Am 10.06.2009 um 17:16 schrieb Damian Viano:
>
>> On Wed, Jun 10, 2009 at 03:06:59PM +0200, Alexander Malysh wrote:
>>> here is a link to throttling branch:
>>> http://github.com/amalysh/kannel/tree/smpp-throttling
>>> only smpp implemented for now.
>>
>> Great, thanks for the link.
>>
>> I see that you basically break out when you are over throughput[1], I
>> see your concern now, since the thread for receiving is the same as
>> the sender, so you just break out to keep processing incoming pdu.
>> Makes sense, and this indeed breaks my assumption that the smsc
>> implementations would have a thread for sending and another one for
>> receiving.
However there's still a problem. In the case of smpp not in transceiver
mode the io_thread() code that calls send_messages() is basically:
for (;conn != NULL;) {
while ((ret = read_pdu(smpp, conn, &len, &pdu)) == 1) { //nothing since we
don't read
}
...
if (transmitter && difftime(time(NULL), smpp->throttling_err_time) >
SMPP_THROTTLING_SLEEP_TIME) {
smpp->throttling_err_time = 0;
send_messages(smpp, conn, &pending_submits);
}
}
So if send_messages() does not sleep and we are over the throughput this
basically comes down to a tight loop that in transceiver is controlled
by read_pdu() blocking on the network read.
Have you considered that?
I'm starting to think that we'll need to know 1) if we are over the throughput
2) if we are in some kind of transceiver mode or not.
>> Unfortunately this is smsc-dependant, so maybe we could move this to
>> a more generic api, and make the rule that the SMSCs MUST check that
>> api (say... conn->over_throughput()?) and move the load
>> setup/increase to the already existing callbacks?.
>
> I think, it's already enough abstracted in load object. Now it's SMSC
> dependent how to use it.
> So I don't think we need more api there but I'm open for a patch that
> would make it easier to implement
> this in other smsc modules.
>
>>
>> That way it's a bit more generic and we can implement
>> conn->sleep_for_throughput() for the smsc that need their sender
>> thread to sleep, or let the others (i.e. smpp) handle it accordingly.
>>
>> Opinions about this approach?
>>
>> [1] I didn't check the load object but I assume it account for
>> load_increases in the set interval, which would make sense.
>>
>>> Thanks,
>>> Alex
>>
>> Thanks your code, made your points a lot more clearer, I hope we can
>> come up
>> with a better implementation of throughput for all kannel users.
Damián Viano(Des).
Re: [PATCH] Guaranteed throughput smsc-independent
On Wed, Jun 10, 2009 at 05:00:21PM +0200, Alexander Malysh wrote: > false assumption, TX and MX may running in one thread (e.g. SMPP v3.4 > transceiver mode). > > see above.. > and yes, current implementation is wrong. and why should we accept fix > for a wrong version instead of > doing it right ? > > something like this: > http://github.com/amalysh/kannel/commit/31db912a9e5aecd6d9f038b56d09f408f2c256f9 I've answered all this on the mail just sent in reply to your link to your branch on github, I already checked your code, and came to this same conclusions. Damián Viano(Des).
Re: [PATCH] Guaranteed throughput smsc-independent
On Wed, Jun 10, 2009 at 03:06:59PM +0200, Alexander Malysh wrote: > here is a link to throttling branch: > http://github.com/amalysh/kannel/tree/smpp-throttling > only smpp implemented for now. Great, thanks for the link. I see that you basically break out when you are over throughput[1], I see your concern now, since the thread for receiving is the same as the sender, so you just break out to keep processing incoming pdu. Makes sense, and this indeed breaks my assumption that the smsc implementations would have a thread for sending and another one for receiving. Unfortunately this is smsc-dependant, so maybe we could move this to a more generic api, and make the rule that the SMSCs MUST check that api (say... conn->over_throughput()?) and move the load setup/increase to the already existing callbacks?. That way it's a bit more generic and we can implement conn->sleep_for_throughput() for the smsc that need their sender thread to sleep, or let the others (i.e. smpp) handle it accordingly. Opinions about this approach? [1] I didn't check the load object but I assume it account for load_increases in the set interval, which would make sense. > Thanks, > Alex Thanks your code, made your points a lot more clearer, I hope we can come up with a better implementation of throughput for all kannel users. Damián Viano(Des).
Re: [PATCH] Guaranteed throughput smsc-independent
On Wed, Jun 10, 2009 at 11:20:11AM +0300, Nikos Balkanas wrote: > Actually, I don't think that even this patch can guarantee solid throughput. > All I/O (including SMSc) is handled by a polling thread. AFAIK > bb_smscconn_sent just puts the sms in the connection queue, where the What? No. bb_smscconn_sent just account for a sent sms and generate the corresponding DLR, AND is an MUST callback for the smsc after a successful send of a message which is the reason that motivated me to assume that the smsc sender thread will call this function (confirmed by the fakesmsc). > connection layer takes over and sends it. If there is a queue for that SMSc, > the connection layer will still try to empty it as fast as it can. > > IMHO, any robust throughput should be implemented in the connection layer. The idea behind this patch is to avoid implementing it for every connection, without loosing the fact that the affected layer is the connection layer. That's exactly the reason I choose this callback to do the sleeping and no other part. > BR, > Nikos I hope I clarified your doubts about my patch, and I welcome any further input. Thanks for your point of view, I think I failed to inform better why this is supposed to work and your mail make me write it a lot clearer (I hope!). Damián Viano(Des).
Re: [PATCH] Guaranteed throughput smsc-independent
On Wed, Jun 10, 2009 at 09:38:41AM +0200, Alexander Malysh wrote: > Hi, > > not only this is issue. You want not sleep! Just think about it a bit. Sleeping was the current approach so I just followed it. I think the idea behind that (and behind the code to ensure the sleeping time is accomplished) is that the smsc have different threads for TX(MTs) and RX(MOs) so making the TX thread sleep shouldn't be a problem, this might be a false assumption. > Hint: you want to process MOs and ACKs without delays... > -1 Could you point me to some code affected by this? And please don't just tell me that the way throughput is currently implemented is wrong and therefore the patch I made to fix the current implementation is also wrong. I'd really appreciate it, since I'm only trying to help by fixing a problem. If the current (sleeping) approach is wrong, great, let's talk about it and find another way to implement it better. > Thanks, > Alex > > P.S. I already posted patch that fixes this issue but not in ideal way. > SMSC will not sleep but sending > all allowed messages in the time slot at full speed instead of delaying > each message. Full speed? I'm not sure I get that point keeping in mind that if you send for a 10 sms/s throughput, 20 sms in 1 second and then nothing for another second, you still violated the throughput, as you sent 20sms/s. Thanks for your input on my patch, I appreciate it, and hope you have time to further explain your points. Damián Viano(Des).
Re: [PATCH] Guaranteed throughput smsc-independent
On Wed, Jun 10, 2009 at 09:51:08AM +0300, Nikos Balkanas wrote: > Hi Donald, > > I think you hit the nail exactly on the head. AFAIK, there is a single thread > at this point, sms_router, that is implementing bb_smscconn_sent. Therefore > all sms in the outgoing smscs should be delayed. Kannel doesn't raise threads > on demand. Sorry I don't get your point, each smsc is delay on it's own according to my tests (4 fake smsc with 10sms/s load balanced). > There may be an additional issue. This approach, which delays everything by a > set amount if there is a throughput constraint, will utilize exactly half the Uh? > available bandwidth. If for example an smsc has a throughput of 10 sms/1', > and there is a queue of 20 sms, In the old way, assuming it works correctly, > the first 10 should be send at once at time 0, and the rest at atime 1' with > an average delay of 30"/sms. With this approach, 1 sms would be sent every > 6", with an average delay of 1'/sms. Sorry, I don't get this, could you maybe explain it in another way? If you got 20 sms in the queue and a throughput of 10, they'll be send in 2 seconds. 10 during the first second and 2 during the second second ;). Even more, with that algorithm you might take a sample of length N seconds and you should only find throughput*N outgoing messages (with each message separated by 1/throughput seconds). > Coupled with the previous point, it could spell problems for queues in large > installations that use many smscs to load balance. > > The advantage is that it offers solid throughput handling. > > I am a wap person, so I leave this decision for peoaple with a lot of smss. I'll be answering the other mails soon. Thanks for your input. Damián Viano(Des).
