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

[Andy Zhou]

Thanks for the explanation. May be we could benefit from having *_locked
variants of vlog functions that can be called while holding the
log_file_mutex?


On Wed, Jul 17, 2013 at 1:31 PM, Ben Pfaff <blp at nicira.com> wrote:

> On Wed, Jul 17, 2013 at 01:23:41PM -0700, Ben Pfaff wrote:
> > On Wed, Jul 17, 2013 at 01:11:38PM -0700, Andy Zhou wrote:
> > > On Wed, Jul 17, 2013 at 11:42 AM, Ben Pfaff <blp at nicira.com> wrote:
> > > > > 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.
> >
> > We could cache it, but I don't think that it would help.
>
> I meant to write more here, but forgot before I sent the email.
>
> We could cache it.  Suppose we did.  Then when we release the lock, some
> other thread could come in and close the file.  If it logged it, then
> we'd log it twice.  So I don't see the benefit.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130717/f361c081/attachment-0003.html>