[ovs-dev] [PATCH] ofproto: fix interactions between flow monitors and barriers

Ben Pfaff blp at nicira.com
Tue Feb 4 21:32:01 UTC 2014


On Fri, Jan 17, 2014 at 11:04:14AM +0900, YAMAMOTO Takashi wrote:
> > On Wed, Jan 08, 2014 at 07:31:23PM +0900, YAMAMOTO Takashi wrote:
> >> Following OpenFlow 1.4 semantics, make barriers wait for
> >> flow monitor replies.  This should fix a race in
> >> "ofproto - flow monitoring pause and resume" test.
> >> 
> >> Signed-off-by: YAMAMOTO Takashi <yamamoto at valinux.co.jp>
> > 
> > Clang says:
> > 
> >     ../ofproto/connmgr.c:1126:56: error: reading variable 'monitor_paused' requires locking 'ofproto_mutex' [-Werror,-Wthread-safety-analysis]
> >         return !list_is_empty(&ofconn->updates) || ofconn->monitor_paused;
> >                                                            ^
> > 
> > But I do not think that this change is actually necessary.  The
> > 'updates' member is only nonempty during the processing of a
> > flow_mod.  By the time the flow_mod is actually committed to the flow
> > table, all of the updates have been flushed to the ofconns anyhow.
> > 
> > Here is another way to look at it.  I can think of two ways that the
> > flow table can change.  The primary way is from the main thread that
> > processes all OpenFlow requests.  That thread sends out the updates
> > synchronously with the flow_mod, so the flow updates will always
> > precede the barrier reply.  The secondary way is from NXAST_LEARN
> > actions.  Such actions are not associated with any ofconn, so any
> > updates that they send don't have to be synchronized with a barrier in
> > any case.
> > 
> > Do you agree?
> 
> at least the ofconn->monitor_paused check is necessary
> because ADDED/MODIFIED events can be generated when a monitor is
> resumed.  i've observed the test mentioned in the commit log
> complained about missing events.
> 
> i think you are right about ofconn->updates.
> how about the following?

Sorry about the delay looking at this.

What version of OVS is this against?  It does not appear to apply to
branch-2.0, branch-2.1, or master.



More information about the dev mailing list