[ovs-dev] [threads v2 13/13] vlog: Make thread-safe.

Andy Zhou azhou at nicira.com
Wed Jul 17 20:11:38 UTC 2013


On Wed, Jul 17, 2013 at 11:42 AM, Ben Pfaff <blp at nicira.com> wrote:

> On Tue, Jul 16, 2013 at 06:11:56PM -0700, Andy Zhou wrote:
> > Review of 11-13 of this patch series.
> >
> > Got a bunch of spare error: warning: Using plain integer as NULL
> pointer,.
> > Most seem to be caused VLOG_RATE_LIMIT_INIT.
>
> Does your tree contain Ethan's patch "atomic: Suppress sparse warning."?
> It is on current master.  I believe that this should avoid the sparse
> warnings.
>
> I ran this based on the github review tree.

> > also:
> > lib/bfd.c:461:52: warning: restricted __be64 degrades to integer
> > lib/bfd.c:461:68: warning: restricted __be32 degrades to integer
> > May be not related to this change.
>
> The latter "bfd" warnings are unrelated.  I pushed a fix earlier this
> morning.
>
O.K. Thanks.

>
> > 2 additional comments inline. Other changes looks good.
> >
> > On Fri, Jul 12, 2013 at 2:54 PM, Ben Pfaff <blp at nicira.com> wrote:
> >
> > > -    /* Close old log file. */
> > > +    /* Log closing old log file (we can't log while holding
> > > log_file_mutex). */
> > >      if (log_fd >= 0) {
> > >          VLOG_INFO("closing log file");
> > > +    }
> > >
> > > log_fd may be changed by other thread, so ideally it should be checked
> > under log_file_mutex. It may be safe to check for (log_fd >= 0) without
> > worry about race conditions, but on the other hand, I don't see a reason
> > not to do it under the mutex.
>
> The comment says why: we can't log while holding log_file_mutex (because
> logging can also take log_file_mutex).  We could do various dodges, but
> it doesn't really seem worth it just to avoid a race on whether we log a
> message about closing a log file.
>
I got it for VLOG_INFO, but it seems we could have cached the value of
log_fd while holding the lock.  It is not a big deal, but seems nicer in
case we support APIs to close a log file without opening a new one
immediately.

>
> > There is another log_fd access around line 985 without lock.
>
> vlog.c only has 985 lines in my tree here, so can you be more specific?
>
> > BTW, can we annotate variable access that needs to be protected by a
> lock?
>
> What kind of annotations do you have in mind?
>
Annotate a variable's access should be done within a lock. Not sure if it
is possible with static analysis tools.

>
> > > -    /* Skip re-opening if it would be a no-op because the old and new
> > > files are
> > > -     * the same.  (This avoids writing "closing log file" followed
> > > immediately
> > > -     * by "opened log file".) */
> > > -    if (log_fd >= 0
> > > -        && !fstat(log_fd, &old)
> > > -        && !stat(log_file_name, &new)
> > > -        && old.st_dev == new.st_dev
> > > -        && old.st_ino == new.st_ino) {
> > > +    if (log_file_name) {
> > >
> > We are accessing log_file_name outside of the lock, why not just check
> fn?
>
> Oops, I changed this code a dozen times and got confused.  I've now
> changed this to use 'fn'.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130717/1d6a4522/attachment-0003.html>


More information about the dev mailing list