GNOME Bugzilla – Bug 760757
systemclock: incorrect handling of wait requests
Last modified: 2016-01-20 11:50:19 UTC
When scheduling an alarm and inserting it at the beginning of the list gst_system_clock_id_wait_async will check if the current head entry is busy.If it is, the worker thread is already handling the head of the list and needs to be wake up to take a look at the new head. If the current head is not busy, the worker thread has not looked yet at the first entry, so inserting a new one before it is safe because the worker thread will start looking at the first entry from the list. But gst_system_clock_async_thread will choose the first entry and drop the clock lock before calling gst_system_clock_id_wait_jitter_unlocked to set the chosen entry to busy. This creates a small window of opportunity in which a new entry is inserted by gst_system_clock_id_wait_async at the head of the list without informing the worker thread and the worker thread starting to handle the chosen entry which now is the second one in the list. As a result, the new entry inserted at the beginning of the list will be ignored until the list becomes empty.
Do you want to provide a patch?
Ok, I will
Created attachment 319277 [details] [review] proposed patch My tests are passing. make gst/gstsystemclock.check and make gst/gstclock.check are passing Did not run other tests.
Review of attachment 319277 [details] [review]: ::: gst/gstsystemclock.c @@ +488,3 @@ + /* mark the entry as busy but watch out for intermediate unscheduled + * statuses */ + } while (G_UNLIKELY (!CAS_ENTRY_STATUS (entry, status, GST_CLOCK_BUSY))); This looks like a lockless routine run with the object lock held. Sounds risky, are you sure this go well in real-world cases ?
(In reply to Nicolas Dufresne (stormer) from comment #4) > Review of attachment 319277 [details] [review] [review]: > > ::: gst/gstsystemclock.c > @@ +488,3 @@ > + /* mark the entry as busy but watch out for intermediate unscheduled > + * statuses */ > + } while (G_UNLIKELY (!CAS_ENTRY_STATUS (entry, status, > GST_CLOCK_BUSY))); > > This looks like a lockless routine run with the object lock held. Sounds > risky, are you sure this go well in real-world cases ? From what I know it is safe to perform atomic operations while holding a lock. Do you have any specific concerns?
Also, do you think you might be able to come up with a unit test for this? It doesn't have to fail reliably, but if we can run something a thousand times in a loop and it fails now and then (before patch, but not after), that'd be nice.
OK, I could do something similar to test_async_order
Created attachment 319314 [details] [review] unit test to reproduce the problem this test (almost) reliable reproduces the problem. (9/10 runs) After the fix is applied, the test never fails. Increasing the number of iterations might increase the reproduction rate, but it depends on how busy the machine is and how threads are scheduled, so there is no guarantee. For my machine this was good enough to reproduce the problem without waiting too much on too many alarms.
commit e10a220a63b4eaa9f2c131da8f8161fb66dbe74a Author: Florin Apostol <florin.apostol@oregan.net> Date: Mon Jan 18 22:45:58 2016 +0000 systemclock: tests: added stress test for async order Keep inserting alarms at the beginning of the list. Due to https://bugzilla.gnome.org/show_bug.cgi?id=760757 alarm thread will get confused and not serve them in order. commit 4e2250e531df41f9795d1f6879e78110dcac9a6a Author: Florin Apostol <florin.apostol@oregan.net> Date: Mon Jan 18 16:25:20 2016 +0000 systemclock: fixed race condition in handling alarms When choosing the first entry from the list, gst_system_clock_async_thread must set the entry state to busy before releasing the clock lock. Otherwise a new entry could be added to the beginning of the list and gst_system_clock_async_thread will be unaware and keep waiting on the entry it has already chosen. Also improved messages about expected state and bumped them to ERROR level to detect unexpected state changes. https://bugzilla.gnome.org/show_bug.cgi?id=760757
Wim reviewed on IRC and said patches can be merged.