Re: [PATCH 1/1] dix: Hold the input_lock() when accessing the miEventQueue through checkForInput
Jeremy Huddleston Sequoia writes: > Thoughts? Sounds like a plan. For functions which aren't called frequently, just grabbing the input_lock would be fine, I was mostly concerned with the dispatch loop. -- -keith signature.asc Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/1] dix: Hold the input_lock() when accessing the miEventQueue through checkForInput
> On Sep 19, 2016, at 09:52, Keith Packard wrote:
>
> Jeremy Huddleston Sequoia writes:
>
>> Yeah, I made the change mainly to shutup the analyzer while I was
>> looking for other races. I decided to propose it in case we want to
>> be strict here.
>
> Thanks. Keeping our request processing overhead low seems important
> enough to me that we should skip this patch, although perhaps adding a
> comment before the test noting that we are intentionally not holding the
> input lock.
>
> Is there some way we can annotate the code to silence the analyzer in
> these cases?
The annotation is at a per-function level. We can basically say "if you notice
data races in this funciton, don't bother reporting them". I'm doing this for
a similar case where we want to limit overhead on the fastpath in
darwinEvents.c. In the code below, TSan would normally complain about the read
of mieqInitialized without holding the mieqInitializedMutex, but we know it's
actually safe in this case.
We could add something like _X_NOTSAN to Xfuncproto.h to cover this and then
break out the actual comparisons into an inline function with that attribute.
Breaking that out into separate inline functions allows us to surgically apply
no_sanitize_thread.
Thoughts?
static BOOL mieqInitialized;
static pthread_mutex_t mieqInitializedMutex = PTHREAD_MUTEX_INITIALIZER;
static pthread_cond_t mieqInitializedCond = PTHREAD_COND_INITIALIZER;
#ifdef __has_feature
# if __has_feature(thread_sanitizer)
# define __tsan_ignore __attribute__((no_sanitize_thread))
# else
# define __tsan_ignore /**/
# endif
#else
# define __tsan_ignore /**/
#endif
__tsan_ignore
extern inline void
wait_for_mieq_init(void)
{
if (!mieqInitialized) {
pthread_mutex_lock(&mieqInitializedMutex);
while (!mieqInitialized) {
pthread_cond_wait(&mieqInitializedCond, &mieqInitializedMutex);
}
pthread_mutex_unlock(&mieqInitializedMutex);
}
}
__tsan_ignore
static inline void
signal_mieq_init(void)
{
pthread_mutex_lock(&mieqInitializedMutex);
mieqInitialized = TRUE;
pthread_cond_broadcast(&mieqInitializedCond);
pthread_mutex_unlock(&mieqInitializedMutex);
}
smime.p7s
Description: S/MIME cryptographic signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/1] dix: Hold the input_lock() when accessing the miEventQueue through checkForInput
Jeremy Huddleston Sequoia writes: > Yeah, I made the change mainly to shutup the analyzer while I was > looking for other races. I decided to propose it in case we want to > be strict here. Thanks. Keeping our request processing overhead low seems important enough to me that we should skip this patch, although perhaps adding a comment before the test noting that we are intentionally not holding the input lock. Is there some way we can annotate the code to silence the analyzer in these cases? -- -keith signature.asc Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/1] dix: Hold the input_lock() when accessing the miEventQueue through checkForInput
> On Sep 19, 2016, at 08:30, Keith Packard wrote: > > Jeremy Huddleston Sequoia writes: > >> == >> WARNING: ThreadSanitizer: data race (pid=4943) >> Read of size 4 at 0x00010c4e3854 by thread T8: >>#0 WaitForSomething WaitFor.c:237 (X11.bin+0x00010049216c) >>#1 Dispatch dispatch.c:413 (X11.bin+0x000100352ed9) >>#2 dix_main main.c:287 (X11.bin+0x00010036e894) >>#3 server_thread quartzStartup.c:66 (X11.bin+0x000100039e63) >> >> Previous write of size 4 at 0x00010c4e3854 by thread T12 (mutexes: write >> M856, write M1976): >>#0 mieqEnqueue mieq.c:263 (X11.bin+0x000100448d14) >>#1 DarwinSendDDXEvent darwinEvents.c:641 (X11.bin+0x000100033613) >>#2 DarwinProcessFDAdditionQueue_thread darwinEvents.c:338 >> (X11.bin+0x000100032039) > > I'm not sure I want to resolve this "bug" -- the event queue has been > designed to be safe in a lockless threaded environment as it was originally > designed to directly map a kernel address which would be modified at > interrupt time. Is there an actual issue with the design? Yeah, I made the change mainly to shutup the analyzer while I was looking for other races. I decided to propose it in case we want to be strict here. I don't see an absolute need to do this. The code looks perfectly safe to me without this change because the data is only read and compared. There could be issues with reads being nonatomic and getting back some meaningless value, but the outcome of that failure would just be an extra unneeded call into ProcessInputEvents(), which is perfectly safe. smime.p7s Description: S/MIME cryptographic signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
Re: [PATCH 1/1] dix: Hold the input_lock() when accessing the miEventQueue through checkForInput
Jeremy Huddleston Sequoia writes: > == > WARNING: ThreadSanitizer: data race (pid=4943) > Read of size 4 at 0x00010c4e3854 by thread T8: > #0 WaitForSomething WaitFor.c:237 (X11.bin+0x00010049216c) > #1 Dispatch dispatch.c:413 (X11.bin+0x000100352ed9) > #2 dix_main main.c:287 (X11.bin+0x00010036e894) > #3 server_thread quartzStartup.c:66 (X11.bin+0x000100039e63) > > Previous write of size 4 at 0x00010c4e3854 by thread T12 (mutexes: write > M856, write M1976): > #0 mieqEnqueue mieq.c:263 (X11.bin+0x000100448d14) > #1 DarwinSendDDXEvent darwinEvents.c:641 (X11.bin+0x000100033613) > #2 DarwinProcessFDAdditionQueue_thread darwinEvents.c:338 > (X11.bin+0x000100032039) I'm not sure I want to resolve this "bug" -- the event queue has been designed to be safe in a lockless threaded environment as it was originally designed to directly map a kernel address which would be modified at interrupt time. Is there an actual issue with the design? -- -keith signature.asc Description: PGP signature ___ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: https://lists.x.org/mailman/listinfo/xorg-devel
[PATCH 1/1] dix: Hold the input_lock() when accessing the miEventQueue through checkForInput
== WARNING: ThreadSanitizer: data race (pid=4943) Read of size 4 at 0x00010c4e3854 by thread T8: #0 WaitForSomething WaitFor.c:237 (X11.bin+0x00010049216c) #1 Dispatch dispatch.c:413 (X11.bin+0x000100352ed9) #2 dix_main main.c:287 (X11.bin+0x00010036e894) #3 server_thread quartzStartup.c:66 (X11.bin+0x000100039e63) Previous write of size 4 at 0x00010c4e3854 by thread T12 (mutexes: write M856, write M1976): #0 mieqEnqueue mieq.c:263 (X11.bin+0x000100448d14) #1 DarwinSendDDXEvent darwinEvents.c:641 (X11.bin+0x000100033613) #2 DarwinProcessFDAdditionQueue_thread darwinEvents.c:338 (X11.bin+0x000100032039) Location is global 'miEventQueue' at 0x00010c4e3850 (X11.bin+0x0001005ab854) Mutex M856 (0x00010c4c8c80) created at: #0 pthread_mutex_lock :144 (libclang_rt.tsan_osx_dynamic.dylib+0x000321fe) #1 DarwinListenOnOpenFD darwinEvents.c:300 (X11.bin+0x000100031607) #2 socket_handoff bundle-main.c:288 (X11.bin+0x00012b40) #3 __do_request_fd_handoff_socket_block_invoke bundle-main.c:379 (X11.bin+0x000129ba) #4 __tsan::invoke_and_release_block(void*) :144 (libclang_rt.tsan_osx_dynamic.dylib+0x0005d97b) #5 _dispatch_client_callout :33 (libdispatch.dylib+0x20ef) Mutex M1976 (0x00010c4e3d68) created at: #0 pthread_mutex_init :144 (libclang_rt.tsan_osx_dynamic.dylib+0x000253c3) #1 input_lock inputthread.c:103 (X11.bin+0x00010049fd10) #2 TimerSet WaitFor.c:343 (X11.bin+0x0001004926c2) #3 RootlessQueueRedisplay rootlessScreen.c:594 (X11.bin+0x000100065d7f) #4 RootlessInstallColormap rootlessScreen.c:514 (X11.bin+0x000100069f1a) #5 miSpriteInstallColormap misprite.c:562 (X11.bin+0x000100467095) #6 miCreateDefColormap micmap.c:270 (X11.bin+0x000100440399) #7 DarwinScreenInit darwin.c:285 (X11.bin+0x0001000303bb) #8 AddScreen dispatch.c:3908 (X11.bin+0x00010036c417) #9 InitOutput darwin.c:671 (X11.bin+0x00010002fdeb) #10 dix_main main.c:197 (X11.bin+0x00010036e228) #11 server_thread quartzStartup.c:66 (X11.bin+0x000100039e63) Thread T8 (tid=4198779, running) created by main thread at: #0 pthread_create :144 (libclang_rt.tsan_osx_dynamic.dylib+0x00024490) #1 create_thread quartzStartup.c:78 (X11.bin+0x000100039dad) #2 QuartzInitServer quartzStartup.c:95 (X11.bin+0x000100039c16) #3 X11ApplicationMain X11Application.m:1238 (X11.bin+0x00010001cde4) #4 X11ControllerMain X11Controller.m:984 (X11.bin+0x00010002a642) #5 server_main quartzStartup.c:136 (X11.bin+0x00010003a03b) #6 do_start_x11_server bundle-main.c:436 (X11.bin+0x00012eb5) #7 _Xstart_x11_server mach_startupServer.c:189 (X11.bin+0x00014e99) #8 mach_startup_server mach_startupServer.c:399 (X11.bin+0x00015734) #9 mach_msg_server mach_msg.c:563 (libsystem_kernel.dylib+0x00012186) #10 start :29 (libdyld.dylib+0x5254) Thread T12 (tid=4198797, running) created by thread T8 at: #0 pthread_create :144 (libclang_rt.tsan_osx_dynamic.dylib+0x00024490) #1 create_thread darwinEvents.c:121 (X11.bin+0x000100031ecf) #2 DarwinEQInit darwinEvents.c:365 (X11.bin+0x000100031860) #3 InitInput darwin.c:571 (X11.bin+0x00010002ea09) #4 dix_main main.c:261 (X11.bin+0x00010036e7ce) #5 server_thread quartzStartup.c:66 (X11.bin+0x000100039e63) SUMMARY: ThreadSanitizer: data race WaitFor.c:237 in WaitForSomething == == WARNING: ThreadSanitizer: data race (pid=22841) Write of size 4 at 0x000105bbd864 by main thread (mutexes: write M1945): #0 mieqEnqueue mieq.c:263 (X11.bin+0x000100448cf4) #1 DarwinSendDDXEvent darwinEvents.c:642 (X11.bin+0x000100033693) #2 -[X11Controller set_window_menu:] X11Controller.m:275 (X11.bin+0x0001000222fd) #3 -[X11Application set_window_menu:] X11Application.m:486 (X11.bin+0x000100018b44) #4 -[X11Application handleMachMessage:] X11Application.m:177 (X11.bin+0x000100016678) #5 __NSFireMachPort :69 (Foundation+0x0009b62b) #6 X11ControllerMain X11Controller.m:984 (X11.bin+0x00010002a5f2) #7 server_main quartzStartup.c:136 (X11.bin+0x000100039ffb) #8 do_start_x11_server bundle-main.c:436 (X11.bin+0x00012e65) #9 _Xstart_x11_server mach_startupServer.c:189 (X11.bin+0x00014e49) #10 mach_startup_server mach_startupServer.c:399 (X11.bin+0x000156e4) #11 mach_msg_server mach_msg.c:563 (libsystem_kernel.dylib+0x00012186) #12 start :29 (libdyld.dylib+0x5254) Previous read of size 4 at 0x000105bbd864 by thread T7: #0 Dispatch dispatch.c:434 (X11.bin+0x000100352fc8) #1 dix_main main.c:287 (X11.bin+0x00010036e874) #2 server_thread quartzStartup.c:66 (X11.bin+0x000100039e23) Location is global 'miEventQueue' at 0x000105bbd860 (X11.bin+0x0001005ab864) Mutex M1945 (0x000105bbdd78) created at: #0 pthread_mutex_init :144 (libclang_rt.tsan_osx_dynamic.dylib+0x000253c3) #1 input_
