Message ID | 20160724150145.GK1018@scylladb.com |
---|---|
State | New |
Headers | show |
On Sun, Jul 24, 2016 at 8:01 AM, Gleb Natapov <gleb@scylladb.com> wrote: > _Unwind_Find_FDE calls _Unwind_Find_registered_FDE and it takes lock even > when there is no registered objects. As far as I see only statically > linked applications call __register_frame_info* functions, so for > dynamically linked executables taking the lock to check unseen_objects > and seen_objects is a pessimization. Since the function is called on > each thrown exception this is a lot of unneeded locking. This patch > checks unseen_objects and seen_objects outside of the lock and returns > earlier if both are NULL. There are problems with this patch. First the stores to unseen_objects and seen_objects are not atomic stores. The second issue is not all targets support atomics because some are single threaded. Another issue is CONSUME memory model should almost never be used; it just decays into acquire anyways. > > diff --git a/libgcc/unwind-dw2-fde.c b/libgcc/unwind-dw2-fde.c > index 5b16a1f..9af558d 100644 > --- a/libgcc/unwind-dw2-fde.c > +++ b/libgcc/unwind-dw2-fde.c > @@ -1001,6 +1001,10 @@ _Unwind_Find_FDE (void *pc, struct dwarf_eh_bases *bases) > struct object *ob; > const fde *f = NULL; > > + if (!__atomic_load_n(&unseen_objects, __ATOMIC_CONSUME) && !__atomic_load_n(&seen_objects, __ATOMIC_CONSUME)) { > + return NULL; > + } There is formatting issues too. if (!__atomic_load_n(&unseen_objects, __ATOMIC_CONSUME) && !__atomic_load_n(&seen_objects, __ATOMIC_CONSUME)) { return NULL; } is the correct formating. > + > init_object_mutex_once (); > __gthread_mutex_lock (&object_mutex); > > @@ -1020,8 +1024,8 @@ _Unwind_Find_FDE (void *pc, struct dwarf_eh_bases *bases) > while ((ob = unseen_objects)) > { > struct object **p; > + struct object *next = ob->next; > > - unseen_objects = ob->next; > f = search_object (ob, pc); > > /* Insert the object into the classified list. */ > @@ -1031,6 +1035,8 @@ _Unwind_Find_FDE (void *pc, struct dwarf_eh_bases *bases) > ob->next = *p; > *p = ob; > > + unseen_objects = next; This store should be an atomic store to be paired with the loads. Thanks, Andrew > + > if (f) > goto fini; > } > -- > Gleb.
Thank you for prompt review. On Sun, Jul 24, 2016 at 11:00:53AM -0700, Andrew Pinski wrote: > On Sun, Jul 24, 2016 at 8:01 AM, Gleb Natapov <gleb@scylladb.com> wrote: > > _Unwind_Find_FDE calls _Unwind_Find_registered_FDE and it takes lock even > > when there is no registered objects. As far as I see only statically > > linked applications call __register_frame_info* functions, so for > > dynamically linked executables taking the lock to check unseen_objects > > and seen_objects is a pessimization. Since the function is called on > > each thrown exception this is a lot of unneeded locking. This patch > > checks unseen_objects and seen_objects outside of the lock and returns > > earlier if both are NULL. > > There are problems with this patch. > First the stores to unseen_objects and seen_objects are not atomic stores. They are not atomic yes, but they are in locked section so there is a release after the store which should order writes to both of them. I can use __atomic_store_n for storing but I do not see (yet) why it is needed. > The second issue is not all targets support atomics because some are > single threaded. __atomic builtins should compile to regular stores/loads on those, no? If __atomic builtins cannot be used on those platforms can this be detected with preprocessor macros somehow? > Another issue is CONSUME memory model should almost never be used; it > just decays into acquire anyways. > > Hmm, yes, not sure why I used CONSUME here instead of ACQUIRE. > > > > diff --git a/libgcc/unwind-dw2-fde.c b/libgcc/unwind-dw2-fde.c > > index 5b16a1f..9af558d 100644 > > --- a/libgcc/unwind-dw2-fde.c > > +++ b/libgcc/unwind-dw2-fde.c > > @@ -1001,6 +1001,10 @@ _Unwind_Find_FDE (void *pc, struct dwarf_eh_bases *bases) > > struct object *ob; > > const fde *f = NULL; > > > > + if (!__atomic_load_n(&unseen_objects, __ATOMIC_CONSUME) && !__atomic_load_n(&seen_objects, __ATOMIC_CONSUME)) { > > + return NULL; > > + } > > There is formatting issues too. > if (!__atomic_load_n(&unseen_objects, __ATOMIC_CONSUME) > && !__atomic_load_n(&seen_objects, __ATOMIC_CONSUME)) > { > return NULL; > } > > is the correct formating. > OK. > > + > > init_object_mutex_once (); > > __gthread_mutex_lock (&object_mutex); > > > > @@ -1020,8 +1024,8 @@ _Unwind_Find_FDE (void *pc, struct dwarf_eh_bases *bases) > > while ((ob = unseen_objects)) > > { > > struct object **p; > > + struct object *next = ob->next; > > > > - unseen_objects = ob->next; > > f = search_object (ob, pc); > > > > /* Insert the object into the classified list. */ > > @@ -1031,6 +1035,8 @@ _Unwind_Find_FDE (void *pc, struct dwarf_eh_bases *bases) > > ob->next = *p; > > *p = ob; > > > > + unseen_objects = next; > > This store should be an atomic store to be paired with the loads. > This store is inside mutex, so unlock will have RELEASE. > Thanks, > Andrew > > > + > > if (f) > > goto fini; > > } > > -- > > Gleb. -- Gleb.
diff --git a/libgcc/unwind-dw2-fde.c b/libgcc/unwind-dw2-fde.c index 5b16a1f..9af558d 100644 --- a/libgcc/unwind-dw2-fde.c +++ b/libgcc/unwind-dw2-fde.c @@ -1001,6 +1001,10 @@ _Unwind_Find_FDE (void *pc, struct dwarf_eh_bases *bases) struct object *ob; const fde *f = NULL; + if (!__atomic_load_n(&unseen_objects, __ATOMIC_CONSUME) && !__atomic_load_n(&seen_objects, __ATOMIC_CONSUME)) { + return NULL; + } + init_object_mutex_once (); __gthread_mutex_lock (&object_mutex); @@ -1020,8 +1024,8 @@ _Unwind_Find_FDE (void *pc, struct dwarf_eh_bases *bases) while ((ob = unseen_objects)) { struct object **p; + struct object *next = ob->next; - unseen_objects = ob->next; f = search_object (ob, pc); /* Insert the object into the classified list. */ @@ -1031,6 +1035,8 @@ _Unwind_Find_FDE (void *pc, struct dwarf_eh_bases *bases) ob->next = *p; *p = ob; + unseen_objects = next; + if (f) goto fini; }