Change in ...libosmocore[master]: logging: Introduce mutex API to manage log_target in multi-thread envs
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/15560 ) Change subject: logging: Introduce mutex API to manage log_target in multi-thread envs .. Patch Set 7: Code-Review+2 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15560 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Id7711893b34263baacac6caf4d489467053131bb Gerrit-Change-Number: 15560 Gerrit-PatchSet: 7 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel Gerrit-Reviewer: laforge Gerrit-Reviewer: lynxis lazus Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Comment-Date: Wed, 09 Oct 2019 17:13:33 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...libosmocore[master]: logging: Introduce mutex API to manage log_target in multi-thread envs
osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/15560 ) Change subject: logging: Introduce mutex API to manage log_target in multi-thread envs .. Patch Set 7: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15560 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Id7711893b34263baacac6caf4d489467053131bb Gerrit-Change-Number: 15560 Gerrit-PatchSet: 7 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel Gerrit-Reviewer: laforge Gerrit-Reviewer: lynxis lazus Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Comment-Date: Wed, 09 Oct 2019 12:38:21 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...libosmocore[master]: logging: Introduce mutex API to manage log_target in multi-thread envs
Hello lynxis lazus, daniel, neels, laforge, osmith, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmocore/+/15560 to look at the new patch set (#7). Change subject: logging: Introduce mutex API to manage log_target in multi-thread envs .. logging: Introduce mutex API to manage log_target in multi-thread envs log_enable_multithread() enables use of locks inside the implementation. Lock use is disabled by default, this way only multi-thread processes need to enable it and suffer related complexity/performance penalties. Locks are required around osmo_log_target_list and items inside it, since targets can be used, modified and deleted by different threads concurrently (for instance, user writing "logging disable" in VTY while another thread is willing to write into that target). Multithread apps and libraries aiming at being used in multithread apps should update their code to use the locks introduced here when containing code iterating over osmo_log_target_list explictly or implicitly by obtaining a log_target (eg. osmo_log_vty2tgt()). Related: OS#4088 Change-Id: Id7711893b34263baacac6caf4d489467053131bb --- M configure.ac M include/osmocom/core/logging.h M libosmocore.pc.in A m4/ax_pthread.m4 M src/Makefile.am M src/gb/gprs_bssgp_vty.c M src/gb/gprs_ns_vty.c M src/logging.c M src/vty/logging_vty.c M tests/Makefile.am M tests/logging/logging_vty_test.c M utils/Makefile.am 12 files changed, 709 insertions(+), 100 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/60/15560/7 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15560 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Id7711893b34263baacac6caf4d489467053131bb Gerrit-Change-Number: 15560 Gerrit-PatchSet: 7 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel Gerrit-Reviewer: laforge Gerrit-Reviewer: lynxis lazus Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-MessageType: newpatchset
Change in ...libosmocore[master]: logging: Introduce mutex API to manage log_target in multi-thread envs
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/15560 ) Change subject: logging: Introduce mutex API to manage log_target in multi-thread envs .. Patch Set 6: (1 comment) https://gerrit.osmocom.org/#/c/15560/6/src/vty/logging_vty.c File src/vty/logging_vty.c: https://gerrit.osmocom.org/#/c/15560/6/src/vty/logging_vty.c@122 PS6, Line 122: > How about placing these two macros in a header file, and using them in > gprs_bssgp_vty. […] I'd prefer not having those defines end up in a header file and eventually being used as a public API. This is a useful in-place macro since 99% of all-ever osmo_log_target related functions are placed in this file. A few more are in logging.c which anyway acquire the lock in a different way. Then, other 2 users in libosmocore (and other libosmocore app users) mainly need to only protect the filter functions, and it's a low amount of them, so it's fine doing what this macro does here manually. -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15560 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Id7711893b34263baacac6caf4d489467053131bb Gerrit-Change-Number: 15560 Gerrit-PatchSet: 6 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel Gerrit-Reviewer: laforge Gerrit-Reviewer: lynxis lazus Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Comment-Date: Wed, 09 Oct 2019 12:18:57 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: osmith Gerrit-MessageType: comment
Change in ...libosmocore[master]: logging: Introduce mutex API to manage log_target in multi-thread envs
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/15560 ) Change subject: logging: Introduce mutex API to manage log_target in multi-thread envs .. Patch Set 6: (1 comment) https://gerrit.osmocom.org/#/c/15560/6/src/logging.c File src/logging.c: https://gerrit.osmocom.org/#/c/15560/6/src/logging.c@1014 PS6, Line 1014: log_tgt_mutex_lock(); > Shouldn't this be log_tgt_mutex_unlock()? Thanks! you saved me/us a lot of debugging time in the future ;) -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15560 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Id7711893b34263baacac6caf4d489467053131bb Gerrit-Change-Number: 15560 Gerrit-PatchSet: 6 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel Gerrit-Reviewer: laforge Gerrit-Reviewer: lynxis lazus Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Comment-Date: Wed, 09 Oct 2019 12:19:41 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: osmith Gerrit-MessageType: comment
Change in ...libosmocore[master]: logging: Introduce mutex API to manage log_target in multi-thread envs
osmith has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/15560 ) Change subject: logging: Introduce mutex API to manage log_target in multi-thread envs .. Patch Set 6: Code-Review-1 (3 comments) -1 for lock instead of unlock https://gerrit.osmocom.org/#/c/15560/6/src/logging.c File src/logging.c: https://gerrit.osmocom.org/#/c/15560/6/src/logging.c@924 PS6, Line 924: /*! Find a registered log target. (unrelated change) https://gerrit.osmocom.org/#/c/15560/6/src/logging.c@1014 PS6, Line 1014: log_tgt_mutex_lock(); Shouldn't this be log_tgt_mutex_unlock()? https://gerrit.osmocom.org/#/c/15560/6/src/vty/logging_vty.c File src/vty/logging_vty.c: https://gerrit.osmocom.org/#/c/15560/6/src/vty/logging_vty.c@122 PS6, Line 122: How about placing these two macros in a header file, and using them in gprs_bssgp_vty.c and gprs_ns_vty.c too? -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15560 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Id7711893b34263baacac6caf4d489467053131bb Gerrit-Change-Number: 15560 Gerrit-PatchSet: 6 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: daniel Gerrit-Reviewer: laforge Gerrit-Reviewer: lynxis lazus Gerrit-Reviewer: neels Gerrit-Reviewer: osmith Gerrit-Reviewer: pespin Gerrit-Comment-Date: Wed, 09 Oct 2019 08:12:55 + Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...libosmocore[master]: logging: Introduce mutex API to manage log_target in multi-thread envs
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/15560 ) Change subject: logging: Introduce mutex API to manage log_target in multi-thread envs .. Patch Set 6: Code-Review+1 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15560 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Id7711893b34263baacac6caf4d489467053131bb Gerrit-Change-Number: 15560 Gerrit-PatchSet: 6 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Reviewer: pespin Gerrit-CC: neels Gerrit-Comment-Date: Sat, 28 Sep 2019 10:49:43 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...libosmocore[master]: logging: Introduce mutex API to manage log_target in multi-thread envs
Hello laforge, Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmocore/+/15560 to look at the new patch set (#6). Change subject: logging: Introduce mutex API to manage log_target in multi-thread envs .. logging: Introduce mutex API to manage log_target in multi-thread envs log_enable_multithread() enables use of locks inside the implementation. Lock use is disabled by default, this way only multi-thread processes need to enable it and suffer related complexity/performance penalties. Locks are required around osmo_log_target_list and items inside it, since targets can be used, modified and deleted by different threads concurrently (for instance, user writing "logging disable" in VTY while another thread is willing to write into that target). Multithread apps and libraries aiming at being used in multithread apps should update their code to use the locks introduced here when containing code iterating over osmo_log_target_list explictly or implicitly by obtaining a log_target (eg. osmo_log_vty2tgt()). Related: OS#4088 Change-Id: Id7711893b34263baacac6caf4d489467053131bb --- M configure.ac M include/osmocom/core/logging.h M libosmocore.pc.in A m4/ax_pthread.m4 M src/Makefile.am M src/gb/gprs_bssgp_vty.c M src/gb/gprs_ns_vty.c M src/logging.c M src/vty/logging_vty.c M tests/Makefile.am M tests/logging/logging_vty_test.c M utils/Makefile.am 12 files changed, 710 insertions(+), 101 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/60/15560/6 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15560 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Id7711893b34263baacac6caf4d489467053131bb Gerrit-Change-Number: 15560 Gerrit-PatchSet: 6 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Reviewer: pespin Gerrit-CC: neels Gerrit-MessageType: newpatchset
Change in ...libosmocore[master]: logging: Introduce mutex API to manage log_target in multi-thread envs
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/15560 ) Change subject: logging: Introduce mutex API to manage log_target in multi-thread envs .. Patch Set 5: Code-Review-1 I believe you need to introduce a library dependency to -lpthread as you're calling pthread functions. See http://git.osmocom.org/libosmocore/commit/?h=laforge/it_q=1d1fc137db5f309d28bfa300014e78d229fc26e5 for a not-yet-merged change of mine doing the same as it needs pthread -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15560 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Id7711893b34263baacac6caf4d489467053131bb Gerrit-Change-Number: 15560 Gerrit-PatchSet: 5 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: laforge Gerrit-Reviewer: pespin Gerrit-CC: neels Gerrit-Comment-Date: Fri, 20 Sep 2019 15:21:06 + Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment
Change in ...libosmocore[master]: logging: Introduce mutex API to manage log_target in multi-thread envs
Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmocore/+/15560 to look at the new patch set (#5). Change subject: logging: Introduce mutex API to manage log_target in multi-thread envs .. logging: Introduce mutex API to manage log_target in multi-thread envs log_enable_multithread() enables use of locks inside the implementation. Lock use is disabled by default, this way only multi-thread processes need to enable it and suffer related complexity/performance penalties. Locks are required around osmo_log_target_list and items inside it, since targets can be used, modified and deleted by different threads concurrently (for instance, user writing "logging disable" in VTY while another thread is willing to write into that target). Multithread apps and libraries aiming at being used in multithread apps should update their code to use the locks introduced here when containing code iterating over osmo_log_target_list explictly or implicitly by obtaining a log_target (eg. osmo_log_vty2tgt()). Related: OS#4088 Change-Id: Id7711893b34263baacac6caf4d489467053131bb --- M include/osmocom/core/logging.h M src/gb/gprs_bssgp_vty.c M src/gb/gprs_ns_vty.c M src/logging.c M src/vty/logging_vty.c M tests/logging/logging_vty_test.c 6 files changed, 213 insertions(+), 93 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/60/15560/5 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15560 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Id7711893b34263baacac6caf4d489467053131bb Gerrit-Change-Number: 15560 Gerrit-PatchSet: 5 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin Gerrit-CC: neels Gerrit-MessageType: newpatchset
Change in ...libosmocore[master]: logging: Introduce mutex API to manage log_target in multi-thread envs
Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmocore/+/15560 to look at the new patch set (#4). Change subject: logging: Introduce mutex API to manage log_target in multi-thread envs .. logging: Introduce mutex API to manage log_target in multi-thread envs log_enable_multithread() enables use of locks inside the implementation. Lock use is disabled by default, this way only multi-thread processes need to enable it and suffer related complexity/performance penalties. Locks are required around osmo_log_target_list and items inside it, since targets can be used, modified and deleted by different threads concurrently (for instance, user writing "logging disable" in VTY while another thread is willing to write into that target). Multithread apps and libraries aiming at being used in multithread apps should update their code to use the locks introduced here when containing code iterating over osmo_log_target_list explictly or implicitly by obtaining a log_target (eg. osmo_log_vty2tgt()). Related: OS#4088 Change-Id: Id7711893b34263baacac6caf4d489467053131bb --- M include/osmocom/core/logging.h M src/gb/gprs_bssgp_vty.c M src/gb/gprs_ns_vty.c M src/logging.c M src/vty/logging_vty.c M tests/logging/logging_vty_test.c 6 files changed, 213 insertions(+), 93 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/60/15560/4 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15560 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Id7711893b34263baacac6caf4d489467053131bb Gerrit-Change-Number: 15560 Gerrit-PatchSet: 4 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin Gerrit-CC: neels Gerrit-MessageType: newpatchset
Change in ...libosmocore[master]: logging: Introduce mutex API to manage log_target in multi-thread envs
neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/15560 ) Change subject: logging: Introduce mutex API to manage log_target in multi-thread envs .. Patch Set 3: (1 comment) I hope this isn't annoying you -- but... https://gerrit.osmocom.org/#/c/15560/2/include/osmocom/core/logging.h File include/osmocom/core/logging.h: https://gerrit.osmocom.org/#/c/15560/2/include/osmocom/core/logging.h@392 PS2, Line 392: void log_tgt_mutex_unlock(void); > Same as explained above. […] I understand. Yet, even easier would be without recompile. Evaluating a bool is not a performance penalty. In an environment like a sysmoBTS, or maybe even just if users installed deb packages and are working on that, it can be really cumbersome to have to recompile libosmocore (and everything else) just to switch on debugging. We have similar debugging bools, see for example osmo_fsm_log_addr(). No reply on the use of different kinds of symbols? I know you can do better ;) Not sure if it's worth spending time and discussing, but I believe my two points are valid... -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15560 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Id7711893b34263baacac6caf4d489467053131bb Gerrit-Change-Number: 15560 Gerrit-PatchSet: 3 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin Gerrit-CC: neels Gerrit-Comment-Date: Wed, 18 Sep 2019 13:48:57 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: pespin Comment-In-Reply-To: neels Gerrit-MessageType: comment
Change in ...libosmocore[master]: logging: Introduce mutex API to manage log_target in multi-thread envs
Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmocore/+/15560 to look at the new patch set (#3). Change subject: logging: Introduce mutex API to manage log_target in multi-thread envs .. logging: Introduce mutex API to manage log_target in multi-thread envs * log_enable_multithread() enables use of locks inside the implementation. Lock use is disabled by default, this way only multi-thread processes need to enable it and suffer related complexity/performance penalties. Locks are required around osmo_log_target_list and items inside it, since targets can be used, modified and deleted by different threads concurrently (for instance, user writing "logging disable" in VTY while another thread is willing to write into that target). Multithread apps and libraries aiming at being used in multithread apps should update their code to use the locks introduced here when containing code iterating over osmo_log_target_list explictly or implicitly by obtaining a log_target (eg. osmo_log_vty2tgt()). Related: OS#4088 Change-Id: Id7711893b34263baacac6caf4d489467053131bb --- M include/osmocom/core/logging.h M src/gb/gprs_bssgp_vty.c M src/gb/gprs_ns_vty.c M src/logging.c M src/vty/logging_vty.c M tests/logging/logging_vty_test.c 6 files changed, 219 insertions(+), 93 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/60/15560/3 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15560 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Id7711893b34263baacac6caf4d489467053131bb Gerrit-Change-Number: 15560 Gerrit-PatchSet: 3 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin Gerrit-CC: neels Gerrit-MessageType: newpatchset
Change in ...libosmocore[master]: logging: Introduce mutex API to manage log_target in multi-thread envs
pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/15560 ) Change subject: logging: Introduce mutex API to manage log_target in multi-thread envs .. Patch Set 2: (2 comments) https://gerrit.osmocom.org/#/c/15560/2/include/osmocom/core/logging.h File include/osmocom/core/logging.h: https://gerrit.osmocom.org/#/c/15560/2/include/osmocom/core/logging.h@383 PS2, Line 383: #define LOG_MTX_DEBUG 0 > would be nicer if using programs could somehow set LOG_MTX_DEBUG 1 without > re-installing entire libo […] I prefer to keep it compile-time only, I don't want to introduce more unneeded performance penalties. This feature is more a handy development/debugging tool in case a deadlock is detected somewhere during development (for instance, adding/changing a logging vty command and forgetting an unlock()). https://gerrit.osmocom.org/#/c/15560/2/include/osmocom/core/logging.h@392 PS2, Line 392: void log_tgt_mutex_unlock(void); > I guess it would be cleaner if both cases created symbols of the same kind. > i.e. […] Same as explained above. The LOG_MTX_DEBUG case is not intended to be ever deployed in normal cases, simply some code easy to enable and rebuild which can help in some specific cases. Converting the symbol to a define and rebuilding libosmocore (+ possibly app) allows for prints to be executed without changing any other code and avoid performance penalties in 99.9% of the times this function is going to ever be called. I could have kept this privat eto myself but it was useful to debug an issue I introduced while writing the patch, and I thought it'd be nice to have feature if someone have a similar issue in the future or wants to understand better how locking works here. -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15560 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Id7711893b34263baacac6caf4d489467053131bb Gerrit-Change-Number: 15560 Gerrit-PatchSet: 2 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-Reviewer: pespin Gerrit-CC: neels Gerrit-Comment-Date: Wed, 18 Sep 2019 10:33:56 + Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: neels Gerrit-MessageType: comment
Change in ...libosmocore[master]: logging: Introduce mutex API to manage log_target in multi-thread envs
neels has posted comments on this change. (
https://gerrit.osmocom.org/c/libosmocore/+/15560 )
Change subject: logging: Introduce mutex API to manage log_target in
multi-thread envs
..
Patch Set 2:
(14 comments)
https://bholley.net/images/posts/thistall.jpg
Looks sane to me, but I'm not seeing the bigger picture really.
Have never written multithreaded osmo code.
Just some style comments...
https://gerrit.osmocom.org/#/c/15560/2/include/osmocom/core/logging.h
File include/osmocom/core/logging.h:
https://gerrit.osmocom.org/#/c/15560/2/include/osmocom/core/logging.h@383
PS2, Line 383: #define LOG_MTX_DEBUG 0
would be nicer if using programs could somehow set LOG_MTX_DEBUG 1 without
re-installing entire libosmocore. That would be a bool then evaluated at
runtime. Would be ok, no?
https://gerrit.osmocom.org/#/c/15560/2/include/osmocom/core/logging.h@392
PS2, Line 392: void log_tgt_mutex_unlock(void);
I guess it would be cleaner if both cases created symbols of the same kind.
i.e. not either #defines or actual functions (bin symbols) depending on the
#define value. So that the list of function names in the resulting library
remain unchanged.
https://gerrit.osmocom.org/#/c/15560/2/src/logging.c
File src/logging.c:
https://gerrit.osmocom.org/#/c/15560/2/src/logging.c@67
PS2, Line 67: /*! This mutex must be hold while using osmo_log_target_list or
any of its
"must be held"
add "in multithreaded programs"
https://gerrit.osmocom.org/#/c/15560/2/src/logging.c@71
PS2, Line 71: #if (!EMBEDDED)
rather put this #if above the API comment, not sure if doxygen picks it up
properly otherwise
https://gerrit.osmocom.org/#/c/15560/2/src/logging.c@88
PS2, Line 88: #if LOG_MTX_DEBUG
(IMHO quite ugly, a plain bool would be much easier to read)
https://gerrit.osmocom.org/#/c/15560/2/src/logging.c@93
PS2, Line 93: /* These lines are useful to debug scenarios where there's only 1
thread and a
wrong indent
missing ' *' each line
https://gerrit.osmocom.org/#/c/15560/2/src/logging.c@930
PS2, Line 930: /*! Find a registered log target. Must be called with mutex
locked.
plz explicitly name the mutex for easier code grepping
https://gerrit.osmocom.org/#/c/15560/2/src/vty/logging_vty.c
File src/vty/logging_vty.c:
https://gerrit.osmocom.org/#/c/15560/2/src/vty/logging_vty.c@105
PS2, Line 105:Lock must be released later with log_tgt_mutex_unlock() */
API doc comment '/*!'
missing ' *'
https://gerrit.osmocom.org/#/c/15560/2/src/vty/logging_vty.c@106
PS2, Line 106: #define ACQUIRE_VTY_LOG_TGT_WITH_LOCK(vty, tgt) do { \
(maybe "LOCK_LOG_TGT_OR_RET_WARNING()" to also highlight the enclosed return?)
https://gerrit.osmocom.org/#/c/15560/2/src/vty/logging_vty.c@113
PS2, Line 113: } while (0)
(IMHO last line should have one less indent)
https://gerrit.osmocom.org/#/c/15560/2/src/vty/logging_vty.c@115
PS2, Line 115: #define RET_WITH_UNLOCK(ret) do { \
(maybe "UNLOCK_LOG_TGT_AND_RET()" to also have "LOG_TGT" in the name and be
similar to above name?)
https://gerrit.osmocom.org/#/c/15560/2/src/vty/logging_vty.c@118
PS2, Line 118: } while (0)
(IMHO middle two lines should have one more indent)
https://gerrit.osmocom.org/#/c/15560/2/src/vty/logging_vty.c@244
PS2, Line 244: ACQUIRE_VTY_LOG_TGT_WITH_LOCK(vty, tgt);
-- gerrit displays this diff wrong for me, silently swallows >200 lines --
-- gosh, those lines appear further down, at around line 1000 instead... o_O --
https://gerrit.osmocom.org/#/c/15560/2/src/vty/logging_vty.c@930
PS2, Line 930: if (tgt->print_ext_timestamp)
explicitly name the mutex for easier grepping
--
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15560
To unsubscribe, or for help writing mail filters, visit
https://gerrit.osmocom.org/settings
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: Id7711893b34263baacac6caf4d489467053131bb
Gerrit-Change-Number: 15560
Gerrit-PatchSet: 2
Gerrit-Owner: pespin
Gerrit-Reviewer: Jenkins Builder
Gerrit-CC: neels
Gerrit-Comment-Date: Wed, 18 Sep 2019 00:37:02 +
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Change in ...libosmocore[master]: logging: Introduce mutex API to manage log_target in multi-thread envs
Hello Jenkins Builder, I'd like you to reexamine a change. Please visit https://gerrit.osmocom.org/c/libosmocore/+/15560 to look at the new patch set (#2). Change subject: logging: Introduce mutex API to manage log_target in multi-thread envs .. logging: Introduce mutex API to manage log_target in multi-thread envs * log_enable_multithread() enables use of locks inside the implementation. Lock use is disabled by default, this way only multi-thread processes need to enable it and suffer related complexity/performance penalties. Locks are required around osmo_log_target_list and items inside it, since targets can be used, modified and deleted by different threads concurrently (for instance, user writing "logging disable" in VTY while another thread is willing to write into that target). Multithread apps and libraries aiming at being used in multithread apps should update their code to use the locks introduced here when containing code iterating over osmo_log_target_list explictly or implicitly by obtaining a log_target (eg. osmo_log_vty2tgt()). Related: OS#4088 Change-Id: Id7711893b34263baacac6caf4d489467053131bb --- M include/osmocom/core/logging.h M src/gb/gprs_bssgp_vty.c M src/gb/gprs_ns_vty.c M src/logging.c M src/vty/logging_vty.c M tests/logging/logging_vty_test.c 6 files changed, 210 insertions(+), 93 deletions(-) git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/60/15560/2 -- To view, visit https://gerrit.osmocom.org/c/libosmocore/+/15560 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: libosmocore Gerrit-Branch: master Gerrit-Change-Id: Id7711893b34263baacac6caf4d489467053131bb Gerrit-Change-Number: 15560 Gerrit-PatchSet: 2 Gerrit-Owner: pespin Gerrit-Reviewer: Jenkins Builder Gerrit-MessageType: newpatchset
Change in ...libosmocore[master]: logging: Introduce mutex API to manage log_target in multi-thread envs
pespin has uploaded this change for review. (
https://gerrit.osmocom.org/c/libosmocore/+/15560
Change subject: logging: Introduce mutex API to manage log_target in
multi-thread envs
..
logging: Introduce mutex API to manage log_target in multi-thread envs
* log_enable_multithread() enables use of locks inside the
implementation. Lock use is disabled by default, this way only
multi-thread processes need to enable it and suffer related
complexity/performance penalties.
Locks are required around osmo_log_target_list and items inside it,
since targets can be used, modified and deleted by different threads
concurrently (for instance, user writing "logging disable" in VTY while
another thread is willing to write into that target).
Multithread apps and libraries aiming at being used in multithread apps
should update their code to use the locks introduced here when
containing code iterating over osmo_log_target_list explictly or
implicitly by obtaining a log_target (eg. osmo_log_vty2tgt()).
Related: OS#4088
Change-Id: Id7711893b34263baacac6caf4d489467053131bb
---
M include/osmocom/core/logging.h
M src/gb/gprs_bssgp_vty.c
M src/gb/gprs_ns_vty.c
M src/logging.c
M src/vty/logging_vty.c
M tests/logging/logging_vty_test.c
6 files changed, 202 insertions(+), 93 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/libosmocore refs/changes/60/15560/1
diff --git a/include/osmocom/core/logging.h b/include/osmocom/core/logging.h
index c2648f3..73df8e9 100644
--- a/include/osmocom/core/logging.h
+++ b/include/osmocom/core/logging.h
@@ -378,4 +378,18 @@
struct log_target *log_target_find(int type, const char *fname);
+void log_enable_multithread(void);
+
+#define LOG_MTX_DEBUG 0
+#if LOG_MTX_DEBUG
+ #include
+ void _log_tgt_mutex_lock(void);
+ void _log_tgt_mutex_unlock(void);
+ #define log_tgt_mutex_lock() do { fprintf(stderr, "[%lu] %s:%d [%s]
lock\n", pthread_self(), __FILE__, __LINE__, __func__); _log_tgt_mutex_lock();
} while (0)
+ #define log_tgt_mutex_unlock() do { fprintf(stderr, "[%lu] %s:%d [%s]
unlock\n", pthread_self(), __FILE__, __LINE__, __func__);
_log_tgt_mutex_unlock(); } while (0)
+#else
+ void log_tgt_mutex_lock(void);
+ void log_tgt_mutex_unlock(void);
+#endif
+
/*! @} */
diff --git a/src/gb/gprs_bssgp_vty.c b/src/gb/gprs_bssgp_vty.c
index 3af6517..5dab94e 100644
--- a/src/gb/gprs_bssgp_vty.c
+++ b/src/gb/gprs_bssgp_vty.c
@@ -181,21 +181,27 @@
"BVCI of the BVC to be filtered\n"
"BSSGP Virtual Connection Identifier (BVCI)\n")
{
- struct log_target *tgt = osmo_log_vty2tgt(vty);
+ struct log_target *tgt;
struct bssgp_bvc_ctx *bvc;
uint16_t nsei = atoi(argv[0]);
uint16_t bvci = atoi(argv[1]);
- if (!tgt)
+ log_tgt_mutex_lock();
+ tgt = osmo_log_vty2tgt(vty);
+ if (!tgt) {
+ log_tgt_mutex_unlock();
return CMD_WARNING;
+ }
bvc = btsctx_by_bvci_nsei(bvci, nsei);
if (!bvc) {
vty_out(vty, "No BVC by that identifier%s", VTY_NEWLINE);
+ log_tgt_mutex_unlock();
return CMD_WARNING;
}
log_set_bvc_filter(tgt, bvc);
+ log_tgt_mutex_unlock();
return CMD_SUCCESS;
}
diff --git a/src/gb/gprs_ns_vty.c b/src/gb/gprs_ns_vty.c
index 53c71a9..4a90436 100644
--- a/src/gb/gprs_ns_vty.c
+++ b/src/gb/gprs_ns_vty.c
@@ -587,12 +587,16 @@
"Identify NS-VC by NSVCI\n"
"Numeric identifier\n")
{
- struct log_target *tgt = osmo_log_vty2tgt(vty);
+ struct log_target *tgt;
struct gprs_nsvc *nsvc;
uint16_t id = atoi(argv[1]);
- if (!tgt)
+ log_tgt_mutex_lock();
+ tgt = osmo_log_vty2tgt(vty);
+ if (!tgt) {
+ log_tgt_mutex_unlock();
return CMD_WARNING;
+ }
if (!strcmp(argv[0], "nsei"))
nsvc = gprs_nsvc_by_nsei(vty_nsi, id);
@@ -601,10 +605,12 @@
if (!nsvc) {
vty_out(vty, "No NS-VC by that identifier%s", VTY_NEWLINE);
+ log_tgt_mutex_unlock();
return CMD_WARNING;
}
log_set_nsvc_filter(tgt, nsvc);
+ log_tgt_mutex_unlock();
return CMD_SUCCESS;
}
diff --git a/src/logging.c b/src/logging.c
index 1c3544f..57d6ee4 100644
--- a/src/logging.c
+++ b/src/logging.c
@@ -42,6 +42,7 @@
#include
#include
#include
+#include
#include
#include
@@ -63,6 +64,54 @@
void *tall_log_ctx = NULL;
LLIST_HEAD(osmo_log_target_list);
+/*! This mutex must be hold while using osmo_log_target_list or any of its
+ log_targets. Prevents race conditions between threads like producing
+ unordered timestamps or VTY deleting a target while another thread is
writing
+ to it */
+pthread_mutex_t osmo_log_tgt_mutex;
+bool osmo_log_tgt_mutex_on = false;
+
+/*! Enable multithread support (mutex) in libosmocore logging
