[libvirt] [PATCH 6/7] Switch to filtering based on log source name instead of filename

[Daniel P. Berrange]

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 :|