Change in ...osmo-trx[master]: Logger: global Log mutex is now available from C code

pespin gerrit-no-reply at lists.osmocom.org
Tue Jul 2 13:58:03 UTC 2019


pespin has posted comments on this change. ( https://gerrit.osmocom.org/c/osmo-trx/+/14645 )

Change subject: Logger: global Log mutex is now available from C code
......................................................................


Patch Set 2:

> Patch Set 1: Code-Review+1
> 
> why do we have a log mutex in the first place?  I did some research into multithreadded logging using libosmocore and found that the only critical part would be adding/removing of log targets at runtime (the linked list of log targets).  However, during the actual log output / log write, as long as you don't use LOGPC, IIRC you should be fine.  Every fprintf should be atomic, whether to stdout or to any other file.


Hi, good point. I did some reseach and here are my conclusions:
* Agree, the only "really dangerous" part seems to be log targets.

Some nuisances or weird situations of not using mutex:
* unordered timestamps on generated output (th1 generates timestamp, th2 preempts th1 generates timestamp and prints, th1 prints).
* color tag left opened if color is disabled through VTY (log color is disabled while other thread is in the middle of generating log line).

So the important cases are basically a thread calling LOGPSRC while another calls:
* race conditons during llog_targets_reopen()->log_target_file_reopen()
* logging_vty.c "logging enable", "log syslog local", "log gsmtap", "log stderr", etc. -> log_add_target() -> append to the list osmo_log_target_list
* logging_vty.c "logging disable" -> log_del_target() -> osmo_log_target_list

so I think all in all there should be a mutex. But actually the one in osmo-trx doesn't seem to be covering the VTY commands AFAICT, so it's really not useful against big issues (but it's useful against nuisances explained first).

The steps to procede should be IMHO:
* Keep the mutex for now in osmo-trx to at least solve nuisances.
* Add some sort of multithread support in libosmocore's logging system (by adding an internal mutex around log targets (and list), and enabling using the mutex through a libosmocore public API (such as log_enable_multithread_support(bool enable)).
* Once libosmocore has that multithread support, drop the osmo-trx mutex and use the libosmocore API.


-- 
To view, visit https://gerrit.osmocom.org/c/osmo-trx/+/14645
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-trx
Gerrit-Branch: master
Gerrit-Change-Id: I473e57479f8ae98a84ad00b76ff338f79f732236
Gerrit-Change-Number: 14645
Gerrit-PatchSet: 2
Gerrit-Owner: pespin <pespin at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: laforge <laforge at gnumonks.org>
Gerrit-Reviewer: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Tue, 02 Jul 2019 13:58:03 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190702/e2ac116b/attachment.html>


More information about the gerrit-log mailing list