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

[YAMAMOTO Takashi]

> 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.

it was for master at that time.  i'll rebase.

YAMAMOTO Takashi