DISPATCH-1956: rewrite of log.c to avoid most locking #1260
Conversation
|
The two fedora asan builds melted down and hardly any tests passed. This PR branch runs all tests fine for me. |
src/log.c
Outdated
|
|
||
|
|
||
|
|
||
|
|
There was a problem hiding this comment.
Just stating the obvious - we'll need to either pull this comment or move it after the license header once this patch is in the final form.
src/log.c
Outdated
| //If file is not there, return 0. | ||
| // We are not logging an error here since we are already holding the log_source_lock | ||
| // Writing a log message will try to re-obtain the log_source_lock lock and cause a deadlock. | ||
|
|
There was a problem hiding this comment.
For now I'd leave this as-is. We can change that in a follow up patch if necessary.
src/log.c
Outdated
| bool syslog; | ||
| log_sink_t *sink; | ||
| uint64_t severity_histogram[N_LEVEL_INDICES]; | ||
| sys_mutex_t * sink_lock; // 1956: This lock protects operations on this source's sink. |
There was a problem hiding this comment.
formatting: no space between * and sink_lock
src/log.c
Outdated
| static qd_log_source_t *qd_log_source_lh(const char *module) | ||
| static bool log_initialized = false; | ||
|
|
||
| static qd_log_source_t * |
There was a problem hiding this comment.
formatting: return type and function signature on the same line please.
src/log.c
Outdated
|
|
||
| qd_log_source_t *log_source = NEW(qd_log_source_t); | ||
| ZERO(log_source); | ||
| log_source->module = (char*) malloc(strlen(module) + 1); |
There was a problem hiding this comment.
While we're at it, we should move away from the malloc+strcpy pattern to simply using qd_strdup()
src/log.c
Outdated
| void qd_log_initialize(void) | ||
| { | ||
| if(log_initialized) { | ||
| // TODO -- log something here? |
There was a problem hiding this comment.
Just assert(!log_initialized) here - it's a coding bug to call it twice.
src/log.c
Outdated
| // Previously, we needed to obtain these attributes outside of the lock, because | ||
| // of a deadlock issue. | ||
|
|
||
| sys_mutex_lock(src->sink_lock); |
There was a problem hiding this comment.
I don't think this is correct: we shouldn't be protecting the sink because the sink is not changing. The source is being modified - and if any sink attributes need to be modified we simply create a new one and drop the reference to the old sink (which can still be in use by other sources).
There was a problem hiding this comment.
For this I think maybe a per-source read/write lock would be a better choice: take the read lock while logging. Take the write lock during this call.
| entry->level = level; | ||
| entry->file = file ? strdup(file) : 0; | ||
| entry->line = line; | ||
| gettimeofday(&entry->time, NULL); |
There was a problem hiding this comment.
We may need to move the gettimeofday into write_log so we're holding the lock while the timestamp is taken to reduce likelyhood of re-ordering log events.
mgoulish
force-pushed
the
mick-log-rewrite
branch
from
b225ff4 to
14f193b
Compare
3 participants
Please see large comment at start of file that describes approach.
I will remove that comment -- as well as all the "1956" comments -- before this code goes to main.