[asterisk-dev] [Code Review] 4483: Separate QoS settings for trunk packets

[Y Ateya]

> On March 23, 2015, 7:52 p.m., Matt Jordan wrote:
> > trunk/channels/chan_iax2.c, lines 345-347
> > <https://reviewboard.asterisk.org/r/4483/diff/2/?file=72090#file72090line345>
> >
> >     This lock concerns me a lot.
> >     
> >     Because you are looking to toggle the QOS setting on the socket, you have to protect access to the socket across all threads. That substantially increases the locking on the socket in chan_iax2.
> >     
> >     At a minimum, this is called from iax2_write, which is called by the PBX threads servicing the channels. As a result, this would globally lock all IAX2 channels on every frame written to any IAX2 channel.
> >     
> >     While this is an interesting idea, the impact of the implementation here feels rather dramatic.
> 
> Joshua Colp wrote:
>     +1 to this. Have you run many simultaneous channels with it in trunking and non-trunking mode?

@Matt: Yes, the lock is dramatic. I can remove the locks, but some packets will be sent with the wrong qos settings.

@Joshua: I tried only trunked calls.


- Y


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/4483/#review14772
-----------------------------------------------------------


On March 12, 2015, 8:11 p.m., Y Ateya wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4483/
> -----------------------------------------------------------
> 
> (Updated March 12, 2015, 8:11 p.m.)
> 
> 
> Review request for Asterisk Developers and mattjordan.
> 
> 
> Bugs: ASTERISK-24866
>     https://issues.asterisk.org/jira/browse/ASTERISK-24866
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Currently the packet priority is assigned to all packets sent from IAX socket; Which minimizes the benefit of having packet priority.
> 
> This patch adds separate qos settings for IAX trunk packets.
> 
> Summary:
>  - Added trunk_cos and trunk_tos configuraiton variables (similar to cos and tos) to set qos values for trunk packets.
>  - Added dynamic_qos configuration variables (true/false) to enable/disable changing qos dynamically. (default disabled).
>  - Before calling ast_send (and if dynamic_qos is enabled) update qos settings of the socket (if it needs change only).
>  - Lock a mutex before setting qos and unlock it after ast_send. To prevent thread1 setting qos, thread two setting qos and send, then thread1 uses thread2 qos.
> 
> Notes:
>  - Protecting `qos_dynamic_enabled` in reload configurations complicated locking mechanism a little bit.
>  - More qos classes can be added to different packet types later.
> 
> 
> Diffs
> -----
> 
>   trunk/channels/chan_iax2.c 432806 
> 
> Diff: https://reviewboard.asterisk.org/r/4483/diff/
> 
> 
> Testing
> -------
> 
> Run 100 calls (over IAX trunk) between client and server. Checked that:
>  - If cos, tos are set but dynamic_qos is disabled, tos and cos values are used.
>  - If tos, cos, dynamic_qos, trunk_cos and trunk_tos are set; trunk packets uses trunk_cos/trunk_tos while all other packets use cos/tos.
> 
> 
> Thanks,
> 
> Y Ateya
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20150324/9459b24e/attachment.html>