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 <b...@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 <b...@nicira.com> wrote:
> > > > > On Fri, Jul 12, 2013 at 2:54 PM, Ben Pfaff <b...@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.
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
