RunLoop::wakeUp() can be called by multiple thread. but ecore_pipe_write() function itself doesn't consider the thread-safety. Additional locking for the ecore pipe is needed.
Created attachment 172193 [details] Patch
Comment on attachment 172193 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172193&action=review > Source/WebCore/platform/efl/RunLoopEfl.cpp:105 > + MutexLocker locker(m_pipeLock); The caller (RunLoop::dispatch()) seems to be already thread safe (using m_functionQueueLock mutex). Could you elaborate why we need an additional mutex in RunLoop::wakeUp()? Also note that no other port seems to be using a mutex in RunLoop::wakeUp(). We should avoid mutexes unless strictly required as it may impact performance.
(In reply to comment #2) > (From update of attachment 172193 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=172193&action=review > > > Source/WebCore/platform/efl/RunLoopEfl.cpp:105 > > + MutexLocker locker(m_pipeLock); > > The caller (RunLoop::dispatch()) seems to be already thread safe (using m_functionQueueLock mutex). Could you elaborate why we need an additional mutex in RunLoop::wakeUp()? The mutex that is used in RunLoop::dispatch() is not for the wakeUp() but for the m_functionQueue. So I have a patch about this. (You can see the details in Bug 98580) As you can see in the comment of the bug, RunLoop::wakeUp() is used in the Connection::wakeUpRunLoop() without any protection. So protecting wakeUp() itself is needed. > Also note that no other port seems to be using a mutex in RunLoop::wakeUp(). We should avoid mutexes unless strictly required as it may impact performance. Yes, protecting with mutex can make some performance issue. But as I know, other ports use thread-safe function for wake up loop. Unfortunately, EFL doesn't provide such thing for now. (And if it provide such API, the API will use mutex or some other protection mechanism to ensure the thread-safety)
Comment on attachment 172193 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=172193&action=review >>> Source/WebCore/platform/efl/RunLoopEfl.cpp:105 >>> + MutexLocker locker(m_pipeLock); >> >> The caller (RunLoop::dispatch()) seems to be already thread safe (using m_functionQueueLock mutex). Could you elaborate why we need an additional mutex in RunLoop::wakeUp()? >> Also note that no other port seems to be using a mutex in RunLoop::wakeUp(). We should avoid mutexes unless strictly required as it may impact performance. > > The mutex that is used in RunLoop::dispatch() is not for the wakeUp() but for the m_functionQueue. So I have a patch about this. (You can see the details in Bug 98580) > As you can see in the comment of the bug, RunLoop::wakeUp() is used in the Connection::wakeUpRunLoop() without any protection. > So protecting wakeUp() itself is needed. Byungwoo, I asked to raster if ecore_pipe_write is thread-safety. According to raster, ecore_pipe_write is intended for the thread. As far as I understand, ecore_pipe_write is one-way communication from thread to mainloop which created a pipe. Then, thread just writes data to the pipe. So, raster said ecore_pipe_write is thread safety. How do you think ?
(In reply to comment #4) > (From update of attachment 172193 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=172193&action=review > > >>> Source/WebCore/platform/efl/RunLoopEfl.cpp:105 > >>> + MutexLocker locker(m_pipeLock); > >> > >> The caller (RunLoop::dispatch()) seems to be already thread safe (using m_functionQueueLock mutex). Could you elaborate why we need an additional mutex in RunLoop::wakeUp()? > >> Also note that no other port seems to be using a mutex in RunLoop::wakeUp(). We should avoid mutexes unless strictly required as it may impact performance. > > > > The mutex that is used in RunLoop::dispatch() is not for the wakeUp() but for the m_functionQueue. So I have a patch about this. (You can see the details in Bug 98580) > > As you can see in the comment of the bug, RunLoop::wakeUp() is used in the Connection::wakeUpRunLoop() without any protection. > > So protecting wakeUp() itself is needed. > > Byungwoo, I asked to raster if ecore_pipe_write is thread-safety. According to raster, ecore_pipe_write is intended for the thread. As far as I understand, ecore_pipe_write is one-way communication from thread to mainloop which created a pipe. Then, thread just writes data to the pipe. So, raster said ecore_pipe_write is thread safety. How do you think ? Really? I checked the ecore_pipe_write() in ecore v1.7.1. First of all, there is no locking in the function. And the function has two parts. 1. writing the length of data to the pipe. 2. writing the data to the pipe. As my understanding, to ensure thread-safety of the ecore_pipe_write(), the writing operation of all those ([data length][raw data]) should be atomic. If multiple threads call the ecore_pipe_write() at one time, the order of pipe data can be invalid - like [length of A][length of B][data of B][data of A]. I'm not sure that _ecore_pipe_read has some protection or recovery logic about this situation. Please let me know if there are some misunderstanding.
I agree with Byungwoo Lee. It seems to me that ecore_pipe_write() in ecore v1.7.1 is not thread-safe because it has the possibility of message format broken when writing messages at the same times. It is my opinion.
For the record, I agree with byungwoo as well.
According to raster, ecore_pipe_write is intended for a thread. So, he recommends to make an ecore_pipe per thread. I also think we need to use a lock for current usage. But, I'm not clear to use ecore_pipe_write here yet. If this is urgent issue, I don't mind to land this code at the moment. If not, I would like to dig this a little further. I wanna check why ecore_pipe_write is used here.
(In reply to comment #8) > According to raster, ecore_pipe_write is intended for a thread. So, he recommends to make an ecore_pipe per thread. I also think we need to use a lock for current usage. But, I'm not clear to use ecore_pipe_write here yet. If this is urgent issue, I don't mind to land this code at the moment. If not, I would like to dig this a little further. I wanna check why ecore_pipe_write is used here. Yes, it is the fundamental question that 'Why EFL WebKit uses the ecore pipe to wake up main loop?' :) Maybe there are many people who have the same question, and so do I. Is there anyone who knows the history about this? (Maybe it is related with some priority between the loop event items, but I'm not sure)
(In reply to comment #9) > (In reply to comment #8) > > According to raster, ecore_pipe_write is intended for a thread. So, he recommends to make an ecore_pipe per thread. I also think we need to use a lock for current usage. But, I'm not clear to use ecore_pipe_write here yet. If this is urgent issue, I don't mind to land this code at the moment. If not, I would like to dig this a little further. I wanna check why ecore_pipe_write is used here. > > Yes, it is the fundamental question that 'Why EFL WebKit uses the ecore pipe to wake up main loop?' :) > > Maybe there are many people who have the same question, and so do I. > > Is there anyone who knows the history about this? > (Maybe it is related with some priority between the loop event items, but I'm not sure) Well, EFL also uses ecore pipe to wake up the main loop it seems so WebKit is no different. See for example the implementation of ecore_main_loop_thread_safe_call_async(): it is using ecore pipe if not in the main thread.
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > According to raster, ecore_pipe_write is intended for a thread. So, he recommends to make an ecore_pipe per thread. I also think we need to use a lock for current usage. But, I'm not clear to use ecore_pipe_write here yet. If this is urgent issue, I don't mind to land this code at the moment. If not, I would like to dig this a little further. I wanna check why ecore_pipe_write is used here. > > > > Yes, it is the fundamental question that 'Why EFL WebKit uses the ecore pipe to wake up main loop?' :) > > > > Maybe there are many people who have the same question, and so do I. > > > > Is there anyone who knows the history about this? > > (Maybe it is related with some priority between the loop event items, but I'm not sure) > > Well, EFL also uses ecore pipe to wake up the main loop it seems so WebKit is no different. See for example the implementation of ecore_main_loop_thread_safe_call_async(): it is using ecore pipe if not in the main thread. Also note that _ecore_main_loop_thread_safe_call() is protecting pipe write with a mutex as well. I think Byungwoo's patch is good.
Comment on attachment 172193 [details] Patch This patch is fine for current implementation.
Byungwoo, are you landing this? :)
Comment on attachment 172193 [details] Patch Yes please :) I tested and there was no regression on layout test and API test.
Comment on attachment 172193 [details] Patch Rejecting attachment 172193 [details] from commit-queue. christophe.dumez@intel.com does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.
I'm in committers.py but apparently the commit queue has not restarted yet :(
Comment on attachment 172193 [details] Patch Clearing flags on attachment: 172193 Committed r136006: <http://trac.webkit.org/changeset/136006>
All reviewed patches have been landed. Closing bug.