[asterisk-dev] [Code Review] Append two container for dialogs to delete and for rtp timeout checks to replace all dialogs container in ao2 callback for dialogs_needdestroy
sst at sil.at
sst at sil.at
Thu Sep 16 16:03:30 CDT 2010
> On 2010-09-16 12:43:28, David Vossel wrote:
> > This is looking great, nice work on cleaning up some of those ref counting issues.
> >
> > Other than what I pointed out below there is only one other thing I noticed. I have no idea why, but for some reason we explicitly set "p->needdestroy = 0" in sip_hangup(). Is it possible for this to happen after needdestroy is already set to 1? if so we need to unlink from the needdestroy container there.
> >
> >
> > As far as testing goes, from your description I see you have done extensive performance testing. Since we are touching the RTP timeout code quite a bit, can you test that we continue to timeout correctly as well?
i didnt thought such situation will occure but if its in the code i will take care of it.
you can see above the sipp output with 500 concurent calls, and using a rtptimeout of 10 sec. each call tooks around 10,97 sec. if there is more incoming traffic or scheduled events this would be even shorter. thats why i wanted to set down the ast_sched_wait maxtimeout down from 1 sec to 100 ms.
> On 2010-09-16 12:43:28, David Vossel wrote:
> > branches/1.6.2/channels/chan_sip.c, lines 15229-15230
> > <https://reviewboard.asterisk.org/r/917/diff/5/?file=12525#file12525line15229>
> >
> > It seems to me like the unlinking of the dialog from the needdestroy and rtpcheck containers should be done in the dialog_unlink_all() function.
the rtpcheck container should be in a different function cause its easy possible a dialog should be removed from rtpchecks but not from the dialogs.
the needdestroy could be moved in there. will do so.
> On 2010-09-16 12:43:28, David Vossel wrote:
> > branches/1.6.2/channels/chan_sip.c, lines 10533-10535
> > <https://reviewboard.asterisk.org/r/917/diff/5/?file=12525#file12525line10533>
> >
> > there is a 'dialog_initialize_rtp()' function. Should we be linking into the rtpcheck container there instead of here?!
i will check this.
- schmidts
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/917/#review2732
-----------------------------------------------------------
On 2010-09-13 18:09:58, schmidts wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/917/
> -----------------------------------------------------------
>
> (Updated 2010-09-13 18:09:58)
>
>
> Review request for Asterisk Developers.
>
>
> Summary
> -------
>
> Instead of iterate through all sip dialogs in the do_monitor function of chan_sip.c i created two separate containers which links to dialogs.
> The container dialogs_needdestroy links to dialogs which are marked as needdestroy, so they should be unrefered from the dialogs container.
> The container dialogs_rtpcheck links to dialogs which have active rtp, vrtp or trtp and rtptimeout, rtpholdtimeout or rtpkeepalive activated.
>
> normaly the container with needdestroy dialogs is empty so the callback takes only around 30 usec. even if there are more than 10 dialogs to be destroyed it takes around 200 usec.
> the callback for the rtpcheck tooks around 200 usec with 30 concurrent calls and 700 usec if the timeout is reached to do a softhangup.
>
> further i have found in check_rtp_timeout a possible locking situation. i am not sure if this would be a working solution cause it could happen that a call would be hung up several ms or also 1 sec later.
>
> i think after speeding the ao2 callback up, its not necessary to wait 1000 ms, with 100ms dialogs would be faster released, the timeout fires even shorter and scheduled events which has been scheduled in a very short future time would be fired faster. see change 40 for this.
>
> i am also not sure if its necessary to iterate through the containers on module_unload to unlink the entries. the dialogs itself would be destroyed after they are unrefered from the dialogs container. see change 42 for this.
>
>
>
> This addresses bug 17912.
> https://issues.asterisk.org/view.php?id=17912
>
>
> Diffs
> -----
>
> branches/1.6.2/channels/chan_sip.c 286455
>
> Diff: https://reviewboard.asterisk.org/r/917/diff
>
>
> Testing
> -------
>
> i have tested this with sipp and 500 concurrent calls all with rtptimeout 10 s and the active time was 10,9s per call.
> also the 20k peers register test run through without any problem. (with the astdbpatch)
>
> Messages Retrans Timeout Unexpected-Msg
> INVITE ----------> 500 0
> 100 <---------- 500 0 0
> 200 <---------- 500 0 0
> ACK ----------> 500 0
> Pause [ 40.0s] 500 500
> BYE ----------> 0 0 0
> 200 <---------- 0 0 0
> ------------------------------ Test Terminated --------------------------------
>
>
> ----------------------------- Statistics Screen ------- [1-9]: Change Screen --
> Start Time | 2010-09-11 23:12:32
> Last Reset Time | 2010-09-11 23:12:47
> Current Time | 2010-09-11 23:12:48
> -------------------------+---------------------------+--------------------------
> Counter Name | Periodic value | Cumulative value
> -------------------------+---------------------------+--------------------------
> Elapsed Time | 00:00:00:964 | 00:00:15:994
> Call Rate | 0.000 cps | 31.262 cps
> -------------------------+---------------------------+--------------------------
> Incoming call created | 0 | 0
> OutGoing call created | 0 | 500
> Total Call created | | 500
> Current Call | 0 |
> -------------------------+---------------------------+--------------------------
> Successful call | 0 | 0
> Failed call | 35 | 500
> -------------------------+---------------------------+--------------------------
> Call Length | 00:00:10:970 | 00:00:10:637
> ------------------------------ Test Terminated --------------------------------
>
>
> Thanks,
>
> schmidts
>
>
More information about the asterisk-dev
mailing list