Fix for data race in mark_rect_dirty_nc

[ceeac]

This patch fixes this data race:

Code Select Expand


==================
WARNING: ThreadSanitizer: data race (pid=54573)
  Write of size 4 at 0x7b6000005438 by thread T16:
    #0 mark_rect_dirty_nc(short, short, short, short) display/simgraph16.cc:1021:25 (sim+0x5fae33)
    #1 mark_rect_dirty_clip(short, short, short, short, signed char) display/simgraph16.cc:1067:3 (sim+0x5fb23a)
    #2 display_img_aux(unsigned int, short, short, signed char, int, int, signed char) display/simgraph16.cc:2634:7 (sim+0x61af8e)
    #3 grund_t::display_boden(short, short, short, signed char, bool) const boden/grund.cc:1148:5 (sim+0x541e67)
    #4 grund_t::display_if_visible(short, short, short, signed char, bool) boden/grund.cc:1267:3 (sim+0x543260)
    #5 main_view_t::display_region(koord, koord, short, short, bool, bool, signed char) display/simview.cc:396:11 (sim+0x632278)
    #6 display_region_thread(void*) display/simview.cc:73:23 (sim+0x631b10)

  Previous write of size 4 at 0x7b6000005438 by main thread:
    #0 mark_rect_dirty_nc(short, short, short, short) display/simgraph16.cc:1021:25 (sim+0x5fae33)
    #1 mark_rect_dirty_clip(short, short, short, short, signed char) display/simgraph16.cc:1067:3 (sim+0x5fb23a)
    #2 display_img_aux(unsigned int, short, short, signed char, int, int, signed char) display/simgraph16.cc:2634:7 (sim+0x61af8e)
    #3 grund_t::display_boden(short, short, short, signed char, bool) const boden/grund.cc:1148:5 (sim+0x541e67)
    #4 grund_t::display_if_visible(short, short, short, signed char, bool) boden/grund.cc:1267:3 (sim+0x543260)
    #5 main_view_t::display_region(koord, koord, short, short, bool, bool, signed char) display/simview.cc:396:11 (sim+0x632278)
    #6 main_view_t::display(bool) display/simview.cc:236:3 (sim+0x6343e9)
    #7 intr_refresh_display(bool) simintr.cc:81:16 (sim+0xa84633)
    #8 karte_t::sync_step(unsigned int, bool, bool) simworld.cc:3587:3 (sim+0xb326c6)
    #9 modal_dialogue(gui_frame_t*, long, karte_t*, bool (*)()) simmain.cc:261:10 (sim+0xa8f1ad)
    #10 simu_main(int, char**) simmain.cc:1429:4 (sim+0xa96003)
    #11 sysmain(int, char**) simsys.cc:1096:9 (sim+0xac71f6)
    #12 main simsys_s2.cc:788:9 (sim+0xc284bb)

  Location is heap block of size 912 at 0x7b6000005400 allocated by main thread:
    #0 malloc <null> (sim+0x430d04)
    #1 xmalloc(unsigned long) simmem.cc:15:18 (sim+0xa98cf8)
    #2 simgraph_init(short, short, int) display/simgraph16.cc:5092:19 (sim+0x62a1f3)
    #3 simu_main(int, char**) simmain.cc:844:2 (sim+0xa91b0f)
    #4 sysmain(int, char**) simsys.cc:1096:9 (sim+0xac71f6)
    #5 main simsys_s2.cc:788:9 (sim+0xc284bb)

  Thread T16 (tid=54594, running) created by main thread at:
    #0 pthread_create <null> (sim+0x4325cb)
    #1 main_view_t::display(bool) display/simview.cc:201:10 (sim+0x633e07)
    #2 intr_refresh_display(bool) simintr.cc:81:16 (sim+0xa84633)
    #3 karte_t::sync_step(unsigned int, bool, bool) simworld.cc:3587:3 (sim+0xb326c6)
    #4 modal_dialogue(gui_frame_t*, long, karte_t*, bool (*)()) simmain.cc:261:10 (sim+0xa8f1ad)
    #5 simu_main(int, char**) simmain.cc:1429:4 (sim+0xa96003)
    #6 sysmain(int, char**) simsys.cc:1096:9 (sim+0xac71f6)
    #7 main simsys_s2.cc:788:9 (sim+0xc284bb)

SUMMARY: ThreadSanitizer: data race display/simgraph16.cc:1021:25 in mark_rect_dirty_nc(short, short, short, short)
==================


[TurfIt]

Access to the dirty tile array was supposed to be by uint8 to prevent this, however someone made changes... see Patches for big-endian machines, clang, no /proc
specifically:

Faulty memory as usual. Using:
Code: [Select]

   ((uint8*)tile_dirty)[bit >> 3] |= 1 << (bit & 7);

instead of:
Code: [Select]

   tile_dirty[bit >> 5] |= 1 << (bit &317);

wasn't due to speed, but artifacting!  Hint: multithreaded non-mutexed access in the drawing routines.

A mutex in that location kills performance to the point you might as well remove the multi threading.
IMHO the uint32 access should be applied only for the big endian architecture (accepting possible graphic artifacts) and leave the rest with uint8.

EDIT: or leave this as uint32 and force wh_x in main_view_t::display() where the threads are spawned to be a multiple of 32 tiles. That should keep the threads from writing to the same uint32.

[Ters]

Although I do favor keeping the threads away from the same memory, could a spinlock be a cheaper alternative to a mutex in this case?

[prissi]

The penalty of strictly bytewise access is rather low compared to locking. (See last commetn from me on the linked thread.) Furthermore, even a race conditions would refresh likely still the content of that box. (It is drawn anyway, just not copied, should the bit be empty.) So, maybe going back to bytewise access?