[libnice] Sending packets uses a static, global lock?

[Anand Sivaram]

What about having a global flag for the lock type?
Set it with g_object_set during the library initialization.
It should have 3 values - Off, Per Agent, Global.
Modify agent_lock() and agent_unlock() accordingly.

That way if the applications using libnice implemented their own locking
mechanism, that could be used seamlessly.

anand




On 12 October 2016 at 21:35, Lorenzo Miniero <lminiero at gmail.com> wrote:

> Thanks for the context! (and sorry for the delay in this response...)
>
> I'm not familiar enough with glib2, unfortunately: considering the patch
> dates back to about 7 years ago, is the problem the global muxed fixed
> still an issue with current versions? We did a quick and dirty test by
> turning those global agent locks to per-agent locks, where possible (that
> is, in methods that actually provided a pointer to an agent instance) and
> we did notice improvements in the performance (about twice as many sessions
> before it started to break down), but of course this means little if it
> risks causing those race conditions and crashes again. Is this road worth
> exploring? I'm willing to help and contribute to such work, of course,
> although I admit I'm not an expert on the libnice internals (but it's never
> too late to learn! ;-) ).
>
> Thanks,
> Lorenzo
>
>
> 2016-10-05 13:09 GMT+02:00 Miguel París Díaz <mparisdiaz at gmail.com>:
>
>> Sorry for the copy-paste error.
>>
>>
>>
>> 2016-10-05 13:06 GMT+02:00 Miguel París Díaz <mparisdiaz at gmail.com>:
>>
>>> Good point Lorenzo!!!
>>>
>>> This change was added in this commit:
>>>
>>> commit 2faa76a4840100bebd9532bd47a2755874c5b3a0
>>> Author: Youness Alaoui <youness.alaoui at collabora.co.uk>
>>> Date:   Tue Jun 16 20:40:33 2009 -0400
>>>
>>>     Use a global mutex for all nice agents and use g_source_is_destroyed
>>> to avoid the race condition of a source being destroyed while we wait for
>>> the mutex
>>>
>>> And then the RecMutex was changed by a Mutex in:
>>>
>>
>> commit 1deee69325284c726c3a8380a7d5839a51e20c48
>> Author: Olivier Crête <olivier.crete at collabora.com>
>> Date:   Mon Feb 24 18:51:31 2014 -0500
>>
>>     agent: Replace recursive mutex with non-recursive
>>
>>     This should prevent us from re-adding re-entrancy in the future
>>
>>
>>>
>>>
>>> 2016-10-05 11:45 GMT+02:00 Lorenzo Miniero <lminiero at gmail.com>:
>>>
>>>> Hi all,
>>>>
>>>> we've been working on some stress tests of our Janus WebRTC server,
>>>> which uses libnice for all the ICE related stuff. While it works great in
>>>> general, we reach a wall much sooner than we expect, and performance start
>>>> to degrade quite a bit. By doing some digging and diagnostic in the code we
>>>> wrote and the one we use, we found out something we thought was weird.
>>>>
>>>> Specifically, we noticed that the internal method that is used to send
>>>> packets, nice_agent_send_messages_nonblocking (which nice_agent_send
>>>> itself uses internally) has a lot of code wrapped in an
>>>> agent_lock()/agent_unlock(), which locks/unlocks a global mutex, and this
>>>> includes the process of physiclly sending the packet. This means that the
>>>> whole library is locked until that code is called, which seems overkill. I
>>>> guess the same happens in other parts of the code as well. I'd understand
>>>> locking the specific agent if the purpose is thread-safety, but why lock
>>>> ALL agents at the same time for each packet you send? I tried searching the
>>>> archives for some info on that, but couldn't find any discussion related to
>>>> this choice.
>>>>
>>>> Since I'm already handling thread-safety for most stuff in my own code,
>>>> is there any way for preventing this from happening? I guess that one
>>>> possible way could be to retrieve the file descriptor from the agent object
>>>> and send the packet manually myself, but while this may work in simple
>>>> (host/prflx/srflx) connectivity, this would definitely not be the case
>>>> when, for instance, TURN is involved, as that's one of the things the
>>>> library would take care of for me.
>>>>
>>>> Looking forward to your thoughts, thanks!
>>>> Lorenzo
>>>>
>>>> _______________________________________________
>>>> nice mailing list
>>>> nice at lists.freedesktop.org
>>>> https://lists.freedesktop.org/mailman/listinfo/nice
>>>>
>>>>
>>>
>>>
>>> --
>>> Miguel París Díaz
>>> ------------------------------------------------------------------------
>>> Computer/Software engineer.
>>> Researcher and architect in http://www.kurento.org
>>> http://twitter.com/mparisdiaz
>>> ------------------------------------------------------------------------
>>>
>>
>>
>>
>> --
>> Miguel París Díaz
>> ------------------------------------------------------------------------
>> Computer/Software engineer.
>> Researcher and architect in http://www.kurento.org
>> http://twitter.com/mparisdiaz
>> ------------------------------------------------------------------------
>>
>
>
> _______________________________________________
> nice mailing list
> nice at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/nice
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/nice/attachments/20161013/ea7c5d1e/attachment-0001.html>