On Fri, Mar 07, 2014 at 11:05:28AM -0700, Eric Blake wrote:
On 03/03/2014 12:18 PM, Daniel P. Berrange wrote:
Both of these options have a notable performance impact, however, and it is believed that worst case behaviour where the fields are read concurrently with being written would merely result in an mistaken emissions or dropping of the log message in question. This is an acceptable tradeoff for the performance benefit of avoiding locking.
Almost. As long as writes are safe, the worst that can happen is we fail to emit a message that just got enabled, or we emit a message that just got disabled. But had we used locks to avoid this race, and the locks get obtained in reverse order, we would see the same behavior. So the locks add no protection, and eliding them in favor of simpler non-atomic integer ops is a safe action.
If there were only a single writer, then writes would be automatically safe. However, you have multiple writers. Thus, you have the situation that if two writers both try to increment the global serial with no locks or atomic increments, you could end up with the classic symptoms of over- or under-incrementing the global serial. If three threads compete:
start: global is 1 thread one: read global to compute its increment thread two: read global to compute its increment thread two: write the increment, global is now 2 thread three: compare local 0 against global 2, recompute priority thread one: write the increment, global is now 2 thread three: compare local 2 against global 2, no priority recompute
Oops - if thread one changed priority so that thread three should no longer log, we've messed up, and may have LOTS of messages logged that should not have been, rather than just one or two at the race boundary case. So I think your _increments_ need to be atomic, to protect the writers, but those are not the hot path; while the READS of the global can remain unprotected, and those are the actual hot path you are optimizing in this patch.
The writes are already protected by a mutex so I don't think we need to use atomic ops for increment either.
static virLogFilterPtr virLogFilters = NULL; static int virLogNbFilters = 0;
@@ -514,6 +515,7 @@ virLogResetFilters(void) VIR_FREE(virLogFilters[i].match); VIR_FREE(virLogFilters); virLogNbFilters = 0; + virLogFiltersSerial++; return i; }
@@ -569,6 +571,7 @@ virLogDefineFilter(const char *match, virLogFilters[i].priority = priority; virLogFilters[i].flags = flags; virLogNbFilters++; + virLogFiltersSerial++;
These are the increments that I think are not hotpath, and need to be serialized with an atomic increment.
These two functions run with the global log mutex held, so there can only be one writer at a time.
+static void +virLogSourceUpdate(virLogSourcePtr source) +{ + virLogLock();
The common case is that the per-log serial will match the global serial; while holding the lock on the boundary case of an updated global serial is not going to affect hot case.
/* - * check against list of specific logging patterns + * 3 intentionally non-thread safe variable reads. + * Worst case result is a log message is accidentally + * dropped or emitted, if another thread is updating + * log filter list concurrently with a log message
and again, if we WERE thread-safe, the race could have always been won the other way in obtaining the lock, for the same end result, so our lack of thread-safety isn't critical.
*/ - fprio = virLogFiltersCheck(filename, &filterflags); - if (fprio == 0) { - if (priority < virLogDefaultPriority) - emit = false; - } else if (priority < fprio) { - emit = false; - } - - if (!emit) + if (source->serial < virLogFiltersSerial) + virLogSourceUpdate(source); + if (priority < source->priority) goto cleanup; + filterflags = source->flags;
Makes sense.
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|