> 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 <yamam...@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?
YAMAMOTO Takashi
diff --git a/ofproto/connmgr.c b/ofproto/connmgr.c
index 6b1ed22..1eb6ba6 100644
--- a/ofproto/connmgr.c
+++ b/ofproto/connmgr.c
@@ -1120,10 +1120,23 @@ ofconn_pktbuf_retrieve(struct ofconn *ofconn, uint32_t
id,
}
/* Returns true if 'ofconn' has any pending flow monitor updates. */
+static bool
+ofconn_has_pending_monitor__(const struct ofconn *ofconn)
+{
+
+ ovs_assert(list_is_empty(&ofconn->updates));
+ return ofconn->monitor_paused;
+}
+
bool
ofconn_has_pending_monitor(const struct ofconn *ofconn)
{
- return !list_is_empty(&ofconn->updates) || ofconn->monitor_paused;
+ bool result;
+
+ ovs_mutex_lock(&ofproto_mutex);
+ result = ofconn_has_pending_monitor__(ofconn);
+ ovs_mutex_unlock(&ofproto_mutex);
+ return result;
}
/* Returns true if 'ofconn' has any pending opgroups. */
@@ -2055,7 +2068,7 @@ ofmonitor_has_pending(struct connmgr *mgr)
struct ofconn *ofconn;
LIST_FOR_EACH (ofconn, node, &mgr->all_conns) {
- if (ofconn_has_pending_monitor(ofconn)) {
+ if (ofconn_has_pending_monitor__(ofconn)) {
return true;
}
}
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
