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 :|
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list